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

Fix a potential incorrect vertex buffer access #10812

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

karimnaaji
Copy link
Contributor

A user reported a rendering artifact with a custom data set of fill-extrusions. The proposed PR prevents the centroid vertex buffer to be filled for some features but not others (when some features fall through the flat roof check but not others), eventually leading to mismatching sizes between the fill-extrusion vertex buffer and its associated centroid buffer. This prevents access to unassigned gpu data that leads to the rendering artifact this user is experiencing.

121939782-394e6600-cd02-11eb-86a2-e15048197791.mp4

The regression render test manifests the assertion on main the branch, and goes through with this fix:

>  mapbox-gl-js git:(main) ✗ yarn run test-render tests=regressions/mapbox-gl-js-team#217

$ SUITE_NAME=render CI=true testem ci -f test/integration/testem/testem.js 'tests=regressions/mapbox-gl-js-team#217'
    ---
        browser log: |
            testContext: [object Object]
            ERROR: Uncaught AssertionError: false == true at blob:http://localhost:7357/30198dab-7e9d-421c-ba40-745ff656fa55, line 952
    ...
not ok 2 Chrome 91.0 - [undefined ms] - render-tests/regressions/mapbox-gl-js-team#217 timed out after 5000ms
    ---

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
  • manually test the debug page
  • 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>Fix a potential rendering artifact when using custom fill-extrusion data set with terrain</changelog>

Copy link
Contributor

@rreusser rreusser left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm a little fuzzy on the precise details, but feels like the optimization could be kept in the case that it's all either one or the other, and that the issue is the mismatch arising from a little bit in both branches. Unless there's a clear performance win by making the fix fancier, this seems 💯 .

Copy link
Contributor

@arindam1993 arindam1993 left a comment

Choose a reason for hiding this comment

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

👍🏽

@karimnaaji karimnaaji merged commit 42cb3de into main Jul 1, 2021
@karimnaaji karimnaaji deleted the karim/fix-mismatching-centroid-buffers branch July 1, 2021 23:59
SnailBones pushed a commit to SnailBones/mapbox-gl-js that referenced this pull request Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants