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

Improve narrow-phase collision physics performance #5789

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Web-eWorks
Copy link
Member

This PR makes the performance of the narrow-phase ("are these objects really colliding?") collision detection pass more betterer, with an average gain for nearly-colliding ship-station pairs on the order of 1.5x-5x. It's slightly slower for interpenetrating objects, as the old code saturated the maximum 8 collision contacts without traversing as many layers of the BVH, but that's not the scenario we should be optimizing for.

I've intentionally structured the commits so that intermediate commits can be built and tested if needed to check performance numbers across multiple systems. This PR strongly needs testing by as many people as possible, as it has a high possibility of completely broken physics if bugs slip through.

Please note when testing that this PR does not improve the quality of the physics - if you were able to fall through the landing pad before, you'll be able to fall through the pad now. It also only affects object-object collision, so ships vs. stations, buildings, and other ships. Collision with terrain is not affected by this PR.

I'm mainly looking for feedback about the performance and any regressions - if you phase entirely through an object that is solid in master, that's a problem. Review is welcome if anyone feels like it, but testing is more important, especially if you want this in the upcoming 2024-03-14 release.

CC @nozmajner @fluffyfreak @JonBooth78(?) for sanity-testing on Windows.

- Alternate BVH construction algorithm using a bin-based surface area heuristic search to find optimal splitting planes.
- Optimized for human-authored models which are highly ordered compared to runtime scene broadphase information which is much more chaotic.
- Binning implementation inspired by https://jacco.ompf2.com/2022/04/21/how-to-build-a-bvh-part-3-quick-builds/
- Provides a general 10-50% improvement in tree SAH vs. existing BVHTree, while ensuring each leaf only contains 1 element.
- Tree SAH on worst-case available inputs is ~15% higher than existing BVHTree.
- Overall memory usage is reduced by 20-25% in worst-case scenes
- Tested binning factors of 8, 12, and 16, with 12 producing the best results on existing input data.
- Useful when doing simultaneous traversal of two BVHs and need to limit ray-tracing to a subtree
- The buffer size wasn't properly set when creating a buffer for incoming draw data >1MB
- Moving the geomFlag variable around reduces the size of a CollisionContact to 96 bytes
- Produces generally faster and higher-quality BVHs
- Query performance can be significantly faster due to better cache occupancy and lower memory consumption
- Implemented side-by-side in this commit for testing purposes
- Traversal over new BVHTree implementation sees significantly better narrow-phase performance (average: 1.5-2x, up to 5x in some cases) due to improved cache occupancy and higher-quality tree layout
- With maximum reported contacts, new implementation can take up to 2x time to saturate allowed contact reports, though overall performance is still significantly faster than existing BVHTree worst-case performance with 0-7 contacts
- Tested against Cydonia and Mars High to provide equal or faster performance in Debug builds and significant performance gains in Release builds
- Implemented side-by-side with old collision algorithm for performance testing
- Fully remove the now-deprecated BVHTree implementation and use new BinnedAreaBVHTree for collision detection code
- Geom::Collide now returns a list of contacts rather than taking a callback to execute on each collision
- This could provide future potential for multithreading collision detection if required
- GeomTree only loads its EdgeTree AABB data from disk - this should be extended in future SGM versions to include the entire triangle/edge BVH structure
- Render debug visualizer for edge and triangle BVH trees for model collision mesh
- Compute and display surface area heuristic for edge/triangle collision mesh BVH
- Add support for displaying dynamic collision mesh BVHs, even though no current models use dynamic collision meshes
- Variable-length stack-allocated arrays are a GCC extension
@fluffyfreak
Copy link
Contributor

Looks good, given it a test and not had any problems so far 👍

@impaktor
Copy link
Member

impaktor commented Mar 7, 2024

given it a test and not had any problems so far

Perhaps not the first time you're back in Pioneer after hiatus, but I imagine it would be a mix of shock of all the new stuff intertwined with familiarity over the stuff being like they used to.

Copy link
Contributor

@Gliese852 Gliese852 left a comment

Choose a reason for hiding this comment

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

It was very interesting to dive into this topic, I hope my nitpicking is useful!

src/Aabb.h Outdated Show resolved Hide resolved
src/collider/BVHTree.cpp Outdated Show resolved Hide resolved
src/collider/BVHTree.cpp Outdated Show resolved Hide resolved
src/collider/BVHTree.cpp Outdated Show resolved Hide resolved
startIdx++;
} else {
endIdx--;
std::swap(keys[startIdx], keys[endIdx]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This swap turns out to be useless if the element at the end is also greater. Generally, if the array is already sorted, it seems to make a lot of unnecessary swaps. Maybe it's possible to check the element in endIdx before swap? (applies to all places where this algorithm is used)

src/collider/Geom.cpp Show resolved Hide resolved
src/collider/GeomTree.cpp Show resolved Hide resolved
src/collider/GeomTree.cpp Show resolved Hide resolved
src/collider/GeomTree.cpp Outdated Show resolved Hide resolved
src/collider/GeomTree.cpp Show resolved Hide resolved
- Accidentally used volume of the contained area instead of surface-area of the AABB bounding box
- An off-by-one error resulted in splitting planes ignoring the last object bin
- Resolve mixed-signs warnings where feasible
- Introduce new vector3::ReciprocalSafe() method to compute 1.0 / vector without introducing NaNs
- Fix initialization order warning in CollisionContact.h
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.

None yet

4 participants