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 particle spread values affecting particle velocity #85880

Merged

Conversation

Rudolph-B
Copy link
Contributor

@Rudolph-B Rudolph-B commented Dec 7, 2023

Resolves #85744 and partially #85620

The spread direction vector for a particle had affected the particles velocity. This was caused by spread_direction not being normalized before it was returned from get_random_direction_from_spread(). A compounding issue to this was that get_random_direction_from_spread() still generated a 3D spread_direction even when PARTICLE_FLAG_DISABLE_Z is set.

I made changes to get_random_direction_from_spread() so that it:

  • Uses a dedicated 2D spread calculation (used in the 4.1 release)
  • Normalize the 3D spread_direction before returning it.

@Rudolph-B Rudolph-B changed the title #85744 Fixed particle spread values effecting particle velocity Fixed particle spread values effecting particle velocity Dec 7, 2023
@akien-mga akien-mga added this to the 4.3 milestone Dec 7, 2023
@akien-mga akien-mga added cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:2d labels Dec 7, 2023
@clayjohn
Copy link
Member

clayjohn commented Dec 7, 2023

Other than my comment, everything else looks good. The rest matches what we had in 4.1. So this PR should be good to go as long as we aren't introducing a behaviour change to 3D now

@Rudolph-B Rudolph-B force-pushed the GPU-particles-2D-spread-issue branch from c5ccf66 to 6b2a1b4 Compare December 8, 2023 04:09
@AThousandShips AThousandShips changed the title Fixed particle spread values effecting particle velocity Fixed particle spread values affecting particle velocity Dec 8, 2023
@QbieShay
Copy link
Contributor

Would superseed #86233

@YuriSizov
Copy link
Contributor

@QbieShay Please help me out here. #84977 is superseded by #86233, and #86233 is superseded by this PR?

Should we close the other two?

@QbieShay
Copy link
Contributor

Yep sorry. I have seen this PR only after opening my PR that supersedes the other one. I havent tested this one yet

@Rudolph-B
Copy link
Contributor Author

Rudolph-B commented Jan 9, 2024

@QbieShay Is there any tests in particular that I can run that can help?

@akien-mga akien-mga requested a review from paddy-exe February 13, 2024 14:17
@georgwacker
Copy link
Contributor

I tested this PR with my particle system from 4.1.2 and #85620 looks to be fixed.

Did you have a 2D test case where there are still issues compared to 4.1.x?

@akien-mga akien-mga requested a review from clayjohn March 1, 2024 13:58
@Rudolph-B
Copy link
Contributor Author

This is the original MRP for #85744
GPUParticlesBug.zip

@Rudolph-B Rudolph-B force-pushed the GPU-particles-2D-spread-issue branch from dcaf6d3 to 6b2a1b4 Compare April 7, 2024 14:55
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Let's merge this for 4.3!

We should cherrypick this for 4.2 as well since the original bug was a regression in 4.2

@akien-mga akien-mga changed the title Fixed particle spread values affecting particle velocity Fix particle spread values affecting particle velocity Apr 24, 2024
@akien-mga akien-mga merged commit 6f21267 into godotengine:master Apr 24, 2024
31 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Rudolph-B Rudolph-B deleted the GPU-particles-2D-spread-issue branch April 27, 2024 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release regression topic:particles topic:2d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GPU particles 2D spread issue
7 participants