Fix Transform::xform(Plane) functions, add Transform unit tests

[lawnjelly]

The Transform::xform and xform_inv are made safe for Planes when using non-uniform scaling.

Basic unit tests for Transform.

Optimization of calling sites to prevent loss of performance from the changes to xform(Plane).

Fixes #50548
Fixes #32764
Fixes scaled objects to work in the physics (static objects seem fine there may still be some physics bugs to sort for rigid bodies).
Replacement for #50549

Notes

• Based on feedback from @reduz this is a cut down version of Make Transform::xform functions safe with non-uniform scaling #50549
• I went through the current 3.x branch finding places where Plane xforms were used, these are now both safer and faster.
• Notably aside from the Plane xform functions, the other xform_inv functions in Transform remain unsafe to use except with uniform (1, 1, 1) scaling, as they use the transpose rather than affine_inverse.
• The unit tests have commented out section for non-uniform scale tests. These sections of the tests will fail for Vector3 and AABB because the current implementations aren't safe for use with scaling.
• I'm a newbie to doing unit tests, so these may not be perfect, and they may need something else doing for them to run as part of CI (they do seem to run if you put --test transform in the command line).

The results of the scaling checks all fail with xform_inv functions, even with a regular scale (e.g. 3, 3, 3), basically anything that deviates from 1, 1, 1 scale will return incorrect results:

Start Transform checks.
Fail with scale due to Transform::xform_inv(Vector3)
Fail with non-uniform scale due to Transform::xform(Vector3)
Fail with non-uniform scale due to Transform::xform_inv(Vector3)
Fail with non-uniform scale due to Transform::xform_inv(AABB) position
Fail with non-uniform scale due to Transform::xform_inv(AABB) size
	bb2 : 1, 2, 3 - 2, 6, 12
	bb3 : 1, 4, 9 - 2, 12, 36
Transform checks FAILED.

Discussion

This has proved to be a controversial area. There are two options, exemplified by these two PRs:

(Option A) Make all headline xform and xform_inv functions safe (#50549)

This fixes the bug and makes all the headline xform functions safe to use with scaling transforms outside of 1, 1, 1. Alternative versions provided for special cases.

Essentially, the idea is to e.g.:

• Make xform_inv perform the inverse.
• Make another function, e.g. xform_transpose perform the transpose.

At the moment we have the situation that xform_inv is actually the transpose.

(Option B) Make only the Plane xform and xform_inv functions safe (this PR)

This fixes the original bug (#50548) and makes minimal other changes.

Option B is currently preferred by @reduz. He points out that for types other than Plane - the transpose is the 'mathematically correct' way to do this, citing that in gdscript you use the multiplication operators, and that the onus should be on the user to understand the 3d math involved.

I remain unconvinced and am still personally in favour of option A, as I believe there are too many opportunities for user error with the current approach. There were a number of bugs as a result of Plane, and there may be other undiscovered bugs still occurring with other types, where people expect xform_inv to be an inverse and not a transpose.

Although reduz is familiar with the 3d math involved, he may be an outlier. I'm not sure it is reasonable to expect everyone, including users who may be quite young, and even all engine devs, to be so versed. I suspect that most people will expect that when you inverse a transform, you should get back to your starting point - but that is not currently the case. Note also that the function used by multiplication in gdscript need not necessarily be the same one as xform / xform_inv, even though it is currently.

Normal transforms

As an aside, I know Godot tries to keep the basic classes (Basis, Transform) as simple as possible, and pushes complexity to the calling code, but if we look at for example the code needed in physics, for simply transforming a normal:

Basis b_xform_normal = p_transform_b.basis.inverse().transposed();
Vector3 axis = b_xform_normal.xform(faces[i].plane.normal).normalized();

Although this is technically correct, to me as a reader it is not clear.

The longhand for proper transformation of normals is 'messy', there's no getting around it. So it would be nice to do another pass at these after the PR and see whether we can have some helper functions (perhaps in a separate class?) for transforming normals, to make them easier to read if nothing else.

[lawnjelly]

Reminder to myself for finishing this off after 3.4 beta:

reduz 6:59 PM:
lawnjelly I have the feeling that Transform::xform_fast should most likely go to Plane given its too plane specific
lawnjelly 7:00 PM:
Ok can do no problem.
reduz 7:00 PM:
other than that its ok

Slight snag here, I've just been experimenting with moving the xform_fast and xform_inv_fast functions to Plane. Because Plane is a dependency of Transform, we can't simply inline the Plane::xform_fast function, so it ends up in the cpp. I've just done some timings in release (just in case the link time optimization solved this) but the Plane version of the function does seem to be slower, by a factor of 3:2.

Normally this would not be a concern, but given that this function is likely to be a bottleneck in the physics (hence why it is optimized), it might be worth coming up with an alternative place for the xform_fast where it can be inlined.

[Flarkk]

@lawnjelly this PR also fixes culling issues discussed in #32764

Reason : the changes include the removal of Vector3 point_dir = point + p_plane.normal; which causes floating point precision error when point is very far away (p_plane.normal having unit length)

[lawnjelly]

@lawnjelly this PR also fixes culling issues discussed in #32764

Reason : the changes include the removal of Vector3 point_dir = point + p_plane.normal; which causes floating point precision error when point is very far away (p_plane.normal having unit length)

Ah great, I did have a sneaking suspicion that it might! 👍 😁

I'll try and get this finished when I get a spare moment as I'm sure it solves a few bugs. I could get it finished easily by putting the function into cpp but it causes a slowdown, so I need to re-evaluate / possibly discuss with juan again where he is happy to put the xform_fast function.

[Flarkk]

Maybe it’s worth adding specific unit tests for large scaled input values, to prevent eventual regressions on floating point precision topics in the future ?

[lawnjelly]

Maybe it’s worth adding specific unit tests for large scaled input values, to prevent eventual regressions on floating point precision topics in the future ?

Possibly. We can add further unit tests once this is merged. Various tests have to be looked at on their own merit, this PR is primarily to fix a particular problem, with unit tests to address that problem - if it gets outside this scope it becomes more difficult to review.

[lawnjelly]

After trying various permutations I've added a second commit for a variation that achieves what I think reduz was after, to keep the Transform class as simple as possible. I tried moving the xform_fast into Plane, but because of the cyclical includes I couldn't work out an easy way without putting this function into the cpp, which defeated the object because it was slower due to presumably not being inlined.

So the second commit moves the fast functions to a third helper class (where we can perhaps put other such functions in the future). This means they can be inlined so will run at full speed. The only downside is these are not exposed to the binding for gdscript / gdnative etc, but realistically they are more likely to be used from the engine code in bottleneck areas.

If this second commit is preferred, I can squash the commits, but I'll leave them separate until reviewed.

[lawnjelly]

Ok changes made. I'm happy with this now. We looked at moving the fast functions to Plane but because of the inlining problem have agreed to go back to having them in the Transform.

I'll leave this to get the once over by @pouleyKetchoupp for the changes to physics. The physics changes are all bug fixes to allow non-uniform scales, or changes to give identical transforms by using Basis xform rather than Plane xform (where only the normal was required).

I'll have a look at doing the version for 4.0 tomorrow. 👍

[pouleyKetchoupp]

Changes in physics look fine, I haven't re-tested but I assume you did and it still works :)

I agree with your note about having a helper function for the inverse.transpose bit, it makes the code hard to understand. Maybe it could be added as part of this PR, with something like Basis::normal_xform() the same way we have Basis::transpose_xform(), with a comment that explains it helps with non-uniform scaling of normals?

[lawnjelly]

Changes in physics look fine, I haven't re-tested but I assume you did and it still works :)

I agree with your note about having a helper function for the inverse.transpose bit, it makes the code hard to understand. Maybe it could be added as part of this PR, with something like Basis::normal_xform() the same way we have Basis::transpose_xform(), with a comment that explains it helps with non-uniform scaling of normals?

Yeah I'm all for making things as clear as possible in cases like this. I will do this today, but I think a separate PR would be better as this really needs merging (once 3.4 beta 3 build is sorted) to get the bugs fixed as it has spent long enough in limbo already. 👍

[lawnjelly]

OK, all changes done, should be good now if it passes CI. I'll do the other PR for the basis function, and the version for 4.0. 👍

[akien-mga]

Thanks!