Prevent setting a bad Node3D scale or transform

[JohanAR]

Setting a scale to 0 or a transform with 0-determinant in Node3D will later lead to a division by 0 when the basis is inverted, or in best case error message spam if MATH_CHECKS is defined.

Fixes #32405

[Zylann]

Just wanted to indicate that with a change like this, making a 3D node to "despawn" with a shrinking animation is no longer possible without thinking about the case of zero.
It also adds a bunch of multiplications in places that often run every frame, due to the checking the determinant (should it be the determinant? Why not check the diagonal for zero?), but I don't know if ERR_FAIL_COND checks are turned off in release builds?

[JohanAR]

Just wanted to indicate that with a change like this, making a 3D node to "despawn" with a shrinking animation is no longer possible without thinking about the case of zero.

Was this possible? I'm on master and it looks like it would segfault if you set any scale dimension to 0 (unless you compiled with extra runtime math checks). I think with this change it should stay at the last valid value, so it might be near-infinitely small depending on your animation step

It also adds a bunch of multiplications in places that often run every frame, due to the checking the determinant (should it be the determinant? Why not check the diagonal for zero?), but I don't know if ERR_FAIL_COND checks are turned off in release builds?

I was a bit worried about performance.. I didn't see that many calls to set_transform, but I only have a simple test case with a bunch of non-moving meshes and a flying camera. Perhaps it's better to put a check in the editor GUI instead? I suppose one could see entering a transform manually as an advanced operation, and put less protections on it, but IMHO the editor should avoid crashing at all costs.

I have to admit I don't remember much from matrix algebra, but based on when the program crashes I think the limitation is that no transform.basis row or column may be all zeroes. A zero diagonal seems to work fine as long as the other elements are filled in with something. Perhaps it would be faster to check for this condition using only boolean operators instead of multiplication..

I think all the ERR checks are enabled from all builds, I can't see any mechanism that disables them in the code

[Zylann]

It probably wasnt possible already, just saying I've seen shrinking animations being a thing in games.

I didn't see that many calls to set_transform

It's not about the amount of times set_transform() is written in the codebase, but more how often it gets called. I can imagine it being called often when animating, or when the physics simulation updates the node.
It shouldn't be much of a problem (and I'll eventually see comparisons when I get to run my benchmark project in the next versions), the editor should avoid crashing. I'm mostly wondering about release builds.

A zero diagonal seems to work fine as long as the other elements are filled in with something

That's weird because to my understanding, a zero diagonal essentially means scale IS zero^^"
Nvm, I was mistaken.

[JohanAR]

It probably wasnt possible already, just saying I've seen shrinking animations being a thing in games.

An alternative could be to hide the object if scale is set to 0, but I think that might add a bit of complexity.

It's not about the amount of times set_transform() is written in the codebase, but more how often it gets called. I can imagine it being called often when animating, or when the physics simulation updates the node.
It shouldn't be much of a problem (and I'll eventually see comparisons when I get to run my benchmark project in the next versions), the editor should avoid crashing. I'm mostly wondering about release builds.

Yea, I meant I put a debug print every time it was called to get a feeling for how often it happens. But now I tried adding some physics objects and it's called much more often as you said. Perhaps it's better to move this check to the GUI, and let people crash if they set bad values through script :) With a large number of moving objects the overhead might be too much, even if it's only done when running through the editor and not in exported projects.

What do you think about keeping the check in set_scale, since that one is used more rarely and the test is trivial, and finding a different solution for set_transform? Would that be too inconsistent?

That's weird because to my understanding, a zero diagonal essentially means scale IS zero^^"

No idea.. Maybe that's only valid when you have no rotation and translation? I just played around with setting different translations, scales and rotations and looked for what caused problems.

[aaronfranke]

@Zylann For a shrinking animation, you can just have it shrink to 0.001 or something.

@Zylann That's weird because to my understanding, a zero diagonal essentially means scale IS zero^^"

Nope, that is not the case. You can't really make general statements about any specific part of the matrix meaning anything specific about the decomposed transform, except for the last column (wrongly displayed as a row, but it shouldn't be displayed anyway).

The only case where the diagonal alone determines the scale is if all the other Basis elements are zero.

@JohanAR An alternative could be to hide the object if scale is set to 0, but I think that might add a bit of complexity.

I think it's better to throw an error for the user to look at.

[JohanAR]

Updated with some feedback from @aaronfranke (on discord). The check is kept in Node3D::set_transform but wrapped in #ifdef DEBUG_ENABLED to avoid the performance hit on release builds.

I think if this pull request is accepted a similar check in Node2D would be a good idea, but I couldn't test if that is necessary since it seems like 2D rendering isn't ported to 4.0 yet (getting this message if I try to create a 2D scene for testing: "ERROR: FIXME: Mesh, MultiMesh and Particles render commands are unimplemented currently, they need to be ported to the 4.0 rendering architecture.")

One minor issue with this change is that when it blocks setting an invalid transform or scale from the editor, the operation is still put in the undo history. I.e. the first time you press undo after this it will appear as if nothing changed. I don't know if this is acceptable, but fixing it I think requires a more complex change.

[akien-mga]

I think @reduz won't be fond of this check. I added something similar to Node2D in #35073 (forcing a scale != 0), but it's hacky and confusing to users, and I remember @reduz didn't want me to do the same for Spatial.

[JohanAR]

I also made an experiment where I added a Node3D::safe_set_transform, which is regular set_transform plus the param check, and used that one with Node3D's ADD_PROPERTY. I didn't dig that much deeper into it, so I don't know if it would have any unexpected side effects, but it looked like it was working and only affecting changes made in the editor.

[aaronfranke]

@akien-mga In that case, what about adding an editor check in editor/editor_properties.cpp? I'm not entirely sure how useful it would be, but it shouldn't hurt performance since the check would only run when a user manually sets values in the editor.

[madmiraal]

Superseded by #41426.