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

Rework AsyncCollider and add AsyncSceneCollider #160

Merged
merged 7 commits into from
May 6, 2022

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented May 2, 2022

Closes #144.

I turned AsyncCollider into a struct and moved collision shape into a separate enum to reuse it for AsyncSceneCollider. This also allows to check if mesh is loaded without expanding the enum.

Maybe we should replace bevy_mesh, bevy_mesh_convex_decomposition and bevy_mesh_convex_decomposition_with_params with a single function which accepts AsyncColliderShape (if yes, how would you will rename it)?
This will make things more clear and allow to get rid of matches in init_asyc_* systems.

@Shatur
Copy link
Contributor Author

Shatur commented May 3, 2022

I also think we probably should use Option<AsyncColliderShape> so that the user can specify None for shapes to skip.

@sebcrozet
Copy link
Member

sebcrozet commented May 3, 2022

Maybe we should replace bevy_mesh, bevy_mesh_convex_decomposition and bevy_mesh_convex_decomposition_with_params with a single function which accepts AsyncColliderShape (if yes, how would you will rename it)?

Yeah, that would be more ergonomic. Though in that case, the name AsyncColliderShape doesn’t fit very well because the computation is carried out immediately. Perhaps we could rename it ComputedColliderShape? And then we could rename bevy_mesh to from_bevy_mesh.

I also think we probably should use Option so that the user can specify None for shapes to skip.

Good idea.

@Shatur
Copy link
Contributor Author

Shatur commented May 3, 2022

@sebcrozet done!

Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! It looks nice. All mentions of "recursive children" should be replaced by descendant since that’s what they are called in a tree-like data structure.

This doesn’t handle changes of Transform properly when a collider if down the hierarchy. But fixing this can be considered out of the scope of this PR.

) {
for (entity, async_collider) in async_colliders.iter() {
if scenes.get(&async_collider.handle).is_some() {
for child in get_children_recursively(entity, &children) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of building a LinkedList with all the descendants, I think it would be more efficient and versatile to replace get_children_recursively by a traverse_descendants function that would look like this:

fn traverse_descendants(entity: Entity, children: &Query<&Children>, f:&mut impl FnMut(Entity)) {
    if let Ok(entity_children) = children.get(entity) {
        for child in entity_children.iter() {
            f(child);

            let mut children = get_children_recursively(*child, children);
            all_children.append(&mut children, f);
        }
    }
}

So this for child in get_children_recursively(entity, &children) would simply call traverse_descendants and provide a closure.

.get(name.as_str())
.unwrap_or(&async_collider.shape);
if let Some(shape) = shape {
let mesh = meshes.get(handle).unwrap(); // SAFETY: Mesh is already loaded
Copy link
Member

Choose a reason for hiding this comment

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

As far as I know the SAFETY comments are for unsafe blocks instead of unwraps.

Suggested change
let mesh = meshes.get(handle).unwrap(); // SAFETY: Mesh is already loaded
let mesh = meshes.get(handle).unwrap(); // NOTE: Mesh is already loaded

Some(collider) => {
commands.entity(child).insert(collider);
}
None => panic!("Unable to generate collider from mesh"),
Copy link
Member

Choose a reason for hiding this comment

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

Let’s just print an error instead of panicking, and specify the name of the mesh that failed:

Suggested change
None => panic!("Unable to generate collider from mesh"),
None => error!("AsyncSceneCollider: unable to generate collider from mesh `{}`", name),

@Shatur
Copy link
Contributor Author

Shatur commented May 5, 2022

Thanks! Applied the suggestions and fixed conflicts in changelog with the latest master.
I made error messages without AsyncSceneCollider: for consistency with other error messages in this crate. But feel free to let me know if I should change it.

@Shatur Shatur requested a review from sebcrozet May 5, 2022 17:14
@sebcrozet sebcrozet merged commit e7aa26a into dimforge:master May 6, 2022
@sebcrozet
Copy link
Member

Looks great, thanks!

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.

Add API to generate collision for scene
2 participants