Fix wrong rotation matrix for orbit z velocity

[QbieShay]

Closes #84033
Closes #84035

[YuriSizov]

I'm not an expert, but comparing to this image from Wikipedia, X and Y are also incorrect:

Is this true? Or is this because of the coordinate system that we use?

[QbieShay]

Maybe this difference is about clockwise and counterclockwise?

[QbieShay]

I just tested on top of this PR and it seems to all work fine, so i'm not sure if matrices should change ..?

[aaronfranke]

@QbieShay What I am certain of is:

• The old code is wrong.
• The change in this PR is also wrong (because it's not consistent with the other cases). PR updated.

Or is this because of the coordinate system that we use?

@YuriSizov Fortunately, all coordinate systems agree on rotation: X to Y around Z (not Y to X), Y to Z around X (not Z to Y), and Z to X around Y (not X to Z). This is a matter of correctness. The only tricky part is to remember if we are working with rows or columns. Usually code works with columns since it's more geometrically intuitive. I referenced this wiki page which indicates mat3 uses columns (see // first column (not row!) and mat3 n = mat3(column0, column1, column2); // sets columns of matrix n).

[QbieShay]

Thanks y'all. I think it's fixed now.

[aaronfranke]

See my comment above.

[clayjohn]

This looks mostly good to me. Testing it out it seems great. I just caught one additional bug. When using disble_z, we are reading from the r channel of the orbit_velocity_curve which means if you switch a particle system to disable z (that was orbiting in the z direction then you lose the orbit unless you use the x-channel of the texture, or you switch to a single channel texture.

I recognize there is some tension here in maintaining backwards compatibility as previously users could only use a regular CurveTexture and not a CurveXYZ texture. Further, CurveTexture allows you to only pass a red channel if you want. So we need to adjust the logic to maintain backwards compatibility and make it compatible with CurveXYZTextures

I would change the lines from:

if (tex_parameters[PARAM_ORBIT_VELOCITY].is_valid()) {
	code += "   orbit_amount *= texture(orbit_velocity_curve, vec2(lifetime)).r;\n";
}

To:

if (tex_parameters[PARAM_ORBIT_VELOCITY].is_valid()) {
	CurveTexture *texture = Object::cast_to<CurveTexture>(tex_parameters[PARAM_ORBIT_VELOCITY].ptr());
	if (texture) {
		code += "   orbit_amount *= texture(orbit_velocity_curve, vec2(lifetime)).r;\n";
	} else {
		code += "   orbit_amount *= texture(orbit_velocity_curve, vec2(lifetime)).b;\n";
	}
}

This maintains backwards compatibility and allows users to continue using a CurveXYZTexture

[QbieShay]

Thanks Clay! Updated and tested.

[clayjohn]

Oof, I just realized that checking the type of the texture won't update the MaterialKey as the MaterialKey only checks for the existence of the texture. So we need to add a new flag :(

Here is the diff:

diff --git a/scene/resources/particle_process_material.cpp b/scene/resources/particle_process_material.cpp
index b5049fab69..629b1bb1b6 100644
--- a/scene/resources/particle_process_material.cpp
+++ b/scene/resources/particle_process_material.cpp
@@ -31,7 +31,6 @@
 #include "particle_process_material.h"
 
 #include "core/version.h"
-#include "scene/resources/curve_texture.h"
 
 Mutex ParticleProcessMaterial::material_mutex;
 SelfList<ParticleProcessMaterial>::List *ParticleProcessMaterial::dirty_materials = nullptr;
diff --git a/scene/resources/particle_process_material.h b/scene/resources/particle_process_material.h
index 83228104b2..c7a38cef95 100644
--- a/scene/resources/particle_process_material.h
+++ b/scene/resources/particle_process_material.h
@@ -33,6 +33,7 @@
 
 #include "core/templates/rid.h"
 #include "core/templates/self_list.h"
+#include "scene/resources/curve_texture.h"
 #include "scene/resources/material.h"
 
 /*
@@ -125,6 +126,7 @@ private:
                uint64_t alpha_curve : 1;
                uint64_t emission_curve : 1;
                uint64_t has_initial_ramp : 1;
+               uint64_t orbit_uses_curve_xyz : 1;
 
                MaterialKey() {
                        memset(this, 0, sizeof(MaterialKey));
@@ -166,6 +168,9 @@ private:
                mk.emission_curve = emission_curve.is_valid() ? 1 : 0;
                mk.has_initial_ramp = color_initial_ramp.is_valid() ? 1 : 0;
 
+               CurveXYZTexture *texture = Object::cast_to<CurveXYZTexture>(tex_parameters[PARAM_ORBIT_VELOCITY].ptr());
+               mk.orbit_uses_curve_xyz = texture ? 1 : 0;
+
                for (int i = 0; i < PARAM_MAX; i++) {
                        if (tex_parameters[i].is_valid()) {
                                mk.texture_mask |= ((uint64_t)1 << i);

[QbieShay]

Updated!

[clayjohn]

Looks great! Tested locally and it works as far as I can tell.

[akien-mga]

Thanks!