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 ParticleMaterial scale on start() not correctly resetting #55397

Closed

Conversation

RPicster
Copy link
Contributor

This PR fixes #55396 by always resetting the scale when RESTART_ROT_SCALE is true (was already all there, just not used).

@Calinou Calinou added this to the 4.0 milestone Nov 27, 2021
clayjohn
clayjohn previously approved these changes Nov 29, 2021
@RPicster RPicster force-pushed the particlematerial-scale-reset branch from 1d35c9c to a40bcc3 Compare December 6, 2021 15:11
Comment on lines +384 to +386
code += " TRANSFORM[0].x = 1.0;\n";
code += " TRANSFORM[1].y = 1.0;\n";
code += " TRANSFORM[2].z = 1.0;\n";
Copy link
Member

Choose a reason for hiding this comment

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

This only resets the scale of the matrix (kind of, it will change the rotation, but won't necessarily reset the rotation). See: https://download.brainvoyager.com/bv/doc/UsersGuide/CoordsAndTransforms/SpatialTransformationMatrices.html

Copy link
Contributor Author

@RPicster RPicster Dec 6, 2021

Choose a reason for hiding this comment

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

@clayjohn : It wasn't my intention to reset the rotation - I only wanted to reset scale as this was the cause for the bug.
The RESTART_ROT_SCALE was just a variable I am piggybacking that was already there but not used (and worked).
I don't think it is necessary to reset rotation as it can not lead to problems (that I know of).

Copy link
Member

Choose a reason for hiding this comment

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

Well I don't think we should re-use RESTART_ROT_SCALE if it's only handling scale and rotation. That's a hint that we should look into what RESTART_ROT_SCALE is/does and see if that's actually semantically correct to use it here, or if a new/different variable should be added.

It seems like in master at least RESTART_ROT_SCALE is never used. So if it's only use is to reset the scale only, it could be renamed (and the places where it gets set to true or false should be reviewed to make sure they match what this needs).

$ rg restart_rotation_scale
servers/rendering/renderer_rd/renderer_storage_rd.cpp
10060:          actions.renames["RESTART_ROT_SCALE"] = "restart_rotation_scale";

servers/rendering/renderer_rd/shaders/particles.glsl
245:    bool restart_rotation_scale = false;
320:                                    restart_rotation_scale = true;
392:                    restart_rotation_scale = true;

Copy link
Member

Choose a reason for hiding this comment

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

Bump :)

Copy link
Member

Choose a reason for hiding this comment

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

Taking a look now, to fix this we can just fully reset scale and rotation. Scale and rotation can't really be separated once baked into the transform which is likely why they are bundled together in the first place. In other words, there are situations where it makes sense to reset position without scale and rotation, but there likely aren't any places where it makes sense to reset scale but not rotation (and it may not even be possible)

Suggested change
code += " TRANSFORM[0].x = 1.0;\n";
code += " TRANSFORM[1].y = 1.0;\n";
code += " TRANSFORM[2].z = 1.0;\n";
code += " TRANSFORM[0] = vec4(1.0, 0.0, 0.0, 0.0);\n";
code += " TRANSFORM[1] = vec4(0.0, 1.0, 0.0, 0.0);\n";
code += " TRANSFORM[2] = vec4(0.0, 0.0, 1.0, 0.0);\n";

@clayjohn
Copy link
Member

clayjohn commented Jun 8, 2023

Superseded by #78021. Thanks for your contribution!

@clayjohn clayjohn closed this Jun 8, 2023
@clayjohn clayjohn removed this from the 4.1 milestone Jun 8, 2023
@akien-mga akien-mga added this to the 4.1 milestone Jun 8, 2023
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.

ParticleMaterial particle size not resetting under certain conditions
5 participants