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 spawn callback for scene entities #4933

Closed
Shatur opened this issue Jun 5, 2022 · 16 comments
Closed

Add spawn callback for scene entities #4933

Shatur opened this issue Jun 5, 2022 · 16 comments
Labels
A-Scenes Serialized ECS data stored on the disk C-Feature A new feature, making something new possible

Comments

@Shatur
Copy link
Contributor

Shatur commented Jun 5, 2022

What problem does this solve or what need does it fill?

When spawning a scene, you often need to add additional components. For example, generate a collision from a mesh or assign a collision group.

What solution would you like?

It would be great to be able to specify a closure with EntityCommands
and EntityRef as arguments which will be called for each spawned entity from scene.

So I would suggest adding a bean called SceneHook with a closure that users can assign on scenes. For convenience, I would add it to SceneBundle introduced in #2424.

Example:

commands.spawn_bundle(SceneBundle {
    scene: asset_server.load("models/FlightHelmet/FlightHelmet.gltf#Scene0"),
    hook: SceneHook::new(|entity_ref, entity_commands| {
        if let Some(mesh) = entity_ref.get::<Mesh>() {
            // Entity have mesh, let's assign collision:
            entity_commands.insert(RigidBody::Fixed);
            entity_commands.insert(Collider::from_bevy_mesh(mesh));
        } else if let Some(name) = entity_ref.get::<Name>() {
            // Can insert some components based on name
        }
    }),
    ..Default::default()
});

What alternative(s) have you considered?

But both approaches require adding a separate system for each scene for which you need to add components after spawn. Also it forces user to create a separate component for each scene to somehow differentiate events or added SceneRoots in "post-spawn" systems.

@Shatur Shatur added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jun 5, 2022
@alice-i-cecile alice-i-cecile added A-Scenes Serialized ECS data stored on the disk and removed S-Needs-Triage This issue needs to be labelled labels Jun 5, 2022
@alice-i-cecile
Copy link
Member

Can you give a realistic snippet of what this proposed API would look like? I'm having a bit of trouble piecing together the design and its value.

@MrGVSV
Copy link
Member

MrGVSV commented Jun 5, 2022

Yeah a snippet would be helpful. I don't think it can just take an EntityCommands. You might also need to query or check resources. Maybe even full access to the &mut World.

@Shatur
Copy link
Contributor Author

Shatur commented Jun 5, 2022

Here is how I imagined it:

let map = asset_server.load("my_map.gltf");
commands.spawn_scene(map).with_commands(|entity_commands| {
    entity_commands.insert(RigidBody::Fixed);
});

In heron we have PendingConvexCollision and in rapier we have AsyncSceneCollider for it. In Heron it just copy physics-based components from the root and assign them to all children with meshes, but the list of components is just hardcoded. In Rapier it only assigns colliders, so I was going to submit a PR with behavior similar to heron (the author agreed on it), but then I realized that we probably could solve it here.

Maybe even full access to the &mut World.

Yes, you are right. I think we should have a world and an EntityCommands (to be able to insert things).

@alice-i-cecile
Copy link
Member

I don't know if EntityCommands is the right paradigm here 🤔 Scenes can contain multiple entities. I think you probably want the &mut World version and some helper APIs for common tasks. Or a version of entity commands that applies to all entities in the scene?

@Shatur
Copy link
Contributor Author

Shatur commented Jun 5, 2022

I don't know if EntityCommands is the right paradigm here thinking Scenes can contain multiple entities.

I would expect to have this callback called for each spawned entity. Here is a better example:

let map = asset_server.load("my_map.gltf");
commands.spawn_scene(map).with_commands(|world, entity_commands| {
    if let Some(mesh) = world.entity(entity_commands.id()).get::<Mesh>() {
        // Entity have mesh, let's assign collision:
        entity_commands.insert(RigidBody::Fixed);
        entity_commands.insert(Collider::from_bevy_mesh(mesh));
    }
});

Users will be able to check for Name to do things conditionally, like assigning special components.

Having EntityRef instead of World could be nicer to read, but it could be more limiting 🤔

@nicopap
Copy link
Contributor

nicopap commented Jun 5, 2022

I've came up with something here https://github.com/nicopap/bevy-scene-hook Which seems to be exactly what you are describing. The initial version only provided a EntityCommands handle, but I found it quickly too limiting (even if it was called for each concerned entities), so I added a world module with access to the world (which I use in some private code to do exactly what you are describing). It call some hook_node method once for each entity with a Name field spawned from a specified scene. (I think it would combine amazingly with #2424) I've an update coming up with some improvement. Notably the ability for it to work for more than a single loaded scene.

@nicopap
Copy link
Contributor

nicopap commented Jun 5, 2022

Ok. Having read your issue more carefully. You seem to specifically not want that, but rather want a "lightweight" sort of thing, that doesn't require adding a system to the schedule. bevy-scene-hook does require adding a system to the schedule. But I think it's really not a big loss, since you statically need to know what kind of scenes you are spawning, usually, and you also need to know the hierarchy of your scene (or stuff like names of some entities) to add the correct components to the correct entity (which is something you are overlooking in describing this issue).

@Shatur
Copy link
Contributor Author

Shatur commented Jun 5, 2022

Looks nice. But instead of name I would use EntityRef to be able to access components. To generate collisions from mesh, for example.

@Shatur
Copy link
Contributor Author

Shatur commented Jun 5, 2022

You also need to know the hierarchy of your scene (or stuff like names of some entities) to add the correct components to the correct entity (which is something you are overlooking in describing this issue).

This is exactly the problem that this proposal is supposed to solve :) With EntityRef you will be able to get Name component. And the callback is specific to spawned scene, so you always know your scene.

@Shatur
Copy link
Contributor Author

Shatur commented Jun 6, 2022

I realized that after #2424 scenes won't be a special case anymore. I think it's an awesome change, but it also means that the suggested command API won't fit. So what if we add a component called SceneHook, similar to what https://github.com/nicopap/bevy-scene-hook with the mentioned callback? Example:

commands.spawn_bundle(SceneBundle {
    scene: asset_server.load("models/FlightHelmet/FlightHelmet.gltf#Scene0"),
    ..Default::default()
}).insert(SceneHook::new(|entity_ref, entity_commands| {
    if let Some(mesh) = entity_ref.get::<Mesh>() {
        // Entity have mesh, let's assign collision:
        entity_commands.insert(RigidBody::Fixed);
        entity_commands.insert(Collider::from_bevy_mesh(mesh));
    } else if let Some(name) = entity_ref.get::<Name>() {
        // Can insert some components based on name
    }
}));

And it should be much simpler to implement. What do you think?

@nicopap already reworked his plugin to provide a similar API for 0.7 :)

@MrGVSV
Copy link
Member

MrGVSV commented Jun 6, 2022

So what if we add a component called SceneHook, similar to what https://github.com/nicopap/bevy-scene-hook with the mentioned callback?

Should this be included in SceneBundle directly? Since they're so related at least.

@Shatur
Copy link
Contributor Author

Shatur commented Jun 6, 2022

Should this be included in SceneBundle directly? Since they're so related at least.

I thought of it, but declaring a boxed closure will look like this:

commands.spawn_bundle(SceneBundle {
    scene: asset_server.load("models/FlightHelmet/FlightHelmet.gltf#Scene0"),
    hook: Box::new(|entity_ref, entity_commands| {
        if let Some(mesh) = entity_ref.get::<Mesh>() {
            // Entity have mesh, let's assign collision:
            entity_commands.insert(RigidBody::Fixed);
            entity_commands.insert(Collider::from_bevy_mesh(mesh));
        } else if let Some(name) = entity_ref.get::<Name>() {
            // Can insert some components based on name
        }
    }),
    ..Default::default()
});

Or is it okay?
We could avoid having box by replacing it with fn, but it means that users won't be able to move anything to it.
With SceneHook::new we could use somethings like impl FnOnce(&EntityRef, EntityCommands).

@nicopap
Copy link
Contributor

nicopap commented Jun 6, 2022

@MrGVSV
Copy link
Member

MrGVSV commented Jun 6, 2022

Should this be included in SceneBundle directly? Since they're so related at least.

I thought of it, but declaring a boxed closure will look like this:

commands.spawn_bundle(SceneBundle {

    scene: asset_server.load("models/FlightHelmet/FlightHelmet.gltf#Scene0"),

    hook: Box::new(|entity_ref, entity_commands| {

        if let Some(mesh) = entity_ref.get::<Mesh>() {

            // Entity have mesh, let's assign collision:

            entity_commands.insert(RigidBody::Fixed);

            entity_commands.insert(Collider::from_bevy_mesh(mesh));

        } else if let Some(name) = entity_ref.get::<Name>() {

            // Can insert some components based on name

        }

    }),

    ..Default::default()

});

Or is it okay?

We could avoid having box by replacing it with fn, but it means that users won't be able to move anything to it.

With SceneHook::new we could use somethings like impl FnOnce(&EntityRef, EntityCommands).

I meant more along the lines of including SceneHook as a component of SceneBundle (as opposed to inserting it separately). And give it some sort of default that does nothing.

Like in @nicopap's example.

@Shatur
Copy link
Contributor Author

Shatur commented Jun 6, 2022

@MrGVSV, oh, right, updated the description with your suggestion.

I made a prototype that mirrors this PR's SceneBundle:

I like it, but I would remove the trait and use Box<dyn Fn(&EntityRef, &mut EntityCommands)> as the SceneHook field. Things like:

hook: SceneHook::new_comp::<Name, _>(move |name, cmds| hook(&decks, name.as_str(), cmds)),

and

hook: SceneAssets {
    deck: deck_assets.clone(),
    cards: cards_assets.clone(),
}.into(),

Looks confusing as to me. So I would replace new, new_fn and new_comp with a single new with closure as an argument. But let's see what other people think.

@Shatur
Copy link
Contributor Author

Shatur commented Jun 21, 2022

Closing per #4980 (comment) (better to modify scenes once loaded as discussed in #3972)
Until we have a better asset system we could use https://github.com/nicopap/bevy-scene-hook to solve the problem.

@Shatur Shatur closed this as completed Jun 21, 2022
Shatur added a commit to gardum-game/bevy_rapier that referenced this issue Jul 28, 2022
This will be solved on the Bevy side with asset preprocessing: bevyengine/bevy#4933. In the meanwhile there is a dedicated plugin that can do this and even more: https://github.com/nicopap/bevy-scene-hook.
Shatur added a commit to gardum-game/bevy_rapier that referenced this issue Jul 28, 2022
This will be solved on the Bevy side with asset preprocessing: bevyengine/bevy#4933. In the meanwhile there is a dedicated plugin that can do this and even more: https://github.com/nicopap/bevy-scene-hook.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Scenes Serialized ECS data stored on the disk C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants