Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add POV and other v1.8, v1.7 changes #277

Merged
merged 29 commits into from
Aug 9, 2019
Merged

Conversation

nvkelso
Copy link
Member

@nvkelso nvkelso commented Jul 18, 2019

  • ⭐️Add POV for boundaries and capitals; add map unit lines (eg Wales and Scotland)
    • tilezen_v1d8_disputed_boundaries
  • 🚚Add heavy goods vehicle (hgv) overlay
  • 🤑Add toll road overlay
  • 🚧 Add construction roads
  • 🏞 Add new landuse treatments
  • 👨 Fix aboriginal_lands boundary line treatment
  • 🔋 Fix generator icons to use kind_detail lookup
  • 🐯 Fix zoo enclosure labels
  • 🚘 Fix motorway junction (exit) labels
  • ☠️ Fix danger_area and range landuse styling (not RED!)
  • 🏷 Update labels-11 theme to not reference all the client side hacks that were fixed server side
  • 💻 Remove bunch of logic that moved server side (like for filtering out bad features, setting label collision priority)
  • 🎉 Upgrade Tangram JS to v0.19

bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
@nvkelso nvkelso changed the title WIP: add POV and other v1.8, v1.7 changes add POV and other v1.8, v1.7 changes Jul 31, 2019
@nvkelso
Copy link
Member Author

nvkelso commented Jul 31, 2019

@bcamper This PR is cleaned up and ready for new 👀, please!

Copy link
Member

@bcamper bcamper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great new features and I love all the code removal!

There are a few questions around filter logic or values that are worth addressing, may be one or two unintentional items, but otherwise all looks good.

I'd like to pull out a couple of these oft-repeated lines into globals, to be more concise and lower risk of copy-paste errors, such as:

var kind = (global.ux_point_of_view && feature['kind:'+global.ux_point_of_view]) || (global.ux_point_of_view_fallback && feature['kind:'+global.ux_point_of_view_fallback]) || feature.kind;

and

priority: |
                    function() { return (feature['min_zoom'] + (1 - 1 / feature['collision_rank']) * 0.1) || 65; }

I can take a crack at that.

bubble-wrap-style.yaml Show resolved Hide resolved
bubble-wrap-style.yaml Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Show resolved Hide resolved
bubble-wrap-style.yaml Outdated Show resolved Hide resolved
bubble-wrap-style.yaml Show resolved Hide resolved
bubble-wrap-style.yaml Show resolved Hide resolved
@@ -5120,6 +5036,19 @@ layers:
color: [0.765,0.765,0.765]
visible: true

# # (20190729: nvk) TODO: This should only show when no other kind condition
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this kind of "else" condition can be done with the new exclusive layer property added in JS -- make all the other layers exclusive, and if none matches, the non-exclusive "else" one does -- but it's not in ES yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YES! exclusive would be great here :) So in Tangram ES the layer won't match and won't display? Seems backwards compatible and okay to implement?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the docs in tangrams/tangram#705 I'm confused how I'd implement this. Is there a !exclusive or priority: -1 to mean evaluate this one layer last? Or do I have to add priority to every other sibling layer here, too?

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@bcamper
Copy link
Member

bcamper commented Aug 4, 2019

OK I extracted some repetitive logic in:

f591907
91f3b8f

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.

@bcamper
Copy link
Member

bcamper commented Aug 4, 2019

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?

@bcamper
Copy link
Member

bcamper commented Aug 4, 2019

So is this the desired collision priority logic:

  • If both min_zoom and collision_rank exist, use the formula.
  • If only 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).
  • If neither exist, use the provided default.

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?

@nvkelso
Copy link
Member Author

nvkelso commented Aug 8, 2019

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:

  • Feature A:
    • min_zoom: 10
    • collision_rank: 1000
  • Feature B:
    • min_zoom: 10
    • collision_rank: 1001
  • Feature C:
    • min_zoom: 10
    • collision_rank: null

The 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?

@nvkelso
Copy link
Member Author

nvkelso commented Aug 8, 2019

I've addressed the collision priority changes primarily in 92afcab (simplified to remove the defaults), and added logic for road, water, and transit labels (turn transit overlay on to see it in action).

@nvkelso
Copy link
Member Author

nvkelso commented Aug 8, 2019

@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?

(otherwise it'll divide by null rather than falling back to the desired value)
Copy link
Member

@bcamper bcamper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made an additional fix to the those collision priority functions -- they need to check for null first, otherwise they'll divide by null which will be Infinity instead of falling back to the desired value.

Couple other small questions about things that may have issues, but probably not significant/blocking (please do take a look though).

interactive: false
color: [[13,[0,0,0.5,0.25]],[15,[0,0,0.5,0.8]]]
width: [[11,0.5px],[12,0.8px],[16,1.5px],[18,1.5m]]
# let roads sort themselves past zoom 14
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just assigning an explicit order value though, is the comment incorrect? (or the code?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment in 1da2c65.

We moved the zoom changes server side a few releases ago so the comment was outdated. There look to be a few static int values in the scene file still. I've filed #278 to track that.

bubble-wrap-style.yaml Outdated Show resolved Hide resolved
visible: true
color: blue
width: 1.7px
z-none:
Copy link
Member

@bcamper bcamper Aug 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wondering about this one.

NVK: Removed in 930a049.

@bcamper
Copy link
Member

bcamper commented Aug 9, 2019

@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.

@bcamper
Copy link
Member

bcamper commented Aug 9, 2019

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.
...
I think this could be accomplished by falling back to feature.min_zoom + 0.999 and call it good?

Yep that works, made some updates in 8fdfe3b.

Copy link
Member Author

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed a noticeable regression in the style between prod and this PR by changing order of operations in the collision_priority function in eafad26, which was preventing neighbourhood labels in SF to show up at zooms less than 16 and was preventing Golden Gate Park label from showing up w/r/t roads labels, and too many "bay" labels north of San Francisco.

@nvkelso nvkelso merged commit ab18c14 into gh-pages Aug 9, 2019
@nvkelso nvkelso deleted the nvkelso/v1d8-changes branch August 9, 2019 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants