godotengine
Improve physics body/shape scale warnings, check shear/global, add to 2D
Fixes https://github.com/godotengine/godot/issues/88415
Previously, the body/shape scale warning code did not handle these cases:
- If an ancestor node had a non-uniform scale.
- If the node or an ancestor had a shear/skew.
This PR changes the code to check if the Basis is conformal, which handles all these cases. It actually does this twice, another time for the global transform, to ensure that there is no invalid transform inherited from ancestors (we also want the node's local transform to be validated in addition to global).
Also, this PR adds the same checks to 2D, since it was missing before.
Note: Negative scale is allowed in master and still allowed in this PR.
Note: Zero scale is invalid but is not validated in master or in this PR. I don't know if that's worth checking for since it's kinda obvious that it's invalid because most things in 3D are considered invalid if they have an zero scale.
In https://github.com/godotengine/godot/pull/67847 I did not add the warnings to 2D because I could not reproduce the issues there. I did see some issues later with CCD when using non-uniform scale. Are there any open issues except for https://github.com/godotengine/godot/issues/12335?
If you can reproduce issues with 2D, that would justify adding the warnings, but otherwise we maybe should wait with adding those to avoid annoying users unnecessarily (even if it may be teaching good practice).
The code changes look good.
@rburing I do not use 2D physics so I'm not sure, but many physics calculations don't quite work the same with non-conformal transforms. For example, circles would become ellipses. If it really works with Godot's physics I could remove it I suppose, however, I think it's still best to keep this check because 1) It's a good practice for physics objects to have conformal transforms and 2) What about third-party 2D physics engines like Box2D or Rapier2D, wouldn't it be a bad idea to assume all 2D physics engines support non-conformally transformed physics objects?
What about third-party 2D physics engines like Box2D or Rapier2D, wouldn't it be a bad idea to assume all 2D physics engines support non-conformally transformed physics objects?
I guess I'll paraphrase myself from #88914 seeing as how there's a good amount of overlap here.
I can't speak for 2D, but I would prefer to see these is_conformal() checks done in the physics server instead, so as to allow other physics engines to use non-uniform scaling and whatever else they might want to allow.
With Jolt you can non-uniformly scale boxes, cylinders (so long as X and Z stays the same), convex polygons and concave polygons without any issues. I don't know if the same holds true for Godot Physics.
You can also scale physics bodies with Jolt to some degree as well.
Seeing as how this PR (unlike #88914) also checks to see that the basis is orthogonal I guess the method name proposed in #88914 of PhysicsServer*D::shape_is_valid_scale isn't as good of a fit, but maybe some PhysicsServer*D::shape_is_valid_transform and PhysicsServer*D::body_is_valid_transform? I'm not sure what the error message would be in that case though, given that the physics server could then reject it based on any part of it.
many physics calculations don't quite work the same with non-conformal transforms. For example, circles would become ellipses. If it really works with Godot's physics I could remove it I suppose, however, I think it's still best to keep this check
If the approach of "all non-uniform scaling is bad" is taken, then there should be primitive shapes for things like ellipses. Or even better some more generic "oval" shape (like eggs).
Just as an FYI, there are runtime scaling checks happening server-side in the newly added Jolt module, which is a somewhat crude carry-over from the extension. See these methods, these macros and their usage.
Jolt provides a JPH::Shape::IsValidScale method that checks to see whether a given scale is valid (within a tight tolerance) for that particular shape instance, allowing for things like non-uniform scaling for certain shapes (even compound shapes). Ideally this would have been used through a configuration warning I guess, similar to what this PR does, but I wasn't able to add to that from the extension for the default physics nodes, so I went with the runtime checks.
However, just relying on JPH::Shape::IsValid for these runtime checks quickly became a point of friction, as I also support scaling all physics bodies, and doing something as simple as rotating a CharacterBody3D over a few thousand frames without orthonormalizing meant you'd end up with a slightly non-uniformly scaled transform and trigger these runtime checks.
As a result, a new method was added to Jolt, called JPH::Shape::MakeScaleValid, which changes the scale just enough to be valid for that given shape instance. So what I do instead is always call this method, and in DEBUG_ENABLED builds emit an error if the valid scale differs from the given scale within a tolerance that's greater than the one in JPH::Shape::IsValidScale (0.01 instead of 0.0001). That way you end up with Jolt always receiving a valid scale on its end, but we only emit an error if the user is deliberately scaling things the wrong way, or when they have a very dire precision problem.
With that said, there are still some people who find this behavior annoying, since it results in significant error spam in situations where you might not necessarily care about that particular problem, so there might be some argument for just ditching these runtime checks altogether and solely rely on node configuration warnings instead, but I would still like to see that these checks be deferred to the physics server in some way, so that we can allow for Jolt's more relaxed requirements around scaling.