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

Data-driven line-dasharray and line-cap #10591

Merged
merged 29 commits into from
May 6, 2021
Merged

Data-driven line-dasharray and line-cap #10591

merged 29 commits into from
May 6, 2021

Conversation

mourner
Copy link
Member

@mourner mourner commented Apr 17, 2021

Closes #3045. Implements data-driven expressions support for line-dasharray and line-cap. Example:

"paint": {
  "line-dasharray": [
    "match", ["get", "property"],
    1, ["literal", [1, 2]],
    2, ["literal", [2, 2]],
    3, ["literal", [3, 2]],
    ["literal", [1, 1]]
  ]
},
"layout": {
  "line-cap": ["match", ["get", "property"], 2, "round", "butt"]
}

Similar to imageAtlas and glyphAtlas, lineAtlas is now generated per tile rather than globally, allowing for more dash values to fit the atlas, and moving dash SDF generation to the worker.

Implementation status

  • Fix a bug where a zoom dasharray expression causes flickering when crossing the zoom stops zooming in.
  • Fix feature line-cap with constant dasharray
  • Fix feature line-cap with zoom dasharray
  • Fix composite line-cap with constant/zoom dasharray
  • Trim line atlas to only the space used to avoid uploading 256x256px texture for every tile.
status constant dash zoom dash feature dash composite dash
constant cap 🟢 🟢 🟢 🟢
zoom cap 🟢 🟢 🟢 🟢
feature cap 🟢 🟢 🟢 🟢
composite cap 🟢 🟢 🟢 🟢

Something to follow up but not necessarily in this PR: we could switch to a statically packed sprite layout with potpack, similar to glyph and image atlases. This should drastically reduce the line atlas sizes while also improving rendering for more complex dashes, since the current dash SDF is fixed 256px x 1px or 15px (round) regardless of its configuration or expected line width range.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Add support for data-driven expressions in line-dasharray and line-cap properties.</changelog>

mourner and others added 11 commits April 7, 2021 17:13
The attribute names used for dashes were the same as those used for
patterns. This was leading to incorrectly generated shaders.

Since there was no pattern set for the dash layer it was setting the
defines that generate the shader without those attributes.

The line atlas texture is now bound with gl.REPEAT to render more than
one dash.
@ryanhamley ryanhamley added this to the v2.3 milestone Apr 21, 2021
@mourner mourner marked this pull request as ready for review April 23, 2021 16:01
@ansis
Copy link
Contributor

ansis commented Apr 29, 2021

This looks good to me. Can we add render tests to cover the all the possible variations?

@ansis
Copy link
Contributor

ansis commented Apr 29, 2021

Benchmark results

Screen Shot 2021-04-29 at 11 57 52 AM

After these changes load times are a bit more than 1% slower. It probably makes sense to merge these results anyways. @mourner can you comment to document why this is probably not something we can optimize away right now?

There is a slight bias in the results that we still need to fix but that only accounts for about 5ms of the difference.

@mourner
Copy link
Member Author

mourner commented Apr 29, 2021

@mourner can you comment to document why this is probably not something we can optimize away right now?

The minor slowdown comes mostly from 2 things:

  • Setting up and uploading a line atlas for every tile instead of a global one.
  • Properties that may potentially be data-driven get treated differently in our program configuration code, adding overhead even if data-driven values aren't used.

The first item could potentially be addressed by keeping a separate global line atlas for non-data-driven dash layers, but this would add considerable complexity and isn't worth doing at the moment, at least until we reduced the overall complexity of dashed lines implementation by removing cross-fading #10605.

It's not clear how to address the second one without a major refactoring of how data-driven styling works, so we'll leave that for future optimizations.

@mourner mourner requested a review from ansis April 30, 2021 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement property functions for "line-dasharray" and "line-cap"
4 participants