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

Use SystemParam for physics hooks #323

Merged
merged 3 commits into from
Feb 4, 2023
Merged

Conversation

bravely-beep
Copy link
Contributor

Use SystemParam instead of WorldQuery to allow more flexible World access by physics hooks

The new API doesn't use a Resource to hold the physics hooks' user data anymore. Instead, the user data is the SystemParam itself. This SystemParam can include access to a Resource if needed, so no functionality is lost here.

PhysicsHooksWithQuery has been renamed BevyPhysicsHooks as it no longer has a query type associated with it - instead, BevyPhysicsHooks and SystemParam are both implemented by the user onto the same type. Now the only real difference between rapier::PhysicsHooks and bevy_rapier::BevyPhysicsHooks is that the latter allows the user to use the "get entity" helper methods on PairFilterContextView and ContactModificationContextView.

Use SystemParam instead of WorldQuery to allow more flexible World access by physics hooks
@bravely-beep
Copy link
Contributor Author

bravely-beep commented Feb 2, 2023

I've just realised this technically removes some functionality; before, you could switch which physics hooks are being used at runtime by replacing the contents of the PhysicsHooksWithQuery resource. Should I rework this PR so that this remains possible?

(Personally I don't see switching the hooks as a particularly useful feature: if changing the functionality of the hooks at runtime is required, the user can simply branch within the hooks' methods. The added API complexity of separating the hooks type from the WorldQuery/SystemParam type doesn't seem worth it to me. But I'm willing to rework it if it's considered an important feature.)

@sebcrozet sebcrozet merged commit be96f14 into dimforge:master Feb 4, 2023
@sebcrozet
Copy link
Member

I really like the ergonomics improvement here. Thanks!
I agree that replacing does not seem worth adding complexity to explicitly allow switching the hooks at runtime. The user could simulate that through their own resource that they would access from the BevyPhysicsHook.

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