-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
Preserve collisions between inactive entities, add sensor example #266
Conversation
.map(Option::unwrap) | ||
.collect_vec(); | ||
|
||
collisions.extend(existing_unmoving_collisions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't introduce duplicate collisions since broad_collision_pairs
doesn't include pairs of inactive entities, where "inactive" is determined by the same is_changed
checks
Thanks! I'll review and test properly when I have time, hopefully tomorrow or at least by the end of the week. Tests are failing with just |
Looks like my fix has a problem where the collisions aren't pruned if the position of the character is updated directly without using the velocity cursor-hero_LZl9QWoVlg.mp4 |
also ensure that it runs with and without `parallel` feature
Updated the new example to use WASD for velocity-based movement and arrow keys for position-based movement. Code_lIQYSL5VbW.mp4Looks like it's better now :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry I'm properly reviewing this so late.
The approach here should be fine for now, but I think it might be a little hacky. Ideally, the broad phase would just preserve the collisions automatically and the narrow phase wouldn't need so much get_many
. This can be attempted in a future PR though, I'm content getting this merged as soon as possible since it's been sitting for a while.
I'd also like the sensor example to be more minimal and focused; having both velocity and position based movement and other velocity stuff is useful for debugging this issue, but it's not really useful from a user learning perspective. I generally try to roughly follow Bevy's example guidelines for examples showcasing a specific feature, like sensors in this case. But to be fair, some of our examples already don't follow it very well, and it can be improved in future PRs, so it's not a blocker here.
There's some linting and compilation issues that still need to be resolved though. I left review comments for these.
for &(entity1, entity2) in broad_collision_pairs | ||
.0 | ||
.iter() | ||
.chain(existing_stationary_collisions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes ownership issues because existing_stationary_collisions
borrows collisions
but you also insert to it inside the loop.
// we want to preserve collisions between entities that are both stationary | ||
// they are not included in [`BroadCollisionPairs`], so we need to ensure they are still checked | ||
let existing_stationary_collisions = collisions.0.keys().filter(|&&(e1, e2)| { | ||
if let Ok([bundle1, bundle2]) = bodies.get_many([e1, e2]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, the broad phase would automatically preserve these collisions and we wouldn't need to do so much get_many
. This can definitely be attempted in a follow-up PR though
Co-authored-by: Joona Aalto <[email protected]>
Co-authored-by: Joona Aalto <[email protected]>
Co-authored-by: Joona Aalto <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the rest of the errors myself and cleaned up the code a bit to reduce duplication. Merging now! I'll probably try to find a less hacky approach at some point though, and also simplify the sensor example a bit
Objective
Fixes #264
Code_OpvVDqEdIr.mp4
Changelog
Other
5 tests are failing when I run them, but they fail without my changes so idk
Details