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.

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

Refactor _scene_cull() to use two stages #74118

Closed
wants to merge 1 commit into from
Closed

Refactor _scene_cull() to use two stages #74118

wants to merge 1 commit into from

Conversation

myaaaaaaaaa
Copy link
Contributor

@myaaaaaaaaa myaaaaaaaaa commented Feb 28, 2023

Splits it into a filter stage (where >99% of runtime is spent) and a binning stage. A followup patch converts the binning stage (which is very prone to race conditions, see #71705 ) to single-threaded so that just the filter stage is multithreaded, greatly simplifying the culling code and removing the need to keep track of a local InstanceCullResult in each thread.

This also opens the doors for potentially adding an option to turn off filtering altogether (resulting in no culling), making it easier to detect culling regressions.

I've also observed a slight increase in culling performance (going from 1 -> 2 threads results in an average of 1.3x speedup rather than 1.2x), although obviously YMMV.

Required for converting _scene_cull() to use parallel foreach(): #78016
Required for occlusion soft-culling: #76297

@RandomShaper
Copy link
Member

Warrants a good review from the rendering team, but I can't spot anything fishy.

My only comment is that I'd favor (uint32_t)1 over 1ULL. More verbose, but clearer IMO.

@myaaaaaaaaa myaaaaaaaaa changed the title Separate _scene_cull() into two stages for better stability Refactor _scene_cull() to use two stages Jul 24, 2023
@myaaaaaaaaa myaaaaaaaaa closed this Sep 3, 2023
@myaaaaaaaaa myaaaaaaaaa deleted the split-cull branch September 3, 2023 22:37
@AThousandShips AThousandShips removed this from the 4.x milestone Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants