Check and correct for zero scaling when unscaling Bullet basis.

[madmiraal]

Fixes #40952.
Fixes #41031.

Bullet keeps the scale separate from the rotational matrix. When converting from Godot's Transform, the scale is extracted and the remaining Basis Vectors are normalized. This works if there are no dimensions scaled to zero, but results in a division by zero if any of the dimensions of the CollisionObject or any of it's parents are scaled to zero. The consequence depends on the OS and the compiler, but generally this division by zero results in nans being stored in the Bullet Basis Vectors. Bullet expects the user (Godot) to ensure the Basis Vectors are ortho-normalised. Therefore, they go undetected until they are used in a calculation and cause unexpected results. The above issues show that the unexpected results may be completely unrelated to the Objects with the bad Basis Vectors.

This patch checks for zero scaling when unscaling the Bullet Basis Vectors. If zero scaling is detected, the required Basis Vectors are recreated. The Basis Vectors are recreated in such a way as to ensure that both the "handedness" is correct and the Basis Vectors are aligned to the world axes where possible.

[madmiraal]

Rebased following #41428, which fixed some unrelated style check errors causing CI to fail.

[AndreaCatania]

This change would introduce a significant overhead, considering that the function UNSCALE_BT_BASIS is used consistently.

The reason for this change is to make sure the user doesn't use the engine in the wrong way, so I was thinking in this solution:

#ifdef DEBUG_ENABLED
if (unlikely(btFuzzyZero(column0[0]) || btFuzzyZero(column1[1]) || btFuzzyZero(column2[2]))) {
	column0 = btVector3(1, 0, 0);
	column1 = btVector3(0, 1, 0);
	column2 = btVector3(0, 0, 1);
	ERR_PRINT("The physical bodies and the shapes must always have greater than 0 scaling.");
}
#endif

In this way:

• We have a lot less checks (that are usually slow).
• The user is warned to fix it.
• At runtime we have valid data.
• This check is removed with non debug build.

What do you think?

[madmiraal]

This change would introduce a significant overhead

Although it looks like a lot of code, the actual additional overhead in this PR is a single cross() if one or two of the dimensions is scaled to 0. Note, a cross() is less of an overhead than a normalize().

We have a lot less checks (that are usually slow).

The number of checks in this PR and the proposed alternative is the same. Although using unlikely on the checks may help the compiler make it faster, aside from avoiding a JMP it won't make things much faster. I also wouldn't call these checks "slow".

The user is warned to fix it.

Putting the warning here, would spam the console. Furthermore, as the issues this PR fixes allude to, the problematic shape is often far removed from the problem being experienced; so the warnings would often be ignored while trying to fix the "bigger" problem.

At runtime we have valid data.

By resetting the basis as suggested in the proposed alternative, the orientation of the shape in the physics engine would be different from the actual orientation. This PR specifically ensures that the basis is both valid and correct.

This check is removed with non debug build.

By only making the correction when DEBUG_ENABLED, the issues would only arise in the released game. The last thing we'd want is for the game to work in the editor, but not work in the released game.

[AndreaCatania]

Putting the warning here, would spam the console. Furthermore, as the issues this PR fixes allude to, the problematic shape is often far removed from the problem being experienced; so the warnings would often be ignored while trying to fix the "bigger" problem.

I would put an error anyway, because the user is not allowed to submit such transformations, and this new code just tries to lower the danger introduced by the wrong transformations - but is not able to always extract the wanted transform.

See the cases when the scale is 0, in 2 or 3 axis.

The engine would just behave strange, like if it's broken, so print an error is necessary.

By only making the correction when DEBUG_ENABLED, the issues would only arise in the released game. The last thing we'd want is for the game to work in the editor, but not work in the released game.

The user is supposed to release the game with 0 errors in the console and this proposal is not too different from what the fast math flag does.

I propose to

• Add some ERR_PRINT so to correctly guide the user.
• Add the unlikely so to help the compiler.
• Use the DEBUG_ENABLED to avoid those checks in release.

[madmiraal]

is not able to always extract the wanted transform. See the cases when the scale is 0, in 2 or 3 axis.

This PR will ensure that the Basis vectors are correct, and the transform is what is wanted; when one or more scales are zero. When the scale is 0 in 2 or 3 axes, there are an infinite number of correct transforms; so this PR deliberately selects the (correct) transform that aligns the axes with the world axes where possible.

I would put an error anyway, because the user is not allowed to submit such transformations

With this fix, users will be able to successfully submit such transformations to Bullet physics. The only reason I can find for not allowing such transformations is that (without this fix) they weren't being submitted to Bullet properly, which caused collisions to fail. Godot physics (and the editor) may have other issues with zero scales, but that doesn't mean we should spam the user with error messages here.

The engine would just behave strange, like if it's broken

Not with this fix. I've tested it with this simple project:
Godot 3.2: TestScale-3.2.zip
Godot 4.0: TestScale-4.0.zip

[AndreaCatania]

Let's suppose the best case, where the user puts the RB rotated by 45 degrees in the X axis. Then it scales the Y and Z axis to 0.

As you said, you have infinite possibilities to choose those two axis. So, the taken axis are unlikely to be rotated as the user originally intended, resulting in a weird transformation that can potentially make appear wrong the entire simulation.

(Just imagine a floor object rotated by 45 degrees, with its collider being un-rotated internally, without making the user know about why that happens. He would just think the physics engine is broken, which is what is happening here.)

Following the above case, we can make it a bit worse by also rotating another axis, or we could continue by putting all those 3 axis to 0.

When I say that the result transform is not correct, is because you can potentially obtain a transform that is different from the one that the user thinks he's using. The guessed transform, that for sure make the simulation going, unlikely is like the one expected. So this PR just move the issue.

By design, Godot cannot be released with errors in the console. Indeed any error is converted into a crash in production. (Try to create a broken GDScript, and release it. It will crash.).
Following this design I think that my proposal is appropriate for this use case.

I propose to:

• Add some ERR_PRINT so to correctly guide the user.
• Add the unlikely so to help the compiler.
• Use the DEBUG_ENABLED to avoid those checks in release.

[madmiraal]

Let's suppose the best case, where the user puts the RB rotated by 45 degrees in the X axis. Then it scales the Y and Z axis to 0.
As you said, you have infinite possibilities to choose those two axis. So, the taken axis are unlikely to be rotated as the user originally intended, resulting in a weird transformation that can potentially make appear wrong the entire simulation.

If you scale an object in the y and z axes to 0. You have a line along the x-axis. A rotation around the x-axis will have no effect. This is the reason there are an infinite number of correct transforms. Any transform representing any rotation around the x-axis will be correct. Furthermore, I have tested this scenario, and it works as expected i.e. the collision shape acts as a thin bar on the x-axis that other objects must go around.

(Just imagine a floor object rotated by 45 degrees, with its collider being un-rotated internally, without making the user know about why that happens. He would just think the physics engine is broken, which is what is happening here.)

This is the the scenario I've presented, tested and shown to work in the simple project included in my previous comment.

we can make it a bit worse by also rotating another axis,

I've also tested this (for example, with a 45° rotation in both the y and z axes and a zero scale on the y-axis), and it also works as expected.

we could continue by putting all those 3 axis to 0.

In this scenario, you've reduced the object to a single point. Any rotation will have no effect; therefore any transform will be correct. When tested, it also works as expected i.e. the collision shape acts as a single point that other objects must go around.

When I say that the result transform is not correct, is because you can potentially obtain a transform that is different from the one that the user thinks he's using. The guessed transform, that for sure make the simulation going, unlikely is like the one expected. So this PR just move the issue.

The transform selected by this PR is one of the correct transforms. Any of the correct transforms would work; however I have taken the extra step of selecting the correct transform where the axes are aligned with the world axes when possible. These transforms are not guessed. They are calculated from the axes that are not zero, and therefore guaranteed to be correct. These transforms have been tested and shown to provide the expected outcomes. This PR solves the issue, it does not move it.

I think that my proposal is appropriate for this use case.

I have provided the reasons why I think your proposal is not appropriate in an earlier comment; so I will not repeat myself here.

[AndreaCatania]

Make sense, thanks!

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 3.2.4.