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 broadphase creates objects with updated AABB to avoid extra checks #45319

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

pouleyKetchoupp
Copy link
Contributor

Description

When set_static is called on a newly added object, the forced collision check in BVH set_pairable was using an empty AABB, which caused unnecessary collision checks at the origin, then a call to move was checking again at the right position.

These changes ensure broadphase objects are added to the BVH tree with proper AABB so collision checks are correctly done right away.

Octree & Basic broadphase types are not affected by these changes.

I'm not doing the same changes in the 2D physics interface because there's no use for them in the 2D broadphase at the moment.


Test Scenario

Test project:
physics_tests.zip

Scene:
res://tests/performance/test_perf_broadphase.tscn

This test adds 125000 non-overlapping objects in physics at once, moves them all then removes them all. It logs timings for the different operations and physics ticks for the following frames.


Test Results

I'm getting almost 30% improvement on adding objects in this test scenario, which makes the dynamic BVH slightly better than Octree when it comes to adding objects.

Detailed results (release_debug):

BVH - Previous version
* Creating objects...
  Create Time: 2319.604 ms
  Physics Tick: 2324.632 ms (total = 2324.632 ms)
  Physics Tick: 12.727 ms (total = 2337.359 ms)
  Physics Tick: 1.553 ms (total = 2338.912 ms)
  Physics Tick: 1.879 ms (total = 2340.791 ms)
  Physics Tick: 1.469 ms (total = 2342.260 ms)
* Adding objects...
  Add Time: 1752.401 ms
  Physics Tick: 1803.262 ms (total = 1803.262 ms)
  Physics Tick: 281.146 ms (total = 2084.408 ms)
  Physics Tick: 209.415 ms (total = 2293.823 ms)
  Physics Tick: 212.772 ms (total = 2506.595 ms)
  Physics Tick: 211.445 ms (total = 2718.040 ms)
* Moving objects...
  Move Time: 188.680 ms
  Physics Tick: 304.528 ms (total = 304.528 ms)
  Physics Tick: 774.700 ms (total = 1079.228 ms)
  Physics Tick: 2.476 ms (total = 1081.704 ms)
  Physics Tick: 1.803 ms (total = 1083.507 ms)
  Physics Tick: 1.735 ms (total = 1085.242 ms)
* Removing objects...
  Remove Time: 371.098 ms
  Physics Tick: 392.731 ms (total = 392.731 ms)
  Physics Tick: 3.450 ms (total = 396.181 ms)
  Physics Tick: 1.797 ms (total = 397.978 ms)
  Physics Tick: 2.015 ms (total = 399.993 ms)
  Physics Tick: 1.297 ms (total = 401.290 ms)
* Done
BVH - This PR
* Creating objects...
  Create Time: 2325.229 ms
  Physics Tick: 2328.636 ms (total = 2328.636 ms)
  Physics Tick: 12.296 ms (total = 2340.932 ms)
  Physics Tick: 1.775 ms (total = 2342.707 ms)
  Physics Tick: 1.569 ms (total = 2344.276 ms)
  Physics Tick: 2.075 ms (total = 2346.351 ms)
* Adding objects...
  Add Time: 1235.450 ms
  Physics Tick: 1286.196 ms (total = 1286.196 ms)
  Physics Tick: 282.293 ms (total = 1568.489 ms)
  Physics Tick: 211.901 ms (total = 1780.390 ms)
  Physics Tick: 212.310 ms (total = 1992.700 ms)
  Physics Tick: 216.016 ms (total = 2208.716 ms)
* Moving objects...
  Move Time: 190.718 ms
  Physics Tick: 306.591 ms (total = 306.591 ms)
  Physics Tick: 616.173 ms (total = 922.764 ms)
  Physics Tick: 2.454 ms (total = 925.218 ms)
  Physics Tick: 1.759 ms (total = 926.977 ms)
  Physics Tick: 1.373 ms (total = 928.350 ms)
* Removing objects...
  Remove Time: 368.543 ms
  Physics Tick: 390.314 ms (total = 390.314 ms)
  Physics Tick: 1.936 ms (total = 392.250 ms)
  Physics Tick: 3.294 ms (total = 395.544 ms)
  Physics Tick: 1.561 ms (total = 397.105 ms)
  Physics Tick: 1.795 ms (total = 398.900 ms)
* Done.
Octree
* Creating objects...
  Create Time: 2271.760 ms
  Physics Tick: 2277.866 ms (total = 2277.866 ms)
  Physics Tick: 12.711 ms (total = 2290.577 ms)
  Physics Tick: 2.808 ms (total = 2293.385 ms)
  Physics Tick: 1.337 ms (total = 2294.722 ms)
  Physics Tick: 1.816 ms (total = 2296.538 ms)
* Adding objects...
  Add Time: 1389.967 ms
  Physics Tick: 1441.042 ms (total = 1441.042 ms)
  Physics Tick: 443.207 ms (total = 1884.249 ms)
  Physics Tick: 296.553 ms (total = 2180.802 ms)
  Physics Tick: 293.483 ms (total = 2474.285 ms)
  Physics Tick: 305.871 ms (total = 2780.156 ms)
* Moving objects...
  Move Time: 194.261 ms
  Physics Tick: 614.988 ms (total = 614.988 ms)
  Physics Tick: 291.280 ms (total = 906.268 ms)
  Physics Tick: 3.030 ms (total = 909.298 ms)
  Physics Tick: 2.032 ms (total = 911.330 ms)
  Physics Tick: 2.002 ms (total = 913.332 ms)
* Removing objects...
  Remove Time: 525.276 ms
  Physics Tick: 546.061 ms (total = 546.061 ms)
  Physics Tick: 2.721 ms (total = 548.782 ms)
  Physics Tick: 2.039 ms (total = 550.821 ms)
  Physics Tick: 1.447 ms (total = 552.268 ms)
  Physics Tick: 1.107 ms (total = 553.375 ms)
* Done.

When set_static is called on a newly added object, the forced collision
check in BVH set_pairable was using an empty AABB, which caused
unnecessary collision checks at the origin, then a call to move was
checking again at the right position.

These changes ensure broadphase objects are added to the BVH tree with
proper AABB so collision checks are correctly done right away.

Octree & Basic broadphase trees are not affected by these changes.
@lawnjelly
Copy link
Member

Yup this definitely makes sense. The old way of setting them to a zeroed AABB then setting the AABB in a second step is going to needlessly do extra processing. 👍

@@ -31,7 +31,7 @@
#include "broad_phase_octree.h"
#include "collision_object_sw.h"

BroadPhaseSW::ID BroadPhaseOctree::create(CollisionObjectSW *p_object, int p_subindex) {
BroadPhaseSW::ID BroadPhaseOctree::create(CollisionObjectSW *p_object, int p_subindex, const AABB &p_aabb) {

ID oid = octree.create(p_object, AABB(), p_subindex, false, 1 << p_object->get_type(), 0);
return oid;
Copy link
Member

Choose a reason for hiding this comment

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

Presumably you could do the same for octree? Not that it matters that much if we do get rid of it, but may as well..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried but it actually makes things slower to just initialize the aabb, because the way it works right now, having an empty aabb to start with avoids extra checks in set_static and things get checked later in move.

It should be possible to dig into it and set things properly, but I'm not sure there's going to be a noticeable gain and as you said it's probably not worth it if we start using the bvh instead.

Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Yeah looks good. I haven't tested but I assume Pouley has.

@akien-mga akien-mga merged commit bcbbb6a into godotengine:3.2 Jan 20, 2021
@akien-mga
Copy link
Member

Thanks!

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