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

pastk · 2023-02-10T15:04:30Z

pastk commented

2023-02-10 15:04:30 +00:00

This PR removes the hardcoded minVisibleScale = 10 for all road shields.
I didn't see any change for higher zooms (z13-) - probably because shields' priorities are very high there anyway (they use minVisibleScale from the road which is lower than that of shields).
However on z10-z12 this change makes motorway, trunk & primary shields' priority somewhat higher (before the PR they were fixed to z10 bucket, now they're in z7 & z8 buckets).
Summary of priority change (shows drawing elements which became lower priority, than shields):

OVRL/z10/16476	 symbol	 landuse-military-danger_area	 	 z[16-19]
OVRL/z10/16590	 symbol	 natural-volcano	 	 z[10-19]
OVRL/z10/16666	 symbol	 boundary-national_park	 	 z[11-19]
OVRL/z10/16666	 symbol	 leisure-nature_reserve	 	 z[11-19]
OVRL/z9/15000	 caption	 place-island	 	 z[9-19]
OVRL/z8/15523	 caption	 place-town	 if[population<20000]	 z[12-19]
OVRL/z8/15523	 caption	 place-town	 if[population>=0]	 z[10-11]
OVRL/z8/15523	 caption	 place-town	 if[population>=20000]	 z[9]
OVRL/z8/15523	 caption	 place-town	 if[population>=40000]	 z[8]
OVRL/z8/16428	 shield	 highway-primary	 	 z[10-19]
OVRL/z8/16428	 shield	 highway-primary-bridge	 	 z[10-19]
OVRL/z8/16428	 shield	 highway-primary-tunnel	 	 z[10-19]
OVRL/z7/3100	 symbol	 aeroway-aerodrome-international	 	 z[11-13]
OVRL/z7/10000	 symbol	 aeroway-aerodrome-international	 	 z[7-9]
OVRL/z7/15009	 caption	 aeroway-aerodrome-international	 	 z[10-19]
OVRL/z7/16009	 symbol	 aeroway-aerodrome-international	 	 z[10, 14-19]
OVRL/z7/16447	 shield	 highway-motorway	 	 z[10-19]
OVRL/z7/16447	 shield	 highway-motorway-bridge	 	 z[10-19]
OVRL/z7/16447	 shield	 highway-motorway-tunnel	 	 z[10-19]
OVRL/z7/16447	 shield	 highway-trunk	 	 z[10-19]
OVRL/z7/16447	 shield	 highway-trunk-bridge	 	 z[10-19]
OVRL/z7/16447	 shield	 highway-trunk-tunnel	 	 z[10-19]

E.g. motorway/trunk shields will displace international airport and place-town captions now.
(should be easy to make airports prioritized over motorway shields as they're in the same z7).

This PR removes the hardcoded `minVisibleScale = 10` for all road shields. I didn't see any change for higher zooms (z13-) - probably because shields' priorities are very high there anyway (they use minVisibleScale from the road which is lower than that of shields). However on z10-z12 this change makes motorway, trunk & primary shields' priority somewhat higher (before the PR they were fixed to z10 bucket, now they're in z7 & z8 buckets). Summary of priority change (shows drawing elements which became lower priority, than shields): ``` OVRL/z10/16476 symbol landuse-military-danger_area z[16-19] OVRL/z10/16590 symbol natural-volcano z[10-19] OVRL/z10/16666 symbol boundary-national_park z[11-19] OVRL/z10/16666 symbol leisure-nature_reserve z[11-19] OVRL/z9/15000 caption place-island z[9-19] OVRL/z8/15523 caption place-town if[population<20000] z[12-19] OVRL/z8/15523 caption place-town if[population>=0] z[10-11] OVRL/z8/15523 caption place-town if[population>=20000] z[9] OVRL/z8/15523 caption place-town if[population>=40000] z[8] OVRL/z8/16428 shield highway-primary z[10-19] OVRL/z8/16428 shield highway-primary-bridge z[10-19] OVRL/z8/16428 shield highway-primary-tunnel z[10-19] OVRL/z7/3100 symbol aeroway-aerodrome-international z[11-13] OVRL/z7/10000 symbol aeroway-aerodrome-international z[7-9] OVRL/z7/15009 caption aeroway-aerodrome-international z[10-19] OVRL/z7/16009 symbol aeroway-aerodrome-international z[10, 14-19] OVRL/z7/16447 shield highway-motorway z[10-19] OVRL/z7/16447 shield highway-motorway-bridge z[10-19] OVRL/z7/16447 shield highway-motorway-tunnel z[10-19] OVRL/z7/16447 shield highway-trunk z[10-19] OVRL/z7/16447 shield highway-trunk-bridge z[10-19] OVRL/z7/16447 shield highway-trunk-tunnel z[10-19] ``` E.g. motorway/trunk shields will displace international airport and place-town captions now. (should be easy to make airports prioritized over motorway shields as they're in the same z7). ![road-shields_z10-11](https://user-images.githubusercontent.com/18434508/218124624-af237a15-dcc3-4cea-affa-08f6c685df73.png)

biodranik (Migrated from github.com) reviewed 2023-02-10 15:04:30 +00:00

vng (Migrated from github.com) reviewed 2023-02-10 15:27:42 +00:00

drape_frontend/rule_drawer.cpp

					
				@ -363,6 +363,7 @@ void RuleDrawer::ProcessLineStyle(FeatureType & f, Stylist const & s,

				  if (needAdditional && !clippedSplines.empty())

				  {

				    minVisibleScale = feature::GetMinDrawableScale(f);

vng (Migrated from github.com) commented

2023-02-10 15:27:29 +00:00

Why? Is it differ with ApplyLineFeatureGeometry calculation?

pastk reviewed 2023-02-10 16:06:39 +00:00

drape_frontend/rule_drawer.cpp

					
				@ -363,6 +363,7 @@ void RuleDrawer::ProcessLineStyle(FeatureType & f, Stylist const & s,

				  if (needAdditional && !clippedSplines.empty())

				  {

				    minVisibleScale = feature::GetMinDrawableScale(f);

pastk commented

2023-02-10 16:06:39 +00:00

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 (Migrated from github.com) reviewed 2023-02-10 16:14:45 +00:00

drape_frontend/rule_drawer.cpp

					
				@ -363,6 +363,7 @@ void RuleDrawer::ProcessLineStyle(FeatureType & f, Stylist const & s,

				  if (needAdditional && !clippedSplines.empty())

				  {

				    minVisibleScale = feature::GetMinDrawableScale(f);

vng (Migrated from github.com) commented

2023-02-10 16:14:45 +00:00

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 reviewed 2023-02-11 11:53:13 +00:00

drape_frontend/rule_drawer.cpp

					
				@ -363,6 +363,7 @@ void RuleDrawer::ProcessLineStyle(FeatureType & f, Stylist const & s,

				  if (needAdditional && !clippedSplines.empty())

				  {

				    minVisibleScale = feature::GetMinDrawableScale(f);

pastk commented

2023-02-11 11:53:13 +00:00

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 (Migrated from github.com) reviewed 2023-02-14 14:12:53 +00:00

drape_frontend/rule_drawer.cpp

					
				@ -363,6 +363,7 @@ void RuleDrawer::ProcessLineStyle(FeatureType & f, Stylist const & s,

				  if (needAdditional && !clippedSplines.empty())

				  {

				    minVisibleScale = feature::GetMinDrawableScale(f);

vng (Migrated from github.com) commented

2023-02-14 14:12:53 +00:00

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 (Migrated from github.com) reviewed 2023-02-14 14:17:39 +00:00

drape_frontend/rule_drawer.cpp

					
				@ -363,6 +363,7 @@ void RuleDrawer::ProcessLineStyle(FeatureType & f, Stylist const & s,

				  if (needAdditional && !clippedSplines.empty())

				  {

				    minVisibleScale = feature::GetMinDrawableScale(f);