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

Switch back to a more compact line attributes layout #8306

Merged
merged 1 commit into from
Jun 10, 2019

Conversation

mourner
Copy link
Member

@mourner mourner commented May 30, 2019

Switch back from 12 to 8 bytes per vertex for line layers. See #8304.

I tried to decode them in a potentially safer way this time (avoiding mod and / in shader math) to see if it fixes the Broadcom GPU bug — we need to verify this before merging we did verify it works properly on Broadcom VideoCore IV GPU, and faster than master 🎉. If not, this PR will be a good starting point for exploring other packing strategies (e.g. packing in most significant bit).

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality covered by render tests
  • document any changes to public APIs no changes
  • post benchmark scores ran Layout/Paint/LayerLine, they don't seem to be affected since this is a GPU thing
  • manually test the debug page
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • test on the affected Broadcom GPU device
  • test GL Native counterpart on a Broadcom GPU device [core] Switch back to a more compact line attributes layout mapbox-gl-native#14851

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

The testing so far looks promising but I think we should try to test this in -native with the buggy device before merging.

Do you think using the most significant bit instead of the least significant bit could make it even more robust?

@ansis
Copy link
Contributor

ansis commented Jun 5, 2019

Would a new buffer size benchmark be useful to track these kinds of changes?

@mourner
Copy link
Member Author

mourner commented Jun 5, 2019

The testing so far looks promising but I think we should try to test this in -native with the buggy device before merging.

Yep, already submitted a PR mapbox/mapbox-gl-native#14851 — will wait for confirmation of that one.

Do you think using the most significant bit instead of the least significant bit could make it even more robust?

I don't know why it wasn't robust in the first place so not sure whether it will make a difference, but if the current approach is confirmed to work with those GPUs, we shouldn't have any problem on newer ones.

Would a new buffer size benchmark be useful to track these kinds of changes?

Yes, I have it locally and want to PR, but it doesn't fit the benchmark format because it's about qualitative values, not timing. Might be worth pushing just as a debug page at first.

@mourner mourner requested a review from ansis June 8, 2019 12:16
@mourner mourner merged commit b0a37a0 into master Jun 10, 2019
@mourner mourner deleted the pack-normals-again branch June 10, 2019 17:40
@ansis ansis mentioned this pull request Nov 14, 2019
15 tasks
candux added a commit to candux/maplibre-gl-js that referenced this pull request May 23, 2022
candux added a commit to geops/maplibre-gl-js that referenced this pull request Jun 3, 2022
birkskyum pushed a commit to birkskyum/maplibre-gl-js that referenced this pull request Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants