Fix inspector showing Projection fields in wrong order

[MP430]

Hi,

When the inspector displays a projection, either as an @export from a script or a shader uniform, the columns and rows are the wrong way around/transposed.

I've fixed this by just swapping the row numbering in inspector_properties.cpp. The ordering is now consistent with i.e. Basis.

Here's the scene I used to test it also:

[gd_scene load_steps=5 format=3 uid="uid://dedlr02a8xi0v"]

[sub_resource type="BoxMesh" id="BoxMesh_4p3ci"]

[sub_resource type="Shader" id="Shader_31pbg"]
code = "shader_type spatial;

uniform mat4 testMatrix;

void vertex() {
	vec4 result = testMatrix * vec4(VERTEX.xyz,1);
	VERTEX = result.xyz / result.w;
}
"

[sub_resource type="ShaderMaterial" id="ShaderMaterial_vyl1g"]
render_priority = 0
shader = SubResource("Shader_31pbg")
shader_parameter/testMatrix = Projection(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1)

[sub_resource type="GDScript" id="GDScript_4p3ci"]
script/source = "extends MeshInstance3D

@export var inVec: Vector4
@export var inProj: Projection
@export var outProj: Projection

func _ready() -> void:
	outProj = inProj

func _process(delta: float) -> void:
	print(inProj * inVec)
"

[node name="Node3D" type="Node3D"]

[node name="MeshInstance3D" type="MeshInstance3D" parent="."]
mesh = SubResource("BoxMesh_4p3ci")
surface_material_override/0 = SubResource("ShaderMaterial_vyl1g")
script = SubResource("GDScript_4p3ci")
inVec = Vector4(1, 1, 1, 1)

I love Godot by the way!

[AThousandShips]

This change is not correct, this swaps the labels, which match those of the class, before they went xx, xy, ..., wz, ww, matching those of the class, now it goes xx, yx, ..., zw, ww which breaks the relationship

[AThousandShips]

See above, the ordering of the array on line 2703 needs to be adjusted to match

[MP430]

Hmm, I'm not sure about that.

With Basis, the xy label corresponds to Basis.y.x. Example:

This seems intended, because the documentation for Basis says that they're exposed in column-major order.

The documentation also says that Projections are column-major. But, this was backwards.

Before my change, I see this, which is unexpected because it's different to Basis:

After my change, I see this, which is consistent with Basis:

So I think this change is correct. Unless the labels for the Basis are also incorrect?

[AThousandShips]

Hmm, you're right, but that does mean that the way Basis is displayed matches the constructor, but not the basis access, with Projection it matches in both cases

I don't think Projection is wrong here, this change makes things more confusing, and it breaks the relationship with the code internally:

godot/core/math/projection.h

Lines 162 to 168 in c2202d3

	constexpr Projection(real_t p_xx, real_t p_xy, real_t p_xz, real_t p_xw, real_t p_yx, real_t p_yy, real_t p_yz, real_t p_yw, real_t p_zx, real_t p_zy, real_t p_zz, real_t p_zw, real_t p_wx, real_t p_wy, real_t p_wz, real_t p_ww) :
	columns{
	{ p_xx, p_xy, p_xz, p_xw },
	{ p_yx, p_yy, p_yz, p_yw },
	{ p_zx, p_zy, p_zz, p_zw },
	{ p_wx, p_wy, p_wz, p_ww },
	} {}

godot/core/math/basis.h

Lines 207 to 212 in c2202d3

	constexpr Basis(real_t p_xx, real_t p_xy, real_t p_xz, real_t p_yx, real_t p_yy, real_t p_yz, real_t p_zx, real_t p_zy, real_t p_zz) :
	rows{
	{ p_xx, p_xy, p_xz },
	{ p_yx, p_yy, p_yz },
	{ p_zx, p_zy, p_zz },
	} {}

Because of their design they are differently ordered, but the question is if Basis should be changed to match the x, y, and z properties, I'd say it should match what you can do with basis.x.x and projection.x.x

To be discussed further, non-trivial change and breaks some alignment between different parts of the code

[MP430]

I think projection is definitely wrong, unfortunately, I don't know about the labelling though, maybe that should be swapped everywhere.

Another example, using an identity matrix, but with a 1 in the top-right. I'd expect this to create a translation.
Before change, I get a projection instead:

After change, I get a translation as expected:

So at least, the positions are wrong. I don't know whether the labelling should be different though.

[Zireael07]

IIRC there was an issue/conversation somewhere about the Basis display @AThousandShips
(Also I strongly suspect projection simply followed the Basis)

[AThousandShips]

Accessing Projection and Basis in the same way is not matching the data though? basis[2][1] is the same as projection.columns[1][2] though, so swapping these makes the data not represent the data correctly

Try using the string representation, it should print:
[X: (0, 2, 0, 0), Y: (1, 0, 0, 0), Z: (0, 0, 0, 0), W: (0, 0, 0, 0)]
And
[X: (0, 2, 0), Y: (1, 0, 0), Z: (0, 0, 0)]
If your changes are correct

[MP430]

The main issue for me is that the layout of the matrix in the UI isn't mathematically correct.
If I put a 1 in the top right of a 4x4 matrix, it should create a translation. Getting a projection instead is very surprising!

[AThousandShips]

But that depends on how it's meant to be represented, I'd suggest checking the actual data to see that both actually print the same when processed directly

[MP430]

@AThousandShips
As suggested,
Before the change:

And after the change:

[AThousandShips]

That's good! Now the question is if the naming should be corrected as it's very confusing

[MP430]

I don't have a strong opinion on that, but I would be happy to swap the labels xy->yx etc. on all the types, if you think that's best?

[MP430]

Hi,

I've updated the PR with new labels for Transform2D, Transform3D, Basis, and Projection.
They're now all consistent and match how they're exposed in gdscript.
Here's an example showing how they all display now:

Hopefully that was the right thing to do, but let me know if not.

Here's the test scene too:

[gd_scene load_steps=2 format=3 uid="uid://dedlr02a8xi0v"]

[sub_resource type="GDScript" id="GDScript_4p3ci"]
script/source = "extends MeshInstance3D

@export var inTransform2D: Transform2D
@export var inTransform3D: Transform3D
@export var inBasis: Basis
@export var inProjection: Projection

func _ready() -> void:
	print(inTransform2D.x.y)
	print(inTransform3D.basis.x.y)
	print(inBasis.x.y)
	print(inProjection.x.y)
	
	print(\"\\n\\n\")
	
	print(inTransform2D)
	print(inTransform3D)
	print(inBasis)
	print(inProjection)
"

[node name="Node3D" type="Node3D"]

[node name="MeshInstance3D" type="MeshInstance3D" parent="."]
script = SubResource("GDScript_4p3ci")
inTransform2D = Transform2D(0, 2, 1, 0, 0, 0)
inTransform3D = Transform3D(0, 1, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0)
inBasis = Basis(0, 1, 0, 2, 0, 0, 0, 0, 0)
inProjection = Projection(0, 2, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)

Thanks!

[aaronfranke]

See #32068 (comment)

The labels in this PR match the intuition of game developers. I initially opened PR #32068 with this order. However, @tagcup insisted that labels must be in row/column order, so xy means row X column Y, even though the basis vectors are columns, therefore in GDScript code you'd write basis.y.x for the same value. To me, I like the change in this PR, which is the same as the original order in my old PR before @tagcup's feedback, but the argument for not changing the labels is also strong: it matches math textbooks.

We could, perhaps, disambiguate by adding a . in the middle of the letters, such that basis.x.y is displayed in the inspector as x.y instead of either xy (this PR) or yx (master).

@AThousandShips made a remark above about the dot notation being intuitive:

I'd say it should match what you can do with basis.x.x and projection.x.x

Also, I opened PR #110526 to fix the bug in Projection, which can be merged ahead of this PR.