organicmaps/organicmaps-tmp
Archived
13
Fork 1
Code Issues 2.7k Pull requests 182 Projects Releases 65 Wiki Activity Actions

WIP: [drape] Remove hardcoded minVisibleScale = 10 from road shields #4426

Closed
pastk wants to merge 1 commit from pastk-drape-roadshields into master
pull from: pastk-drape-roadshields
merge into: organicmaps:master
Conversation 0 Commits 1 Files changed 2 +4 -4
2 changed files with 4 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
drape_frontend/apply_feature_functors.cpp
View file

@ -63,7 +63,6 @@ df::ColorConstant const kRoadShieldOrangeBackgroundColor = "RoadShieldOrangeBack
uint32_t const kPathTextBaseTextIndex = 128;
uint32_t const kShieldBaseTextIndex = 0;
int const kShieldMinVisibleZoomLevel = 10;
#ifdef LINES_GENERATION_CALC_FILTERED_POINTS
class LinesStat
@ -960,7 +959,7 @@ void ApplyLineFeatureAdditional::GetRoadShieldsViewParams(ref_ptr<dp::TextureMan
textParams.m_depthTestEnabled = false;
textParams.m_depth = m_depth;
textParams.m_depthLayer = DepthLayer::OverlayLayer;
textParams.m_minVisibleScale = kShieldMinVisibleZoomLevel;
textParams.m_minVisibleScale = m_minVisibleScale;
textParams.m_rank = m_rank;
textParams.m_featureId = m_id;
textParams.m_titleDecl.m_anchor = anchor;
@ -991,7 +990,7 @@ void ApplyLineFeatureAdditional::GetRoadShieldsViewParams(ref_ptr<dp::TextureMan
symbolParams.m_depthTestEnabled = true;
symbolParams.m_depth = m_depth;
symbolParams.m_depthLayer = DepthLayer::OverlayLayer;
symbolParams.m_minVisibleScale = kShieldMinVisibleZoomLevel;
symbolParams.m_minVisibleScale = m_minVisibleScale;
symbolParams.m_rank = m_rank;
symbolParams.m_anchor = anchor;
symbolParams.m_offset = shieldOffset;
@ -1019,7 +1018,7 @@ void ApplyLineFeatureAdditional::GetRoadShieldsViewParams(ref_ptr<dp::TextureMan
poiParams.m_depth = m_depth;
poiParams.m_depthTestEnabled = false;
poiParams.m_depthLayer = DepthLayer::OverlayLayer;
poiParams.m_minVisibleScale = kShieldMinVisibleZoomLevel;
poiParams.m_minVisibleScale = m_minVisibleScale;
poiParams.m_rank = m_rank;
poiParams.m_symbolName = symbolName;
poiParams.m_extendingSize = 0;

1
drape_frontend/rule_drawer.cpp
View file

@ -363,6 +363,7 @@ void RuleDrawer::ProcessLineStyle(FeatureType & f, Stylist const & s,
if (needAdditional && !clippedSplines.empty())
{
minVisibleScale = feature::GetMinDrawableScale(f);
vng commented 2023-02-10 15:27:29 +00:00 (Migrated from github.com)
Review
Copy link

Why? Is it differ with ApplyLineFeatureGeometry calculation?

Why? Is it differ with ApplyLineFeatureGeometry calculation?
pastk commented 2023-02-10 16:06:39 +00:00
Review
Copy link

There is no minVisibleScale calculation in ApplyLineFeatureGeometry.
But maybe its actually needed there also... Though in #4428 path_text received their minVisibleScale values from ApplyLineFeatureAdditional.. I'll take a closer look.

There is no minVisibleScale calculation in `ApplyLineFeatureGeometry`. But maybe its actually needed there also... Though in #4428 path_text received their minVisibleScale values from `ApplyLineFeatureAdditional`.. I'll take a closer look.
vng commented 2023-02-10 16:14:45 +00:00 (Migrated from github.com)
Review
Copy link

Ah, yes. I was confused that RuleDrawer::ProcessLineStyle takes minVisibleScale by reference.

So, need to investigate the difference between:

  • minVisibleScale calculation in RuleDrawer::operator()
  • feature::GetMinDrawableScale()
    And decide what should we pass into ApplyLineFeatureGeometry.

Also Point/Line/Area functions are differ in this behaviour. Sometimes they use passed minVisibleScale and sometimes call GetMinDrawableScale. It also looks suspicious for me.

And remove passing it by reference ..

Ah, yes. I was confused that RuleDrawer::ProcessLineStyle takes minVisibleScale by reference. So, need to investigate the difference between: - minVisibleScale calculation in RuleDrawer::operator() - feature::GetMinDrawableScale() And decide what should we pass into ApplyLineFeatureGeometry. Also Point/Line/Area functions are differ in this behaviour. Sometimes they use passed minVisibleScale and sometimes call GetMinDrawableScale. It also looks suspicious for me. And remove passing it by reference ..
pastk commented 2023-02-11 11:53:13 +00:00
Review
Copy link

I double-checked:
ApplyLineFeatureGeometry process line primitives and line symbols (i.e. oneway arrows) only, and both of them don't use minVisibleScale at all.

RuleDrawer::operator() inits minVisibleScale to 0 and then passes it by reference to e.g. point/area/line processors, where it could be set to a real value taken from feature::GetMinDrawableScale() if necessary.

So my code is correct here - it adds a calculation of real minVisibleScale value for line processing only when its really needed (e.g. road shields, path_texts...)

I double-checked: `ApplyLineFeatureGeometry` process line primitives and line symbols (i.e. oneway arrows) only, and both of them don't use minVisibleScale at all. `RuleDrawer::operator()` inits minVisibleScale to 0 and then passes it by reference to e.g. point/area/line processors, where it could be set to a real value taken from `feature::GetMinDrawableScale()` if necessary. So my code is correct here - it adds a calculation of real minVisibleScale value for line processing only when its really needed (e.g. road shields, path_texts...)
vng commented 2023-02-14 14:12:53 +00:00 (Migrated from github.com)
Review
Copy link

Oh, yes, this is one of the most complicated logic I've ever seen :)

  • So, we can call shape->SetFeatureMinZoom(0) with zero and I can't say for sure is it ok or not ..
  • We tried to avoid calling GetMinDrawableScale because it makes ft.GetLimitRect(FeatureType::BEST_GEOMETRY) inside. Not sure, do we really need such honest minVisibleScale with best Feature's limit rect ..

I suspect (but not sure), that we can calculate minVisibleScale according to drawing rules only (without rects) and make this code more simple - just call this scale calculation once.

Oh, yes, this is one of the most complicated logic I've ever seen :) - So, we can call ```shape->SetFeatureMinZoom(0)``` _with zero_ and I can't say for sure is it ok or not .. - We tried to avoid calling GetMinDrawableScale because it makes ```ft.GetLimitRect(FeatureType::BEST_GEOMETRY)``` inside. Not sure, do we really need such _honest_ minVisibleScale with best Feature's limit rect .. I suspect (but not sure), that we can calculate minVisibleScale according to drawing rules only (without rects) and make this code more simple - just call this scale calculation once.
vng commented 2023-02-14 14:17:39 +00:00 (Migrated from github.com)
Review
Copy link

organicmaps/organicmaps#4426

https://git.omaps.dev/organicmaps/organicmaps/pulls/4426
ApplyLineFeatureAdditional applyAdditional(m_context->GetTileKey(), insertShape, f.GetID(),
m_currentScaleGtoP, minVisibleScale, f.GetRank(),
s.GetCaptionDescription(), clippedSplines);