Transform.xform_inv() is not the inverse of Transform.xform()

[goatchurchprime]

Godot version:
3.2.1.stable

OS/device including version:
Windows10

Issue description:
The documentation claims that: xform "Transforms the given Vector3 by this transform" and xform_inv "Inverse-transforms the given Vector3 by this transform." but it is not the case. The scale gets applied a second time instead of being undone.

https://docs.godotengine.org/en/stable/classes/class_transform.html?highlight=transform#class-transform-method-xform

Steps to reproduce:

var t = Transform(Vector3(2, 0, 0), 
                  Vector3(0, 1, 0), 
                  Vector3(0, 0, 1), 
                  Vector3(0, 0, 0))
print(t.xform_inv(t.xform(Vector3(1,0,0))))
# --->  Result is (4, 0, 0) !!!

Minimal reproduction project:
See above. (This has been simplified down from a more arbitrary transform I hit this problem with.)

[Calinou]

Related to #26432? (Just a wild guess.)

[goatchurchprime]

I have just been surprised to discover that there are two inverse functions: Transform.inverse() and Transform.affine_inverse().

There is a warning in the description for Transform.inverse() that it is not really an inverse.

There needs to be a similar health warning for the Transform.xform_inv() function, and possibly a new Transform.xform_aff_inv() function.

Anyway, I don't see the point to the Transform.inverse() function, other than it might be a faster version of Transform.affine_inverse() that assumes the scale is 1.0. The answer doesn't mean anything when the scale is not 1.0 (why would you need the scale squared?), and it should probably give a debug warning when used this way.

(The other issue is about the orientation and handedness of the bases, not the scale.)

[Jummit]

What xform_inv does:
a * scale + origin
What it should do:
a / scale + origin

[kleonc]

@Jummit, you omitted the rotation part and the origin should be subtracted, not added.

Current behaviour (for both 2D and 3D)

For a vector v and a transform t representing transformation defined by matrix M = T R S where T is translation matrix, R is rotation matrix, S is scaling matrix:

t.affine_inverse() corresponds to M^-1 = S^-1 R^-1 T^-1
t.inverse() corresponds to S R^-1 T^-1

t * t.affine_inverse() corresponds to M M^-1 = (T R S)(S^-1 R^-1 T^-1) = I (identity matrix)
t.affine_inverse() * t corresponds to M^-1 M = (S^-1 R^-1 T^-1)(T R S) = I
t * t.inverse() corresponds to (T R S)(S R^-1 T^-1) = T R S² R^-1 T^-1
t.inverse() * t corresponds to (S R^-1 T^-1)(T R S) = S²

t.xform(v) corresponds to M v = T R S v
t.xform_inv(v) corresponds to S R^-1 T^-1 v

t.xform(t.xform_inverse(v)) corresponds to (T R S)(S R^-1 T^-1 v) = T R S² R^-1 T^-1 v
t.xform_inv(t.x_form(v)) corresponds to (S R^-1 T^-1)(T R S v) = S² v

So current behaviour of these methods is consistent. However naming isn't good, since it's perfectly reasonable to assume that (for example) result of t * t.inverse() should be an identity transform. There's also no xform... counter-part for affine_inverse method.

Possible changes

(for both Transform and Transform2D)

• For 3.x version (as @goatchurchprime suggested):
  • enhancing documentation for xform_inv method (I'd say it's a must),
  • additionally method named something like xform_aff_inv could be added such that t.xform_aff_inv(v) would correspond to M^-1 v = S^-1 R^-1 T^-1 v.
• For 4.0 version:
  • renaming (or removing) inverse and xform_inv methods since what these methods actually do is not what people expect them to do, they're the main reason of all this confusion (I couldn't come up with proper new names for these methods),
  • either renaming affine_inverse to inverse or not changing it at all,
  • adding xform... method corresponding to current affine_inverse method (with relevant name based on the point above).

What do you think about such changes?

[Kobrar]

This issue goes a little deeper than this. The real culprit here is the Basis class.
As it is documented, xform_inv in Basis only does the inverse transformation if the basis is a rotation-reflection. Implementation just uses its transposed version. To me this seems a serious error. A 3x3 matrix can express a very wide range of transforms and imo it is not enough to clarify in the docs that a method doesn't do what it is supposed to.
The workaround is to use Basis.inverse, but generally speaking it is not advisable to calculate an inverse of a matrix in vain.
The implementation of Transform.xform_inv looks like this:

_FORCE_INLINE_ Vector3 Transform3D::xform_inv(const Vector3 &p_vector) const {
	Vector3 v = p_vector - origin;

	return Vector3(
			(basis.rows[0][0] * v.x) + (basis.rows[1][0] * v.y) + (basis.rows[2][0] * v.z),
			(basis.rows[0][1] * v.x) + (basis.rows[1][1] * v.y) + (basis.rows[2][1] * v.z),
			(basis.rows[0][2] * v.x) + (basis.rows[1][2] * v.y) + (basis.rows[2][2] * v.z));
}

it also uses a simple transpose and even fails to utilize Basis.xform_inv.
So a full fix for this would be:

• rename current Basis.xform_inv to Basis.xform_trans for those cases when you know you can use the optimization
• implement Basis.xform_inv to perform a proper inverse transformation regardless of what the basis represents (Gaussian elimination?)
• utilize Basis.xform_inv in Transform.xform_inv

[Mickeon]

Is this still the case in the latest documentation (4.2, 4.3)? It's a bit difficult to discern for me, given the removal of xform in favour of using the multiplication operator.

[kleonc]

Is this still the case in the latest documentation (4.2, 4.3)? It's a bit difficult to discern for me, given the removal of xform in favour of using the multiplication operator.

AFAICT there's currently no issue with the documentation, it should be resolved in 3.x by #49662 and in 4.x by #83461.

---

Regarding #39433 (comment):

So a full fix for this would be:

• rename current Basis.xform_inv to Basis.xform_trans for those cases when you know you can use the optimization
• implement Basis.xform_inv to perform a proper inverse transformation regardless of what the basis represents (Gaussian elimination?)
• utilize Basis.xform_inv in Transform.xform_inv

Such changes are compatibility breaking, they could have been considered for 4.0 back then indeed, but no such changes have been done. Hence I'd say in 4.x we're settled with the current behavior, which is valid and documented (even if methods' names are questionable etc.).

So to me it seems that this issue can be closed, and if changes like suggested above are still desired then I think there should be a proposal opened for that in https://github.com/godotengine/godot-proposals (for consideration for 5.0 because of compat breaking nature).