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 undefined behavior with atan in GPU Particles #36031

Merged

Conversation

@zxcvdev
Copy link
Contributor

zxcvdev commented Feb 8, 2020

Should fix #35492 , fix #30906 .

@akien-mga akien-mga changed the title Fix GPU Particles Fix undefined behavior with atan in GPU Particles Feb 8, 2020
@akien-mga akien-mga added this to the 3.2 milestone Feb 8, 2020
@akien-mga akien-mga removed the topic:shaders label Feb 8, 2020
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Feb 8, 2020

The bug originates in #26613, CC @KoBeWi.
Might be worth reviewing the CPU Particles{,2D} code too for potential issues (atan2 seems to handle x == 0 fine, as long as y != 0).

@clayjohn

This comment has been minimized.

Copy link
Contributor

clayjohn commented Feb 8, 2020

I wonder if something similar could be done to fix #30340 which also comes from #26613

@zxcvdev

This comment has been minimized.

Copy link
Contributor Author

zxcvdev commented Feb 8, 2020

@clayjohn I pushed a fix for #30340 in this branch which builds upon the code in this PR.

Setting the direction of particles should behave as expected with that fix, including when the direction is Vec3(x != 0, y != 0, 0).

Not sure if it would be considered breaking/changing existing projects though, since the behavior of particles when the direction is Vec3(x != 0, y != 0, 0) gets changed (to the correct behavior).

@clayjohn

This comment has been minimized.

Copy link
Contributor

clayjohn commented Feb 8, 2020

@zxcvdev Looks like a step in the direction. But not sure that it solves the issue in #30340. Copying your code into a test scene, I can still reproduce the issue.

The issue doesn't just occur when a given direction is 0. It occurs whenever Y is different from 0, regardless of the values of X or Z.

@zxcvdev

This comment has been minimized.

Copy link
Contributor Author

zxcvdev commented Feb 9, 2020

@clayjohn You're right, my bad. It only solves the issue where the X direction was ignored when Y != 0 and Z = 0. It does not solve the incorrect spread thing of #30340 .

@clayjohn

This comment has been minimized.

Copy link
Contributor

clayjohn commented Feb 9, 2020

That's fine. :) It should be included in this PR. We can look into the solution for #30340 later.

The special case atan(y,0) of the built-in shader function atan(y,x)
returns different results on different devices. So this commit will add
checks when the atan(y,x) function is used in ParticlesMaterial to set
the direction of GPU Particles to make sure the desired values are
returned (act as atan2(y,x)).
@zxcvdev zxcvdev force-pushed the zxcvdev:fix_gpu_particles_some_devices branch from 4cc2353 to 3580ad6 Feb 9, 2020
@zxcvdev

This comment has been minimized.

Copy link
Contributor Author

zxcvdev commented Feb 9, 2020

Amended to include additional minor fix as mentioned.

@akien-mga akien-mga merged commit 47f19cc into godotengine:master Feb 10, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Feb 10, 2020

Thanks!

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Feb 14, 2020

Cherry-picked for 3.2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.