[3.x] Fixed rotate_y property of particle shaders

[QbieShay]

Rotate_y flag had an issue which made it almost unusable (see #33357).
I'd like some testing on this one, I haven't found any issue with it, but it'd help if someone else can take a look.

This breaks compat because in order to achieve a random, stable, y rotation, one had to pick super small values in the angle property.

Closes #33357

[akien-mga]

Your commit was wrongly rebased into another unrelated commit it seems.

[QbieShay]

@akien-mga darn you were faster to notice my git spaghetti than i was to clean it up. Sorry!

[akien-mga]

Indentation issue. You may want to set up clang-format / pre-commit hooks to avoid this in future commits :)
https://docs.godotengine.org/en/latest/community/contributing/code_style_guidelines.html

[akien-mga]

That bugfix should also be made in the master branch as it seems to have the same code here:

scene/resources/particles_material.cpp
603:            if (particle_flags[PARTICLE_FLAG_ROTATE_Y]) {

[akien-mga]

Looks good to me. Would like to have a quick review from rendering folks if possible, but I think it's fine to include in 3.2.4 even if it changes the behavior (any bugfix does if one relied on the buggy behavior).

[QbieShay]

@akien-mga I'm not in favor of merging for 3.2.4, this is a good fix, but it would break a bunch of things (even some of the particles that I made for the tps demo)

In order to obtain random y rotation before, one had to select rotate y and then set an angle of ~0.01 degrees. This would give you a nice, almost static, random rotation. But with this fix, all the particles will be aligned in the same direction, because a 0.01 angle is very small ..

[lawnjelly]

Codewise it looks fine, but the question seems whether the particles should behave in that way and I'm not at all familiar with particles so can't help on that currently (it's going to be a bit hectic till the RC is released). @clayjohn may be more familiar with particle behaviour?

And as @QbieShay says maybe this fix will break something else, so maybe we need to fix several things at once.

[QbieShay]

@lawnjelly the way that the parameter works right now is completely broken.

The rotation is stored in CUSTOM.x, so the rotation of the matrix should be set directly from the CUSTOM.x value. What happens instead is that the TRANSFORM is multiplied by the rotation matrix, which means that if in your custom.x you have accumulated 90degress, at frame1 you will have 90deg, frame2 you will have 180, frame3 270, frame4 360, even if, by the value stored in CUSTOM.x, you should be only rotated by 90deg. The addition of velocity is all handled in the way that rotation is added to the CUSOTM.x component, which should just be plainly applied as-is to the transform.

[lawnjelly]

Ah I see that makes lots of sense. So it is the right fix but we might have to fix some other bits of the engine that relied on the buggy previous behaviour? Or was it just the demos (and people's projects)? If it is just demos they should be ok to break hehe 😈 !

[akien-mga]

I'm not in favor of merging for 3.2.4, this is a good fix, but it would break a bunch of things (even some of the particles that I made for the tps demo)

But if we can't merge it in 3.2.4, we're unlikely to merge it in the 3.2 branch at all since it will be the same problem for future 3.2.x releases. Or we'd have to add another flag in the 3.2 branch to make the fix opt-in and preserve the previous, buggy behavior.

But yeah we could also merge it for 3.2.5 beta 1, see if anyone complains about their particles having changed behavior, and then assess if we want to add compatibility code.

[QbieShay]

What I meant is that we're already in RC stage, so I'd rather give people time to fix their particles with a beta, rather than dropiing the bomb like this :P again I don't think that many people use the property because it's very broken, but who knows :(

[lentsius-bark]

@QbieShay I can confirm that I've avoided the property due to its behaviour.

[akien-mga]

Thanks!