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 capacity handling for ColliderSet, RigidBodySet. #726

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

waywardmonkeys
Copy link
Contributor

These allow an application to reduce the cost of reallocation when they know that a large number of colliders or rigid bodies will be created.

These allow an application to reduce the cost of reallocation when
they know that a large number of colliders or rigid bodies will
be created.
@waywardmonkeys
Copy link
Contributor Author

We may at times create a scene with a large number of colliders and prefer that doing so be fast. (In our case, we're not running physics or rigid bodies. We're also able to disable the colliders as we don't need the full collision detection, just answers to queries.)

For a silly test case of 8,000,000 (200x200x200) things (using many_static.rs as a starter), this reduced the time for populating the sets from 1.3s to 1.0s.

There's a number of questions that could be considered here beyond just these changes:

  • Should other sets get this sort of thing?
  • Should there be a way to set the capacity for the removed_colliders on ColliderSet?
  • Should we give access to methods for capacity, reserve, shrink_to?

@waywardmonkeys
Copy link
Contributor Author

@Vrixyz @sebcrozet Just checking in on this...

@@ -5,6 +5,10 @@
- The region key has been replaced by an i64 in the f64 version of rapier, increasing the range before panics occur.
- Fix `BroadphaseMultiSap` not being able to serialize correctly with serde_json.

### Added

- `RigidBodySet` and `ColliderSet` have a new constructor `with_capacity`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this point to this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally yes, but it's rare to have the PR link, I'm adding it arbitrarily on bigger/breaking/difficult PRs

@Vrixyz
Copy link
Contributor

Vrixyz commented Sep 16, 2024

I'll merge this as-is ; as for your questions,

Should other sets get this sort of thing?

A good idea.

Should there be a way to set the capacity for the removed_colliders on ColliderSet?

If there's a real use case for it, I'd approve it, but probably pass in the exact values of each properties, so each properties can be tweaked ?

Should we give access to methods for capacity, reserve, shrink_to?

Probably a good idea, but I'd prefer a real use case too.

@waywardmonkeys
Copy link
Contributor Author

Should we give access to methods for capacity, reserve, shrink_to?

Probably a good idea, but I'd prefer a real use case too.

My concern here is that you add a ton of things, so the storage gets really big, but after that, there's only a few modified per frame, so most of that storage is wasted. This isn't new with my changes, but just thinking about the overall impact of everything in this area...

@Vrixyz Vrixyz merged commit e7e196d into dimforge:master Sep 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants