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

Remove cross-fading for patterns and dashes #12326

Merged
merged 23 commits into from
Nov 7, 2022
Merged

Remove cross-fading for patterns and dashes #12326

merged 23 commits into from
Nov 7, 2022

Conversation

mourner
Copy link
Member

@mourner mourner commented Oct 21, 2022

Closes #10605. Removes cross-fading for patterns and dashes, instead making them change instantly when zooming.

This behavior adds a lot of internal complexity. It makes data-driven pattern and dash implementations very hard to reason about. It is hard to test with render tests. I think it also doesn't look great. Instant transitions seem less noticeable for both dasharrays and patterns.

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:

Removing this might have quite a beneficial impact on terrain render cache, as this would reduce the need to re-render an entire tile to the offscreen FBO and instead keep it cached for more frames. [...] as long as you used at least one draped layer with patterns this could start showing fairly noticeable performance difference

The behavior change is subtle enough that it will be hard to notice in most cases:

before after
ezgif com-gif-maker ezgif com-gif-maker (1)
  • Figure out why line-dasharray appears slightly differently
  • Fix a few render tests
  • Make sure there is no leftover cross-fading logic anywhere to remove

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • 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>Improved performance of patterns and line dashes, also improving their appearance when zooming</changelog>

@mourner mourner added performance ⚡ Speed, stability, CPU usage, memory usage, or power usage refactoring 🏗️ labels Oct 21, 2022
@mourner mourner self-assigned this Oct 21, 2022
@mourner mourner requested a review from ansis October 21, 2022 15:02
@mourner
Copy link
Member Author

mourner commented Oct 21, 2022

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:

float sdfwidth = min(dash_from.z * u_scale.y, dash_to.z * u_scale.z);

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

@mourner
Copy link
Member Author

mourner commented Oct 24, 2022

Settled on a comrpomise sdfgamma value that's closest to what we had before, bringing down the number of failing render tests from 14 to 3, which I'm updating the last commit.

@mourner mourner marked this pull request as ready for review October 24, 2022 14:29
@mourner mourner requested a review from a team as a code owner October 24, 2022 14:29
Copy link
Contributor

@karimnaaji karimnaaji left a 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.

src/data/program_configuration.js Outdated Show resolved Hide resolved
src/style-spec/reference/v8.json Show resolved Hide resolved
src/render/painter.js Show resolved Hide resolved
src/style-spec/reference/v8.json Show resolved Hide resolved
@mourner mourner requested a review from ansis November 4, 2022 14:42
@mourner
Copy link
Member Author

mourner commented Nov 4, 2022

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.

Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage refactoring 🏗️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove cross-fading for patterns and dasharrays
5 participants