Closed nvkelso closed 5 years ago
@bcamper This PR is cleaned up and ready for new 👀, please!
OK I extracted some repetitive logic in:
f591907cbf414cab6beb4538295b53b92ff1705c 91f3b8f294d41bb124942474bf935b6a002b9a6b
Noticing that the pre-existing language toggle stuff could benefit from this as well, but it looks more complicated because of the many variants (short name, left/right borders, etc.), so I'll leave that for now.
Actually now that I'm looking at these lines like:
priority: |
function() { return (feature['min_zoom'] + (1 - 1 / feature['collision_rank']) * 0.1) || 65; }
I'm realizing the default value of 65 will never be triggered. If collision_rank
is missing, you'll get -Infinity
. If min_zoom
is missing, you'll still get a number, but less than 1
.
What's the desired fallback logic here?
So is this the desired collision priority logic:
min_zoom
and collision_rank
exist, use the formula.min_zoom
exists, use it (or use min_zoom + 1
if features without a collision_rank
should rank lower by default than those that do).But, even then, the fallback values in the style like 65, 58, etc. don't make sense as final priority values when compared with min_zoom
based values that are going to be in the ~1-20 range.
Are those numbers supposed to be defaults for collision_rank
?
You're right... those fallback values are a holdover from before all the collision_rank
were moved server side and spread out with much greater detail. But something like this is still desired. If a feature has a min_zoom
but no collision_rank
then it should be evaluated as worst among peers of the same min_zoom
.
Example:
min_zoom
: 10collision_rank
: 1000min_zoom
: 10collision_rank
: 1001min_zoom
: 10collision_rank
: nullThe outcome should be Feature A wins over B wins over C.
I think this could be accomplished by falling back to feature.min_zoom + 0.999
and call it good?
I've addressed the collision priority changes primarily in 92afcabb466831752f87ca73d17668dd5b57df8f (simplified to remove the defaults), and added logic for road, water, and transit labels (turn transit overlay on to see it in action).
@bcamper This is ready for another review, please. I've addressed the collision priority stuff you called out (and fixed the transit overlay labels in the process), but don't know how to proceed on the https://github.com/tangrams/bubble-wrap/pull/277#discussion_r311863310 so maybe we defer that to a later PR?
@bcamper This is ready for another review, please. I've addressed the collision priority stuff you called out (and fixed the transit overlay labels in the process), but don't know how to proceed on the #277 (comment) so maybe we defer that to a later PR?
Agree we can follow up on this later.
You're right... those fallback values are a holdover from before all the
collision_rank
were moved server side and spread out with much greater detail. But something like this is still desired. If a feature has amin_zoom
but nocollision_rank
then it should be evaluated as worst among peers of the samemin_zoom
. ... I think this could be accomplished by falling back tofeature.min_zoom + 0.999
and call it good?
Yep that works, made some updates in https://github.com/tangrams/bubble-wrap/pull/277/commits/8fdfe3be79e87e5bb48e355a7f30c19c5ba14a3d.
aboriginal_lands
boundary line treatmentgenerator
icons to use kind_detail lookupenclosure
labelsdanger_area
andrange
landuse styling (not RED!)labels-11
theme to not reference all the client side hacks that were fixed server side