AABB::expand_to spamming errors in the portals

[lawnjelly]

Godot version

3.x dev (5th Jan 2022)

System information

All

Issue description

The AABB::expand_to function has been changed in #37626 to spam errors when size is negative.

This is causing problems because a common method to set AABBs is to set opposite maximums, i.e. set position to maximum, and size to negative maximum, and then loop through points calling expand to for each point. Thus the first point sets the first position and size to zero.

An alternative is to explicitly set the first element in the calling code, or use intermediate code for mins / maxes then set to an AABB later. However it is problematic that AABB can no longer handle this common paradigm.

Steps to reproduce

Create a Room with a static Mesh inside, and convert.
Example use : RoomManager::_bound_findpoints_mesh_instance

Or more simply, just the following code:

AABB aabb;
aabb.position = Vector3(FLT_MAX / 2, FLT_MAX / 2, FLT_MAX / 2);
aabb.size = Vector3(-FLT_MAX, -FLT_MAX, -FLT_MAX);

aabb.expand_to(Vector3(0, 0, 0));

Minimal reproduction project

No response

[akien-mga]

Godot version

3.4.2

I guess you mean 3.x? This PR hasn't been included in 3.4.2-stable.

[lawnjelly]

Godot version

3.4.2

I guess you mean 3.x? This PR hasn't been included in 3.4.2-stable.

Ah yes good point, I only noticed it when checking another issue with latest 3.x, and assumed it was already in 3.4.2, I'll amend.

[lawnjelly]

Fixed by revert above.

[aaronfranke]

This is causing problems because a common method to set AABBs is to set opposite maximums, i.e. set position to maximum, and size to negative maximum, and then loop through points calling expand to for each point. Thus the first point sets the first position and size to zero.

Why can't you just set the starting AABB to the first entry in the loop of points? Instead of this:

AABB aabb;
aabb.position = Vector3(FLT_MAX / 2, FLT_MAX / 2, FLT_MAX / 2);
aabb.size = Vector3(-FLT_MAX, -FLT_MAX, -FLT_MAX);
for (i = 0; i < points.size(); i++) {
    aabb.expand_to(points[i]);
}

Why not:

AABB aabb = points[0];
for (i = 1; i < points.size(); i++) {
    aabb.expand_to(points[i]);
}

(The latter will crash for a zero size list, but the former would also be weird since it returns a negative size AABB, so I'm not sure what the behavior is supposed to be be in that special case).