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 way to insert components to loaded scenes #4980

Closed
wants to merge 3 commits into from

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jun 10, 2022

This adds the hook field to SceneBundle. The hook is a
SceneHook, which accepts a closure, that is ran once per entity in the
scene. The closure can use the EntityRef argument to check components
the entity has, and use the EntityCommands to add components and
children to the entity.

This code is adapted from https://github.com/nicopap/bevy-scene-hook

Objective

Simplify and streamline adding components to entities within scenes that were just loaded.

Often, when loading objects from a glb file or any scene format, we want to add custom ad-hoc components the scene format doesn't handle.

For example, when loading a 3d model, we might be interested in adding components to handle physics interactions or basic animation. We could also want to add marker components so that it's easier to work with the entities that were added to the world through the scene.

A good example of this pattern can be seen in the warlock's gambit source: https://github.com/team-plover/warlocks-gambit/blob/3f8132fbbd6cce124a1cd102755dad228035b2dc/src/scene.rs

Closes #4933

Solution

The solution involves running a function once per entity added to the scene. This has 3 requirements:

  • We want a way to capture the environment and use it in the function. So that it's possible to use things already in the world in the function (for example, to add Handles from asset resources as components). Here, this is by passing the self parameter to the Hook::hook_entity method. This also allows using closures with captured environment.
  • We want to be able to check which entity we are dealing with, so we need access to the components already attached to it. We have a EntityRef argument for that purpose.
  • We obviously want to be able to add components and children to the entity in question, so we provide a EntityCommands argument as well.

Ideally, we'd rather not want to have to add an individual system per scene. And this is why we use a dynamic object Box<dyn Hook> stored in a component.

This is added as a component in SceneBundle (more precisely, as a field of the SceneHook component), since we want to associate scenes with the hook that run for them.

@Sheepyhead & @Shatur will most likely be interested in this PR.

Drawbacks & Side effects

  • A SceneHook will be added to all SceneBundle
  • as a result, run_hook will run once per added scene, though if the SceneHook is not specified, the default value will cause run_hook to iterate over all entities added to the scene with a no-op, only once.
  • as a result, all added scene will have a SceneHooked marker component added once they are fully loaded, which can have other benefits.

Changelog

  • Added hook field to SceneBundle, to make it easy to add components to entities loaded from any scene format.

@nicopap
Copy link
Contributor Author

nicopap commented Jun 10, 2022

The details of this were discussed in https://discord.com/channels/691052431525675048/983050018179121202 . Furthermore, there is debate on whether to store a Box<dyn Hook> in the SceneHook or a Box<dyn Fn(&EntityRef, &mut EntityCommands) + Send + Sync + 'static>. I personally prefer dyn Hook.

@Shatur
Copy link
Contributor

Shatur commented Jun 10, 2022

Closes #4933.

Furthermore, there is debate on whether to store a Box in the SceneHook or a Box<dyn Fn(&EntityRef, &mut EntityCommands) + Send + Sync + 'static>

My thoughts on it: #4933 (comment)

@Weibye Weibye added C-Feature A new feature, making something new possible A-Scenes Serialized ECS data stored on the disk labels Jun 10, 2022
/// Marker Component for scenes that were hooked.
#[derive(Component)]
#[non_exhaustive]
pub struct SceneHooked;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this component is automatically loaded into all scenes added with SceneBundle, since SceneHook is a component of the SceneBundle.

This adds the `hook` field to `SceneBundle`. The `hook` is a
`SceneHook`, which accepts a closure, that is ran once per entity in the
scene. The closure can use the `EntityRef` argument to check components
the entity has, and use the `EntityCommands` to add components and
children to the entity.

This code is adapted from https://github.com/nicopap/bevy-scene-hook

Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com>
@nicopap
Copy link
Contributor Author

nicopap commented Jun 18, 2022

Note that this PR doesn't include re-running the hook on hot-reloading (which should happen) because I'd rather see #4552 merged first. The scene_spawner code is tricky and I'd rather clean it up before adding that functionality.

@nicopap
Copy link
Contributor Author

nicopap commented Jun 18, 2022

We can also imagine passing special meta-data to the EntityCommands we give the hook, so that we can tie entities inserted by the hook to the scene and reload them on hot-reloading for example.

@mockersf
Copy link
Member

mockersf commented Jun 21, 2022

This feel to me like a workaround to the fact that both scenes and asset loading are not finished in Bevy yet. If Bevy scenes were ready to use, you could specify the components you want to add directly in the scene file. But as they are not ready to use to specify a scene with depending assets, we're falling back on gltf for that. Once the asset loader will be more feature complete, it should be able to specify actions to execute on the assets once they are loaded.

I tend to work around this in two ways. The first one is cleaner and better when the same scene will be spawned multiple times, I tend to do the second one for scenes that are spawned only once as it's somewhat quicker and dirtier. Now that I've written both next to each other, I'm not even sure it's quicker anymore

Modify the scene asset so that every scene spawned from it will have the components I want

load the scene -> modify the scene -> spawn the scene

(#1665 would have provided a nicer api for that without needing an extra resource...)

This will modify the scene asset, changing the inner world of the scene. All SceneBundle spawned with this modified scene will use the modified world

struct MyScene {
    handle: Handle<Scene>,
    modified: bool,
}

fn on_scene_loaded(
    mut commands: Commands,
    mut scenes: ResMut<Assets<Scene>>,
    asset_server: Res<AssetServer>,
    my_scene: Option<ResMut<MyScene>>,
) {
    if let Some(mut my_scene) = my_scene {
        if !my_scene.modified {
            if let Some(mut scene) = scenes.get_mut(&my_scene.handle) {
                // do whatever on scene.world
                my_scene.modified = true;
            }
        }
    } else {
        commands.insert_resource(MyScene {
            handle: asset_server.load("my_gltf_file.gltf#Scene0"),
            modified: false,
        });
    }
}

Modify spawned scenes after they are spawned

load the scene -> spawn the scene -> modify the scene

This works more or less like this PR, but unlike it I need to add one system for each scene type I want to modify. This should handle hot-reloaded scenes though:

#[derive(Component)]
struct NewScene;

fn on_scene_spawned(
    new_scenes: Query<Entity, (Changed<SceneInstance>, With<MyTagForMyScene>)>,
    ready_scenes: Query<(Entity, &SceneInstance), With<NewScene>>,
    scene_spawner: Res<SceneSpawner>,
    mut commands: Commands,
    query: Query<()>,
) {
    for entity in new_scenes.iter() {
        commands.entity(entity).insert(NewScene);
    }
    for (entity, instance) in ready_scenes.iter() {
        if !scene_spawner.instance_is_ready(**instance) {
            continue;
        }
        commands.entity(entity).remove::<NewScene>();
        for entity in scene_spawner.iter_instance_entities(**instance).unwrap() {
            if let Ok(query_result) = query.get(entity) {
                // do whatever I want with my entity that passed the query here
            }
        }
    }
}

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 21, 2022
@mockersf
Copy link
Member

mockersf commented Jun 21, 2022

Having to choose between new, new_fn and new_comp feels pretty bad API wise. We already have a concept of a function that can be applied to entities, and that is a system

@Shatur
Copy link
Contributor

Shatur commented Jun 21, 2022

Having to choose between new, new_fn and new_comp feels pretty bad API wise.

@mockersf Agree.

We already have a concept of a function that can be applied to entities, and that is a system

Yes, but it's very boilerplate. Imagine creating a new system and component for each scene you want to spawn. I described it in #4933. But my suggestion was simpler - no trait, just a a component which calls user-defined function on each entity.

The suggestion was inspired by similar Godot feature: https://docs.godotengine.org/en/stable/tutorials/assets_pipeline/importing_scenes.html#custom-script
But it works like your first snippet. Maybe it makes more sense? Or you don't like the suggested feature in general?

This lowers API surface and complexity.
@nicopap
Copy link
Contributor Author

nicopap commented Jun 21, 2022

I've updated the PR to remove the Hook trait in favor of the Fn trait as suggested by @Shatur . This removes the need for new_fn. I still think new_comp is useful, but well, it seems not popular, so I removed it.

@mockersf
Copy link
Member

But it works like your first snippet. Maybe it makes more sense? Or you don't like the suggested feature in general?

From the objective of the PR, I think it makes more sense to implement something on the scene asset that on the spawned scene.

More generally, I think there are three things here that need improvements:

@Shatur
Copy link
Contributor

Shatur commented Jun 21, 2022

From the objective of the PR, I think it makes more sense to implement something on the scene asset that on the spawned scene.
More generally, I think there are three things here that need improvements:

You are right, it's better to preprocess assets on loading once. I like the direction of #3972. But it will require a huge rework.

Probably it's better to keep this as a third-party plugin until we improve the asset system. Will use the author's plugin https://github.com/nicopap/bevy-scene-hook.

@nicopap
Copy link
Contributor Author

nicopap commented Jun 21, 2022

This is closed in favor of an eventual better integrated solution. Please review the previous discussion for pointers on how it could look like. Until an integrated solution is implemented, you can use https://github.com/nicopap/bevy-scene-hook, which is the exact same code as this PR, but as a third party plugin.

@nicopap nicopap closed this Jun 21, 2022
nicopap added a commit to nicopap/bevy-scene-hook that referenced this pull request Jul 16, 2022
nicopap added a commit to nicopap/bevy-scene-hook that referenced this pull request Jul 30, 2022
@nicopap nicopap deleted the scene-hook branch September 5, 2023 15:44
@nicopap nicopap mentioned this pull request Feb 24, 2024
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 X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add spawn callback for scene entities
5 participants