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

Eliminate collision checks between geometry in rendering BVH. #72511

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Feb 1, 2023

Later logic using the pairable_mask would catch cases preventing pairing callbacks between geometry. However the collision checks were still taking place, wasting performance.

Here we utilize the tree_mask feature of BVH to totally eliminate unnecessary collision checks between geometry.

Notes

  • I noticed when profiling benchmarks of rotating cubes that BVH collision checks were occurring with no lights in the scene. This did not seem necessary.
  • I worked out from the logic that geometry always has a pairable_mask of zero, so is eliminated from pairing with other geometry in the templated UserCullTestFunction() callback used by the BVH, which happens near the end of the collision pipeline.
  • However it was simple to totally remove these collision checks from taking place by changing the tree collision mask (which was maybe introduced with the templated trees?).
  • The performance gains typically aren't massive from eliminating these checks, but are well worth doing as it is free speedup. More likely to be beneficial in scenes with lots of moving objects.
  • Will need a beta to check for regressions, I don't think there should be, based on the logic, but something could be unforeseen.
  • This improvement can eventually be also added to Godot 4.x (after 4.0 release) as the longterm intention has been to use the new BVH there too for rendering (it currently still uses the old dynamic_bvh.h by reduz for rendering).
  • I also removed the default parameters from the functions involved. These weren't used, and made it more complicated to work out what was going on.

Later logic using the `pairable_mask` would catch cases preventing pairing callbacks between geometry. However the collision checks were still taking place, wasting performance.

Here we utilize the `tree_mask` feature of BVH to totally eliminate unnecessary collision checks between geometry.
@lawnjelly lawnjelly force-pushed the bvh_render_treemask branch from 55994b5 to 18d7d36 Compare March 9, 2023 11:17
@lawnjelly lawnjelly requested review from clayjohn and akien-mga March 11, 2023 13:30
@lawnjelly
Copy link
Member Author

Requested reviews .. on 3.x especially, with pouleyketchoupp gone, I'm now probably the only one familiar with this area (BVH and collision trees) ! 😆

So it's probably going to have to be a "trust me I know what I'm doing". This is just eliminating some redundant collision checks. If they were necessary it should become apparent quickly and we can revert.

If you can follow the logic, you should see that currently even if these collisions are registered, they get ignored in the callbacks, so it's not necessary to do the first phase of checks.

@clayjohn
Copy link
Member

So it's probably going to have to be a "trust me I know what I'm doing". This is just eliminating some redundant collision checks. If they were necessary it should become apparent quickly and we can revert.

Heh, yep. The code looks fine to me and I trust if something breaks you will fix it very quickly

@akien-mga akien-mga merged commit ed78553 into godotengine:3.x Apr 11, 2023
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the bvh_render_treemask branch April 11, 2023 08:58
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.

3 participants