Fix AABB computation for position compression to not depend on vertex order

[zeux]

The previous computation was dependent on the vertex order in two ways:

• If the first vertex was on the AABB boundary, the AABB would be increased by the epsilon due to size clamping
• Every time the AABB would get expanded, we would recompute end from size and reconstruct size again, which resulted in floating point drift.

In isolation this may not seem like it matters, but it means that the same mesh with a different vertex order may result in a slightly different AABB. This can be a significant issue due to shadow meshes and their use in depth prepass: shadow meshes reorder vertex data as part of the deduplication process, as they append one unique position at a time and as such remove the duplicate positions; this can result in a different AABB which would result in a different reconstructed vertex position during a depth pre-pass, causing mesh self-occlusion.

Bugsquad edit: Fixes #91962

[zeux]

I initially discovered this issue on several meshes when looking into issue 93587 (something like 5 out of 100 unique meshes there have this problem). Since then I also found other simpler meshes that reproduce it, for example PotOfCoals (https://github.com/KhronosGroup/glTF-Sample-Assets/tree/main/Models/PotOfCoals):

What happens here is that the depth prepass uses shadow mesh which uses a separate vertex buffer that has been compressed independently; during compression, floating point drift results in a slightly different AABB which then, for some vertices, results in a different rounding direction - the u16 position components for black vertices that are in the center of the black patch are 1-off from the version used in the color pass. This results in the color rendering failing the depth test, and the clear color - black here - remains. This doesn't affect mobile renderer because it doesn't enable depth prepass as far as I can tell.

After fixing this the rendering of this mesh, as well as other affected meshes, is now proper (after reimport):

[zeux]

Two additional issues that I've discovered while debugging all of this that someone might want to pick up:

1. Some assets, like PotOfCoals above, have a lot (6x!) of redundant vertices. These would presumably get cleaned up during .obj import as part of reindexing, but that doesn't happen during .gltf import. Something to consider is reindexing the mesh, perhaps conditionally if it doesn't have blend shapes and has vertex_count that is more than index_count*2 or thereabouts, to avoid import time overhead of reindexing on reasonable meshes. The status quo is that some users will unknowingly import very inefficient assets and use a lot of vertex memory as a result.
2. The compression code forgets to properly round values during quantization - instead of eg a * 65535 or a * 255 it should be a * 255 + 0.5f. See https://zeux.io/2010/12/14/quantizing-floats/. This results in ~2x higher average quantization error than strictly necessary.

[lawnjelly]

Great spot. 👍
This was ringing a bell, and it turns out we already have such a create_from_points() function on AABB in 3.x.

As in 3.x, this is so common, I'd be wondering whether to add to AABB rather than RenderingServer. (This is my personal opinion, other reviewers may differ.)

Doing this from raw pointer + length is a pain point in Godot as we don't have things like spans so we can end up needing multiple wrappers for the various vectors, or having it convert (which is slow). Maybe a slightly hidden public like this would do the job to start and leave room to improve:

void AABB::_create_from_points(const Vector3 *p_points, uint32_t p_length)
{
	ERR_FAIL_NULL(p_points);

	if (p_length == 0) {
		return AABB();
	}

	Vector3 min = p_points[0];
	Vector3 max = p_points[0];

	for (uint32_t i = 1; i < p_length; i++) {
		min = min.min(p_points[i]);
		max = max.max(p_points[i]);
	}

	return AABB(min, (max - min));
}

then doing the min_size in the calling code as less common.

I bet you'll find this is already used in a few places throughout the codebase already which can also be converted (maybe in 2nd PR). This was the case in 3.x.

Coding convention wise we use p_ for parameters that are not returned, and r_ for parameters that are returned (e.g. by reference).

[zeux]

Fixed code style in place for now and moved min_size outside of the function.

[lawnjelly]

Actually this is likely fine for now, we can look at moving to AABB in separate PR if there's support.

[akien-mga]

Thanks!