-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Remove cross-fading for patterns and dashes #12326
Conversation
This reverts commit 497d082.
c267dbf
to
38b4263
Compare
OK, after ironing out various quirks, this is mostly ready — the only remaining issue is 14 failing tests related to slightly different rendering of line dashes. Looks like this comes down to this code:
So SDF gamma is not interpolated like other properties, but rather uses the smaller of two dashes of a cross-fade. I'm not sure whether it's meant to be like this — the updated rendering looks slightly less blurry in places, but also has a few aliasing artifacts, but not a very noticeable difference overall. Should we update the expected images of these tests, or is there a way to make rendering more similar? @ansis |
Settled on a comrpomise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really glad to see that many simplifications and potential for performance improvements! We should run that against benchmap to get some numbers against this branch.
Benchmarks to review: They're mostly inconclusive or with a very tiny improvement, likely because our styles / locations in the benchmarks don't involve many dashed/patterned layers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
Closes #10605. Removes cross-fading for patterns and dashes, instead making them change instantly when zooming.
This PR, aside from removing a ton of internal complexity, makes it much easier for the Native team to port data-driven
line-dasharray
/line-cap
. It also makes data-driven pattern/dasharray more performant because it's twice less GPU memory cc @galinelle. Also -2kb gzipped bundle is nice.update: another big benefit noted by @karimnaaji:
The behavior change is subtle enough that it will be hard to notice in most cases:
line-dasharray
appears slightly differentlyLaunch Checklist
@mapbox/map-design-team
@mapbox/static-apis
if this PR includes style spec API or visual changes@mapbox/gl-native
if this PR includes shader changes or needs a native portmapbox-gl-js
changelog:<changelog>Improved performance of patterns and line dashes, also improving their appearance when zooming</changelog>