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

Add Context to AnyCollider #665

Merged
merged 8 commits into from
Mar 20, 2025
Merged

Add Context to AnyCollider #665

merged 8 commits into from
Mar 20, 2025

Conversation

NiseVoid
Copy link
Contributor

@NiseVoid NiseVoid commented Mar 7, 2025

Objective

Solution

  • Reapply the changes from the previous PR
  • Add a SimpleCollider trait that works for colliders with no context to shorten function signatures

Changelog

  • Custom colliders can now specify a SystemParam via AnyCollider::Context
  • The context is passed to all AnyCollider methods, allowing data from the World to be fetched
  • AnyCollider functions have been suffixed with _with_collider
  • A new SimpleCollider trait has been added to retain the simplified methods for colliders that don't need context

Migration Guide

  • Custom colliders now need to specify a Context SystemParam, if this is unnecessary () should be used
  • When trying to use methods from AnyCollider on an implementation with () context, SimpleCollider should be used instead
  • Methods on AnyCollider have been suffixed with _with_context

Co-authored-by: Gijs de Jong <[email protected]>
Co-authored-by: aecsocket <[email protected]>
@Jondolf Jondolf added C-Enhancement New feature or request A-Collision Relates to the broad phase, narrow phase, colliders, or other collision functionality labels Mar 10, 2025
Copy link
Owner

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

Looks pretty good, mainly a few stylistic nits and documentation suggestions.

Just to be sure, I'll test later today if this has any measurable perf impact in the default case of an empty context, but I assume it should be basically zero-cost.

@Jondolf Jondolf added this to the 0.3 milestone Mar 19, 2025
@Jondolf Jondolf added the C-Breaking-Change This change removes or changes behavior or APIs, requiring users to adapt label Mar 19, 2025
Copy link
Owner

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

Looks good now! Thanks for the patience on this @NiseVoid and @oxkitsune, I've kept the collider context work sitting here for too long.

I made a couple more small changes just to clean things up, and also ran a quick test to see if this has any measurable perf impact. I didn't notice anything, so it should be good to go :)

@Jondolf Jondolf enabled auto-merge (squash) March 20, 2025 16:03
@Jondolf Jondolf merged commit 2c3ba42 into Jondolf:main Mar 20, 2025
5 checks passed
@Jondolf Jondolf mentioned this pull request Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Collision Relates to the broad phase, narrow phase, colliders, or other collision functionality C-Breaking-Change This change removes or changes behavior or APIs, requiring users to adapt C-Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants