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

Tweak Particle Sample #492

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

greggman
Copy link
Collaborator

I was investigating why this sample didn't work on compat mode in Chrome.

I found 1 issue in the code which is the shader that makes the probability map only worked if the source image is was a multiple of 64 pixels wide. This was because it was not conditionally skipping out of bounds pixels and so those pixels would end up overwriting other results.

This issue does not come up in the sample but if you change the image to some image that is not a multiple of 64 pixels across the issue comes up.

In debugging why GL was failing one my paths was to make a 16x16 pixel F using the canvas 2D api and passing that in so that I can print out all the data. With that I ran into this issue.

So, I added the skipping. Thank's to Peter McNeeley for finding that issue.

While pursing the code I find somethings I thought maybe should be refactored?

  • Calculating the number of mip level was done in a loop.

    Changed it to just math.

  • Moving some calculations to use properties on the texture.

    I can revert these changes. They were useful when I switched the source image since then the code didn't depend on variables that were no longer available.

  • Getting rid of struct Buffer

    I can revert these changes. I get the impression that this was a limitation of an earlier version of WGSL. It seems more confusing to me personally have have a struct with a single array member in it than to just have the array.

  • Simpler math

    I can revert this but, changed sum = dot(vec4f(a, b, c, d), vec4f(1.0)) to sum = a + b + c + d.

Note that, even with the change for the first pass, the code only works for square power-of-2 images. Both the probability generation and the usage of that data require each mip level to be exactly 1/2 the size of the previous level so added some asserts.

I was investigating why this sample didn't work on compat mode in Chrome.

I found 1 issue in the code which is the shader that makes the probability map
only worked if the source image is was a multiple of 64 pixels wide.
This was because it was not conditionally skipping out of bounds pixels
and so processing those pixels would end up overwriting other results.

This issue does not come up in the sample but if you change the image to
some image that is not a multiple of 64 pixels across the issue comes up.

In debugging why GL was failing, one my paths was to make a 16x16 pixel
`F` using the canvas 2D api and passing that in so that I could print
all of the data. With that I ran into this issue.

So, I added the skipping. Thank's to Peter McNeeley for finding that issue.

While pursing the code I find somethings I though maybe should be refactored?

* Calculating the number of mip level was done in a loop.

  Changed it to just math.

* Moving some calculations to use properties on the texture.

  I can revert these changes. They were useful when I switched the
  source image to a canvas since then the code didn't depend on variables that
  were no longer available.

* Getting rid of `struct Buffer`

  I can revert these changes. I get the impression that this was
  a limitation of an earlier version of WGSL? It seems more confusing
  to me personally to have a struct with a single array in it than
  to just have the array.

* Simpler math

  I can revert this but, changed `sum = dot(vec4f(a, b, c, d), vec4f(1.0))`
  to `sum = a + b + c + d`.

Note that, even with the change for the first pass, the code only works
for square power-of-2 images. Both the probability generation and the
usage of that data require each mip level to be exactly 1/2 the size
of the previous level. So, added some asserts so people copying this
won't stumble into issues.
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.

1 participant