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

BVH - add option for expanded AABBs in leaves #55096

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Nov 18, 2021

This PR adds a define BVH_EXPAND_LEAF_AABBS which is set, which stores expanded AABBs in the tree instead of exact AABBs.

This makes the logic less error prone when considering reciprocal collisions in the pairing, as all collision detect is now taking place between expanded AABB against expanded AABB, rather than expanded AABB against exact AABB.

The flip side of this is that the intersection tests will now be less exact when expanded margins are set.

All margins are now user customizable via project settings, and take account of collision pairing density to adjust the margin dynamically.

Related to #54855

Notes

  • This is the second area that could potentially cause bugs mentioned in that PR.

Although initially this change made the BVH a bit slower, I had noticed that the optimum expansion margin was dependent on how dense the local collisions were. When objects are highly stacked, a low margin is best, because it sends the minimum number of collisions to the physics, which becomes the bottleneck. When objects are more rarely interacting, a higher margin is better because the BVH is the bottleneck. So in this PR it uses a metric which lowers the margin dynamically per object based on how many other objects are paired with it.

This significantly improves performance in situations with stacks of items (circa 30% in my tests so far), which performing mostly as well or better with sparser object placements.

  • Although all the maximum margins are now settable via project settings, the default for 2d physics I have increased to 1.0 which is probably a pixel, this gives around 30% increase in performance, as the previous default (0.1) was set for 3d worldspace and was rarely being hit in 2d pixel scenes.
  • I've called the project settings bvh_collision_margin in all cases. I did notice that the 2d physics settings look a little "busy" - @pouleyKetchoupp should there be a "Godot Physics" section in the 2d physics settings as with the 3d settings?
  • This is all currently done with a #define, so can easily be reverted if there are any problems - in particular I'm interested to see how it affects rendering. If this approach seems to work well through a couple of betas, we can commit and remove the old path and probably simplify a couple of little things (there would e.g. no longer be a need to store expanded AABB in the pairing data, as it is now stored in the tree leaves).
  • It may still be worth adding another secondary more accurate phase after the broadphase, but now with the dynamic margins, it may not be necessary. Either way this would be a separate later PR.

Both the movement of A and of B have a certain amount of 'slop', because in each case it is testing an expanded bound against an exact AABB.
(2) is a bit bodgey but has been working empirically, except in this situation, because of the bug in (1).
So I can fix (1) and it will probably work, I'm just wondering whether I should fix (2) though.
Anyway I'll fix 1 first.
Part of the problem as well is that the Tree itself uses exact AABBs, and will do exact cull tests, but it is only the bvh.h wrapper which handles the pairing, and that is the only thing that deals with expanded bounds. So currently there's no way to do a expanded bound versus expanded bound cull check.
For (2) we could potentially instead store only expanded bounds in the tree, and follow the broadphase checks with an exact check.

@Calinou Calinou added this to the 3.5 milestone Nov 18, 2021
@lawnjelly lawnjelly changed the title [WIP] BVH - add option for expanded AABBs in leaves BVH - add option for expanded AABBs in leaves Nov 19, 2021
@lawnjelly lawnjelly marked this pull request as ready for review November 19, 2021 07:41
@lawnjelly lawnjelly requested review from a team as code owners November 19, 2021 07:41
@lawnjelly
Copy link
Member Author

lawnjelly commented Nov 19, 2021

Rebased now #55050 has been merged.

BTW, all these PRs will be relevant in 4.x too, but I'll get them all working in 3.x first then will port in case of any differences in 4.x.

Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Looks great!

I haven't spotted any regression, and in 2D performance is 20% better in my broadphase test compared to 3.x before the recent BVH fixes, which is kind of expected due to using a higher expansion margin!

All other performance tests I've run have similar results as before, which means the BVH expansion fixes have no measurable cost in these tests.

I've made a little comment for the documentation, but apart from that it should be good to go.

doc/classes/ProjectSettings.xml Show resolved Hide resolved
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

I can already approve, it just needs one last tiny change in the documentation.

This PR adds a define BVH_EXPAND_LEAF_AABBS which is set, which stores expanded AABBs in the tree instead of exact AABBs.

This makes the logic less error prone when considering reciprocal collisions in the pairing, as all collision detect is now taking place between expanded AABB against expanded AABB, rather than expanded AABB against exact AABB.

The flip side of this is that the intersection tests will now be less exact when expanded margins are set.

All margins are now user customizable via project settings, and take account of collision pairing density to adjust the margin dynamically.
@lawnjelly
Copy link
Member Author

I can already approve, it just needs one last tiny change in the documentation.

I've also added a similar note to the render tree bvh_collision_margin setting, and also render tree balance, which only takes effect in the octree.

Comment on lines +68 to +69
x = MIN(x, 1.0);
x = 1.0 - x;
Copy link
Member

Choose a reason for hiding this comment

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

Is there any risk to end up with a negative x with floating point errors?
If yes, could possibly be changed to something like:

Suggested change
x = MIN(x, 1.0);
x = 1.0 - x;
x = MAX(1.0 - x, 0);

Copy link
Member Author

Choose a reason for hiding this comment

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

No I don't think so, 1.0 is defined precisely in floating point afaik, and even then I'm not sure a negative expansion margin would be a problem in this case. Should be fine I think. (I do this all the time so it better be lol 😁 )

@pouleyKetchoupp pouleyKetchoupp merged commit 3970f28 into godotengine:3.x Nov 22, 2021
@lawnjelly lawnjelly deleted the bvh_expanded_leaf branch November 22, 2021 17:21
@akien-mga
Copy link
Member

Cherry-picked for 3.4.1.

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.

4 participants