Fix bad LOD selection when Camera in Mesh AABB #79590
Conversation
|
CC @fire Does this PR fix your use-case? |
|
I'll try to review later today. @lyuma too. I'll run my test case. |
There was a problem hiding this comment.
Looks reasonable to me: this should at a minimum prevent changing LOD while inside the bounding box which is one of the bugs that you currently get on a large map.
Note that there was another PR open which was attempting to solve the case of inside LOD in a different way by scaling it. See #79191 - I don't quite understand the mechanics of how it works but I just wanted to mention it.
The rest of the existing code that got moved inside the if statement still seems highly suspect, but that can be solved separately. In particular, if you imagine a FOV of 179.9 (the extreme case), looking parallel to the nearest side of the AABB, p_render_data->scene_data->cam_transform.basis.get_column(Vector3::AXIS_Z) will return a vector roughly perpendicular to the nearest wall which could mean inst->transformed_aabb.get_support(); is going to return a point far away. I think the LOD system still has weird issues depending on the angle you look at the bounding box, and I suspect its' related to the use of the forward vector on the camera rather than the vector from the camera to the AABB.
servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp
Outdated
Show resolved
Hide resolved
|
Please use I need help solving my use case of giving a heuristic guess for the decimation of avatars, given they're inside the AABB. The consensus is that picking distance 0 (which we've discussed is not the original mesh) for when the camera is inside the AABB. We need a consensus for the heuristic guess. TL;DR, the design is acceptable for merge. Please revise the technical review. |
|
Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable. If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase |
Fixed an issue where a bad LOD would be selected although the Camera being inside the Meshes AABB.
|
Thanks! And congrats on your first merged Godot pull request! |
Fixed an issue where a bad LOD would be selected although the Camera being inside the Meshes AABB.