Don't try to merge unused bone AABBs in the rendering server

[aaronfranke]

This PR fixes #56458. With this PR, the test project works, I can import the beetle car GLB without any errors.

I had investigated this half a year ago and was hoping that someone from rendering could confirm this was the correct fix. However, after thinking it over, I am fairly confident that this is indeed the correct fix.

[lawnjelly]

Partly I hadn't commented because I'm not familiar with the pipeline for bones in 3d in godot, especially in 4.x.

Ideally it would probably be nice to mark bones as unused in another way (like passing some bitflags or something), but I suspect the original author thought this might be an easy way of stopping some problem at the time (just making size negative). They are probably essentially hacking in this bool into the AABB, either to make the API smaller and simpler, save typing etc, which might be okay to continue this approach, or might not.

There are two potential downsides that jump out:

1. Could there be legit negative AABBs as a result of bones being mirrored in the skeleton (I don't know, no idea where the AABB calculation code is)
2. Checking no volume is not as cheap as say, checking a bool or bitflag. But then again it's not super expensive, and unless it appears on a profile probably isn't worth worrying out

So I guess providing (1) doesn't seem to occur (I would check this code rigorously) this might not be bad just to fix the current bug, then see if any more appear. If there are knock bugs you could consider something else.

[YuriSizov]

cc @clayjohn

[reduz]

This makes sense to me (I will approve)

[clayjohn]

Looks good to me too

[lawnjelly]

Sorry to only notice this just after merging, but should it be:

const AABB &bone = p_surface.bones_aabbs[i];

for a const reference rather than copying the AABB?

And then using bone directly in the next line:

mesh->bone_aabbs.write[i].merge_with(bone);

It is possible the optimizer might optimize this out, but there seems to be little point in copying by value here.

And same in the 2nd section below.

EDIT:
It looks like the name for this optimization is copy elision:
https://en.wikipedia.org/wiki/Copy_elision
But the compiler can't always do it, depending on what jiggery pokery e.g. the accessors do. So it is usually better practice to not depend on this and be explicit about it.

[Calinou]

It looks like the name for this optimization is copy elision:

I remember one of C++17's features being guaranteed copy elision. Since Godot uses C++17 in master, does this address the issue?

[lawnjelly]

I wouldn't rely on anything like that, especially in hot loops. 😁

Be explicit in what you are asking the compiler to do, and you won't go far wrong. If you are vague, be prepared to examine the assembly. Which will take 10x as long as just writing it correctly in the first place.

[aaronfranke]

Ah, I completely forgot about using bone on the next line. And yeah, I'll update the code to have the AABB a const ref.

[permelin]

I have models that still get "AABB size is negative". The problem is that the object that merge_with() is called on is (-1, -1, -1) while this fix only checks if the the argument has no size. Is this a different bug?

I can try to create a minimal GLB if it helps.

[aaronfranke]

@permelin Yes, a new test GLB would be helpful. The test project in the linked issue was fixed with this PR.

[permelin]

@aaronfranke Here it is. It admittedly looks a bit weird, but I started with a humanoid character and reduced it down as much as I could while keeping the error.

Let me know if I should create a new issue.

godot_negative_aabb_minimal.zip