Rendering BVH is failing cull some meshes [Regression]

[AndreaCatania]

Godot version:
98ccaa1

OS/device including version:
Fedora 33

Issue description:
BVH is not able to properly cull the meshes when you play the scene (in editor it looks fine). This issue is a regression of this PR: #44623

The scene should look like the following:

but instead it looks like this:

Steps to reproduce:

1. Checkout the master or 3fdf4bf
2. Compile.
3. Open the attached project.
4. Play the scene.
   In editor it looks fine, while the floor is not there if you play the scene.

If you checkout 3fdf4bf, you can verify that the new BVH is the issue by doing:

1. git reset --hard HEAD~1, to exclude the BVH changes.
2. Compile.
3. Open the editor.
4. Play the scene.
   You will notice the ground is correctly culled.

Minimal reproduction project:
bvh.zip

[lawnjelly]

I'm not sure it is worth investing time to fix bugs in this, it could be a real rabbit hole. Imo we should probably just replace it with the version from #44901, which has been tested far more rigorously, and should be much faster.

I was kind of waiting until the 3.2 version got merged, but I can expedite a 4.0 version.

[AndreaCatania]

@lawnjelly Probably worth test it with this scene too, so to confirm that merging that PR would solve this issue too.

[lawnjelly]

Runs fine in 3.2 with octree or BVH. The shadows from the crates strangely didn't appear in the editor, but do when running the scene, not sure what that is about, something to do with the material from the gltf perhaps. I don't think that is related though.

Another possibility BTW, is that the AABBs are wrong for some reason due to e.g. the import. This would cause culling to not work correctly.

[AndreaCatania]

Another possibility BTW, is that the AABBs are wrong for some reason due to e.g. the import. This would cause culling to not work correctly.

On this note, the 4.0 editor shows the AABB and it seems correct. However, I want to remark that octree is able to handle it so it's unlikely caused by a wrong AABB.

[lawnjelly]

And here it is working fine in Godot 4.0 with the new BVH.

It's been a little bit more involved converting the new BVH as the culling tests use callbacks in 4.0, but it's quite doable. I'd still prefer to get 3.2 merged and tested first really, as otherwise I'm wary of duplicating work, but I could get potentially get Godot 4.0 working with a wrappered version to start with (it's better than nothing). Just depends on how hurried people are to have this fixed.

[AndreaCatania]

This Issue is fixed by this PR: #44799

- if (p_instance->scenario == nullptr || !p_instance->visible || Math::is_zero_approx(p_instance->transform.basis.determinant())) {
+ if (p_instance->scenario == nullptr || !p_instance->visible || p_instance->transform.basis.determinant() == 0) {

[lawnjelly]

Does beg the question, why does using the octree or my BVH fix it?

It seems possible that #44799 hides the bug - possibly the current BVH isn't working with small AABBs, perhaps zero size after the quantize (or negative size).

[AndreaCatania]

While testing the full map, today, I noticed that even your BVH is not properly working. I don't know if the Gordon's fix is hiding the real issue, however, it seems fully fixed.

[RevoluPowered]

The problem was that the AABB was filtered if the values were small, this is incorrect behaviour because the intended behaviour was to filter out invalid AABB's. I wanted to fix all the cases where AABB's had an actual zero value, like the Editor 3D Grid creates data which is invalid which makes it to the check supplied in the PR.

In summary two cases need supported:

• aabb determinant can be close to zero and not be a precision error, so it shouldn't be filtered this way. (remember values close to zero is not meaningful when speaking about the determinant() since its based on the entire matrix)
• aabb determinant where values are actually zero, like the 3D editor grid did before was also filtered by this code.

I think instead a check should be done on scale values being close to zero, but this fix works now, so better to use it.

This also resulted in some case the cull code producing problematic performance, so worth re-running benchmarks with the fix once I rebase the PR.

Edit: Begs the question, if people use zero scale values to hide things we should definitely be checking scale.

[AndreaCatania]

Edit: Begs the question, if people use zero scale values to hide things we should definitely be checking scale.

I don't think we should support such feature. We have the visibility flag and the user should just use that; add an extra check to support this bad habit doesn't seems a good solution.

[RevoluPowered]

Accidentally edited comment when I meant to hit quote. Sorry Andrea.

I agree with you. Yeah I updated the editor grid in the fix PR to use visibility instead as it wasn't doing this before.

[AndreaCatania]

Nice!

[akien-mga]

Fixed by #44799.