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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

Fix ParticleMaterial scale on start() not correctly resetting #55397

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions scene/resources/particles_material.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,14 +378,20 @@ void ParticlesMaterial::_update_shader() {
code += " CUSTOM.x = base_angle * degree_to_rad;\n"; // angle
code += " CUSTOM.y = 0.0;\n"; // phase
code += " CUSTOM.w = (1.0 - lifetime_randomness * rand_from_seed(alt_seed));\n";
code += " CUSTOM.z = (tex_anim_offset) * mix(anim_offset_min, anim_offset_max, anim_offset_rand);\n"; // animation offset (0-1)
code += " CUSTOM.z = (tex_anim_offset) * mix(anim_offset_min, anim_offset_max, anim_offset_rand);\n\n"; // animation offset (0-1)

code += " if (RESTART_ROT_SCALE) {\n";
code += " TRANSFORM[0].x = 1.0;\n";
code += " TRANSFORM[1].y = 1.0;\n";
code += " TRANSFORM[2].z = 1.0;\n";
Comment on lines +384 to +386
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 •
edited
Loading

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";

code += " }\n\n";

code += " if (RESTART_POSITION) {\n";

switch (emission_shape) {
case EMISSION_SHAPE_POINT: {
//do none, identity (will later be multiplied by emission transform)
code += " TRANSFORM = mat4(vec4(1,0,0,0),vec4(0,1,0,0),vec4(0,0,1,0),vec4(0,0,0,1));\n";
code += " TRANSFORM[3].xyz = vec3(0.0, 0.0, 0.0);\n";
} break;
case EMISSION_SHAPE_SPHERE: {
code += " float s = rand_from_seed(alt_seed) * 2.0 - 1.0;\n";
Expand Down