Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch occlusion culling to be based on depth instead of Euclidean distance #103798

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Switch occlusion culling to be based on depth instead of Euclidean distance #103798

wants to merge 1 commit into from
+5 −6

Conversation

Rudolph-B
Copy link
Contributor

Resolves #94210 while undoing #98257 to improve behavior of #99986

Main reasons this change in approach as stated by @Flarkk in #97712 are:

  1. the edge case discussed above is in my opinion not worth the overhead introduced by moving the whole buffer to euclidiant distance (see Store view depth instead of ray length in occlusion buffer #100907)

  2. it ensures occlusion culling still works with very far away objects (see Enable rendering with unbounded far distance #99986 (comment))

Tested using:

This is a re-implementation of #97712 since I was unable to reopen it.

@Rudolph-B @Flarkk
Fix occlusion culling by using depth instead of Euclidean distance wh…
18eb1c1 
…en selecting the closest point

Co-authored-by: Florent Guiocheau <florent.guiocheau@gmail.com>
@Rudolph-B
Copy link
Contributor Author

@Flarkk If you are happy with this I can change the PR to ready for review?

@Flarkk
Copy link
Contributor

Flarkk commented Mar 8, 2025

Awesome ! Will test it against #99986 and let you know.

@AThousandShips AThousandShips added this to the 4.5 milestone Mar 10, 2025
@Flarkk
Copy link
Contributor

Flarkk commented Mar 10, 2025

I just tested it and I can confirm it works all good :

@Rudolph-B Rudolph-B marked this pull request as ready for review March 10, 2025 16:59
@Rudolph-B Rudolph-B requested a review from a team as a code owner March 10, 2025 16:59
Calinou
Calinou approved these changes Mar 11, 2025
View reviewed changes
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected. Code looks good to me.

I've also tested https://github.com/godotengine/godot-demo-projects/tree/master/3d/occlusion_culling_mesh_lod with it and it works correctly.

ProfessorOctopus added a commit to ProfessorOctopus/godot that referenced this pull request Mar 14, 2025
@ProfessorOctopus
Clean up and added pulls
1873878 
A little cleanup.
Fix Invalid Task ID error spam in PipelineHashMapRD. godotengine#104044 by bruvzg
Fix LoadToken memory leak caused by unused resources obtained using load_threaded_request() godotengine#104008 by AlexOtsuka
Improve Audio Stream Player Gizmo Performance godotengine#104016 by kiroxas
Improve capsule gizmo performance godotengine#101533 by kiroxas
Switch occlusion culling to be based on depth instead of Euclidean distance godotengine#103798 by Rudolph-B
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Calinou Calinou Calinou approved these changes

@Flarkk Flarkk Flarkk approved these changes

Assignees
No one assigned
Labels
bug topic:rendering topic:3d
Projects
None yet
Milestone
4.5
4 participants
@Rudolph-B @Flarkk @Calinou @AThousandShips