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

Animometer Replace Magic Number with device.limits.minUniformBufferOffsetAllignment #336

Closed

Conversation

cmhhelgeson
Copy link
Contributor

@cmhhelgeson cmhhelgeson commented Dec 7, 2023

Replaced use of 256 to calculate alignedUniformBytes with a constant holding the value of device.limits.minUniformBufferOffsetAlignment. Ideally this serves a few goals.

  1. Saving space in the hypothetical scenario where minUniformBufferOffsetAlignment becomes smaller.
  2. Adding a critical piece of context to the calculation of alignedUniformBytes that might not be immediately clear to a user that hasn't worked with buffer offsets in WebGPU before (i.e the size of the offsett is based around the device's limits, and is made to align within those limits)

I'm not an expert at using buffer offsets in WebGPU, so please inform me if my understanding of the use of this value is incorrect.

@greggman
Copy link
Collaborator

greggman commented Dec 7, 2023

minUniformBufferOffsetAllignment does not change on it's own. It only changes if you request it to be changed. Otherwise, it's effectively a constant and so no need to look it up. not saying you shouldn't look at it, only saying that's not bad to hard code them if they fit your needs.

@cmhhelgeson
Copy link
Contributor Author

Understood. I think for learning purposes it might be helpful to specify it explicitly, but I defer to you.

@cmhhelgeson cmhhelgeson closed this Dec 8, 2023
@cmhhelgeson cmhhelgeson deleted the animometer_buffer_offset_change branch April 12, 2024 16:40
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