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 radial inwards velocity clamping incorrectly (regression from #83488) #85252

Merged

Conversation

QbieShay
Copy link
Contributor

@QbieShay QbieShay commented Nov 22, 2023

When i changed the particle code to avoid division by zero, i didn't notice i left out a part in rdial velocity, which now causes radial velocity around the center to just be a fixed speed. This correctly fixes it.

Bugsquad edit:

@akien-mga akien-mga changed the title Fixed radial inwards velocity claming incorrectly (regression from 83488) Fix radial inwards velocity claming incorrectly (regression from #83488) Nov 22, 2023
@akien-mga akien-mga changed the title Fix radial inwards velocity claming incorrectly (regression from #83488) Fix radial inwards velocity clamping incorrectly (regression from #83488) Nov 22, 2023
@akien-mga akien-mga added the bug label Nov 22, 2023
@akien-mga akien-mga added this to the 4.2 milestone Nov 22, 2023
@QbieShay QbieShay force-pushed the qbe/fix-radial-inwards-velocity branch from 4705ddb to a879e59 Compare November 23, 2023 09:33
@QbieShay
Copy link
Contributor Author

Alright so!

This fix is the correct fix, because the variables that are named "displacement" are actually velocities. They used to be raw displacement, but in order to fix some division by zero i did a rework of the functions to instead calculate velocities, but unfortunately i didn't rename the parameters/functions to correctly represent it.

Doing a bunch of renames at this stage of RC is not ideal, so what I suggest it's the following:

  1. merge this fix (otherwise inwards radial velocity is unusable)
  2. for 4.3, fix the naming ( I opened and self assigned an issue for it Particle Shader calls velocities displacement #85310 )

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.

Thank you for the explanation. Indeed this is very confusing because the naming consistently refers to a displacement, but it is calculating a velocity.

On review I can see that all the other components that contribute to radial_displacement (and controlled_displacement) are velocities. So it makes sense to convert this value into a velocity as well.

@akien-mga akien-mga merged commit eda44bf into godotengine:master Nov 27, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants