Transform3D Aabb operator * does not handle rotations properly [C#]

[jasonswearingen]

Tested versions

reproduced in 4.3.stable.mono not attempted in other versions

System information

Godot v4.3.stable.mono - Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 970 (NVIDIA; 32.0.15.6070) - AMD Ryzen 9 7900 12-Core Processor (24 Threads)

Issue description

I think that the csharp code for public static Aabb operator *(Transform3D transform, Aabb aabb) does not handle rotated+offsetPosition AABB's correctly.

The code, from Godot's Transform3D.cs is as follows: https://github.com/godotengine/godot/blob/0a4aedb36065f66fc7e99cb2e6de3e55242f9dfb/modules/mono/glue/GodotSharp/GodotSharp/Core/Transform3D.cs#L460

When I change to use my own custom version, everything works properly:

    public static Aabb _Transformed(this Aabb aabb, Transform3D transform)
    {
        ////////////////////////////////////////////////////////
        //full corner method:  accurate
        /////////////////////////////////////////////////
        ////
        //// Get the 8 corners of the AABB

        Vector3[] corners = new Vector3[8];
        corners[0] = new Vector3(aabb.Position.X, aabb.Position.Y, aabb.Position.Z);
        corners[1] = new Vector3(aabb.Position.X, aabb.Position.Y, aabb.End.Z);
        corners[2] = new Vector3(aabb.Position.X, aabb.End.Y, aabb.Position.Z);
        corners[3] = new Vector3(aabb.Position.X, aabb.End.Y, aabb.End.Z);
        corners[4] = new Vector3(aabb.End.X, aabb.Position.Y, aabb.Position.Z);
        corners[5] = new Vector3(aabb.End.X, aabb.Position.Y, aabb.End.Z);
        corners[6] = new Vector3(aabb.End.X, aabb.End.Y, aabb.Position.Z);
        corners[7] = new Vector3(aabb.End.X, aabb.End.Y, aabb.End.Z);

        // Transform each corner
        for (int i = 0; i < 8; i++)
        {
            corners[i] = transform * corners[i];
        }

        // Find the minimum and maximum points of the transformed box
        Vector3 min = corners[0];
        Vector3 max = corners[0];

        for (int i = 1; i < 8; i++)
        {
            min = min.Min(corners[i]);
            max = max.Max(corners[i]);
        }

        // Create a new AABB from the min and max points
        return new Aabb(min, max - min);
    }

I am not a math guy so all I can guess is that there is some order-of-operations issue going on here with the matrix calculations.

I discovered this because I wrote a kitbash importer that successfully imports complex multimesh composite "models" and I manually track the AABB's of these for use in my custom BVH. For objects that are both rotated and positioned, I noted that transforming the meshInstance3D.GetAabb() had the wrong positioning. It took me a while, but I noticed that my custom OBB's were getting calculated properly, so eventually I tracked it down to this transform operator *

Steps to reproduce

I have a very complex project so it's hard for me to figure out an easy way to provide repo-steps, but it seems that this problem occurs when a model with assets not at the origin is rotated.

I have attached an export of the model I noticed this problem with. quoin_detailed.glb.zip

In the image below: I notice that when I put this model into a scene, godot shows a debug bounds for it that is off, similar to (but not the same) as the issue I have seen. This leads me to think there's an issue with the godot native transform code also.

Minimal reproduction project (MRP)

as I'm not a math guy I don't really fully understand the issue, so maybe if you can glance over the current operator * implementation and consider my statement, you'll realize that either there's a problem or if I have some workflow issue.

[jasonswearingen]

This is an image where I visualize the AABB's using my custom Transform extension method

---

Below is an image where I visualize the AABB's using the builtin transform * aabb operator. Nothing else is changed other than switching from using my custom Transform to the builtin.

Notice that the most obvious outlier is the "quoin" piece bounds in the wrong location, seemingly not rotated properly.

[jasonswearingen]

If this issue is determined to be valid, here is a more optimized solution you can use

    public static Aabb _Transformed(this Aabb aabb, Transform3D transform)
    {
        //builtin method has a bug:  it doesn't handle rotations properly
        //return transform * aabb;

            {
            //////////////////////////////////////////////////////////
            ////geometric rotation method: faster and accurate
            ///////////////////////////////////////////////////

            // Extract scale from the transformation matrix
            Vector3 scale = transform.Basis.Scale;

            // Calculate the half-extents and center of the original AABB
            Vector3 halfExtents = (aabb.End - aabb.Position) * 0.5f;
            Vector3 center = aabb.Position + halfExtents;

            // Apply the scale to the half-extents
            Vector3 scaledHalfExtents = new Vector3(
                halfExtents.X * scale.X,
                halfExtents.Y * scale.Y,
                halfExtents.Z * scale.Z
            );

            // Use the basis directly for rotation
            Vector3 rotatedX = transform.Basis.X.Normalized();  // Rotated X-axis
            Vector3 rotatedY = transform.Basis.Y.Normalized();  // Rotated Y-axis
            Vector3 rotatedZ = transform.Basis.Z.Normalized();  // Rotated Z-axis

            // Project the scaled half-extents onto the new rotated axes
            Vector3 newHalfExtentsX = scaledHalfExtents.X * rotatedX.Abs();
            Vector3 newHalfExtentsY = scaledHalfExtents.Y * rotatedY.Abs();
            Vector3 newHalfExtentsZ = scaledHalfExtents.Z * rotatedZ.Abs();

            // Compute the new half-extents by summing the absolute values of the projections
            Vector3 newHalfExtents = newHalfExtentsX + newHalfExtentsY + newHalfExtentsZ;

            // Transform the center point
            Vector3 transformedCenter = transform * center;

            // Create the new AABB from the transformed center and new half-extents
            Vector3 newMin = transformedCenter - newHalfExtents;
            Vector3 newMax = transformedCenter + newHalfExtents;

            return new Aabb(newMin, newMax - newMin);
        }
}

[kleonc]

Confirmed, the calculations unintentionally swapped rows/columns. Workaround until fixed (#97208):

Transform3D transform3d = ...;
Aabb aabb = ...;

Aabb wrong = transform3d * aabb;
Aabb correct = new Transform3D(transform3d.Basis.Transposed(), transform3d.Origin) * aabb;

[jasonswearingen]

@kleonc thank you for investigating. I think there is a similar issue with the godot native calculations, as this screenshot the orange bounds is godot's editor default selection visualization, which closely matches to the bounds if I were to visualize the transformed aabb's using the current buggy builtin transform

[kleonc]

I think there is a similar issue with the godot native calculations, as this screenshot the orange bounds is godot's editor default selection visualization, which closely matches to the bounds if I were to visualize the transformed aabb's using the current buggy builtin transform

@jasonswearingen I've checked out this model you've provided:

I have attached an export of the model I noticed this problem with. quoin_detailed.glb.zip

And note that by importing it with default settings there's more then just a MeshInstance3D node in there, and that's why it produces AABB exceeding the mesh bounds:

(added it as a child of plain Node3D and enabled Editable Children for inspecting)

After marking Group to be skipped and reimporting:

So briefly looking I don't see AABB calculations being wrong here.

If there are some issues with the importing etc. then they should be reported in separate issue though, this one is specifically about faulty Transform3D * Aabb calculation in C# (which is already addressed in #97208).

[jasonswearingen]

Thank you so much for the detailed explanation. I understand, and it makes sense. Please feel free to close this whenever it makes sense to (I'm not sure if your policy is to wait until the fix is merged, or if it should be closed now)