Enable property functions for line-width #4773
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ansis
reviewed
Jun 2, 2017
src/data/bucket/line_bucket.js
Outdated
| @@ -50,6 +50,8 @@ const lineInterface = { | |||
| {property: 'line-opacity', multiplier: 10, type: 'Uint8'}, | |||
| {property: 'line-gap-width', multiplier: 10, type: 'Uint8', name: 'a_gapwidth'}, | |||
| {property: 'line-offset', multiplier: 1, type: 'Int8'}, | |||
| {property: 'line-width', multiplier: 10, type: 'Uint8', name: 'a_width'}, | |||
| {property: 'line-width', multiplier: 10, type: 'Uint8', name: 'a_floorwidth'}, | |||
There was a problem hiding this comment.
This approach looks great!
I wonder if adding a useIntegerZoom: true to the property definition here and using that to making the if (name === 'a_floorwidth') exceptions less exceptional would be good. Or if that would just hide the exception in a bad way.
|
@lbud awesome 🕵️♀️ !! |
|
Thanks for making this a priority. Dynamic line-width is an important feature. |
anandthakker
reviewed
Jun 2, 2017
| "maxzoom": 14, | ||
| "tiles": [ | ||
| "local://tiles/{z}-{x}-{y}.mvt" | ||
| ] |
There was a problem hiding this comment.
I'd be in favor of simplifying these render tests using some bare-bones GeoJSON, as that'll make em easier to understand/assess.
|
@anandthakker thanks for the suggestion — simplified geojson fixtures in 467ed46 |
lbud
changed the title
lbud
changed the title
lbud
changed the title
lbud
force-pushed
the
dds-line-width
branch
2 times, most recently
from
314aeb9
to
0e447be
Compare
added 10 commits
June 19, 2017 12:52
* line-dasharray is scaled by line-width and therefore requires the value of line-width at floor(z). Here, we add an extra phantom paint attribute (the value of line-width at floor(z)) so that we can move those calculations to the shader.
…erty name in ProgramConfiguration
ansis
approved these changes
Jun 19, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref #3045
The challenge in implementing data-driven
line-widthhas been thatline-dasharrayis scaled byline-width, so dasharray calculations require the value of a line's width at floor(zoom). This PR accomplishes data-drivenline-widthby moving that calculation to the shader and adding an additional, sort of phantom paint attribute that calculates the value of line-width at floor(zoom). This required a bit of special casing in program_configuration but given the existing situation seemed to me the best option. This would nullify #4158. @ansis @jfirebaugh @nickidlugash @anandthakker — does this seem reasonable?I verified manually with a matrix of
{ line-width: [constant, zoom, property, zoom-and-property] }x
{ line-dasharray: [none, constant, zoom] }and added render tests here to the same effect (the naming in the render suite is a bit verbose 💁).
Launch Checklist