Create a DynamicScene from a world and an entity root #6013

mockersf · 2022-09-18T13:44:20Z

Objective

Create a dynamic scene from a world and an entity root.
Sneaked in a few improvements on Scene and DynamicScene to have a more coherent API between the two

Solution

From the root, select all entities that are in the world and descendent of root, and extract all registered components to a dynamic scene

extract all registered components:

DynamicScene::from_world_at_root(
    world,
    Entity::from_bits(0), // Should be a proper way to have an entity here
    world.resource::<AppTypeRegistry>(),
);

extract only a subset of components (here Transform):

let mut atr = AppTypeRegistry::default();
atr.write().register::<Transform>();
DynamicScene::from_world_at_root(
    world,
    Entity::from_bits(0), // Should be a proper way to have an entity here
    &atr,
);

crates/bevy_scene/src/dynamic_scene.rs

alice-i-cecile · 2022-09-18T14:20:36Z

crates/bevy_scene/src/scene.rs


 /// To spawn a scene, you can use either:
 /// * [`SceneSpawner::spawn`](crate::SceneSpawner::spawn)
 /// * adding the [`SceneBundle`](crate::SceneBundle) to an entity
 /// * adding the [`Handle<Scene>`](bevy_asset::Handle) to an entity (the scene will only be
 /// visible if the entity already has [`Transform`](bevy_transform::components::Transform) and
 /// [`GlobalTransform`](bevy_transform::components::GlobalTransform) components)
-#[derive(Debug, TypeUuid)]
+#[derive(Debug, TypeUuid, Deref, DerefMut)]


I'm reluctant to add these derives; they make things more implicit and are likely to break once we need to add more fields.

IMO the Deref / DerefMut impls on Scene should be their own PR so they get appropriate review attention.

'm reluctant to add these derives; they make things more implicit and are likely to break once we need to add more fields.

Then let's remove them later if we need to add more fields? For now they make working with scenes a lot nicer.

Disagreed. Why introduce extra implicitness and potential for future breaking changes, just to save on typing a .0. I wouldn't call this "a lot nicer".

Well then we're back to arguing against the Deref derive that we promotes in Bevy: https://bevyengine.org/news/bevy-0-7/#deref-derefmut-derives

I'm not sure there are plans to add more fields to Scene at the moment?

Hmm, I'm fine to leave it. We should write a manual Deref / DerefMut impl if and when we add more fields then, and call this out in the docs though.

Personally, I don't think the Deref/DerefMut derives are needed. It's probably better for users to explicitly be aware that what they're working with is basically just a plain ol' World. And yeah, it makes future additions to Scenes less problematic (e.g. versioning, etc.).

crates/bevy_scene/src/scene_spawner.rs

inodentry · 2022-09-18T14:51:42Z

crates/bevy_scene/src/dynamic_scene.rs

+    /// Create a new dynamic scene from a given [`World`] at a given [`Entity`].
+    ///
+    /// All descendants entities from the given one will be extracted.
+    pub fn from_world_at_root(


Bikeshed: i think the "at root" naming is confusing. I'd probably have called this from_world_entity_recursive.

totally open to changing the name, even more if we change it to take several entities as input

IMO from_entity_recursive or from_entities_and_children or something. I think the from_world bit is unhelpful in context and can just be dropped.

I actually cast my vote for a simple from_root. Imo it's implicit that we're talking about worlds and entities here. If we want to be a little more specific, I think from_entity is fine too— although, it could be slightly confusing as to whether this recurses into children or not.

Next best would probably be from_entity_recursive.

inodentry · 2022-09-18T14:54:25Z

crates/bevy_scene/src/scene.rs


 /// To spawn a scene, you can use either:
 /// * [`SceneSpawner::spawn`](crate::SceneSpawner::spawn)
 /// * adding the [`SceneBundle`](crate::SceneBundle) to an entity
 /// * adding the [`Handle<Scene>`](bevy_asset::Handle) to an entity (the scene will only be
 /// visible if the entity already has [`Transform`](bevy_transform::components::Transform) and
 /// [`GlobalTransform`](bevy_transform::components::GlobalTransform) components)
-#[derive(Debug, TypeUuid)]
+#[derive(Debug, TypeUuid, Deref, DerefMut)]


Disagreed. Why introduce extra implicitness and potential for future breaking changes, just to save on typing a .0. I wouldn't call this "a lot nicer".

crates/bevy_scene/src/scene_spawner.rs

inodentry · 2022-09-18T15:02:49Z

crates/bevy_scene/src/dynamic_scene.rs

+    /// All descendants entities from the given one will be extracted.
+    pub fn from_world_at_root(
+        world: &World,
+        root: Entity,


Personally, I think this method would have limited practical utility. Who wants to create a scene from just one entity (+ children)? Sure, sometimes you might have such a use case, but it seems needlessly limited. What if you want a second entity? Or a few more?

Why not change this to impl IntoIterator<Item=Entity>, allowing users to provide any number of root entities?

Would add a little extra complexity to the body of the function. Perhaps there should be some sort of deduplication logic to prevent redundant/duplicate entities in the scene. One suggestion might be to use an intermediary HashSet instead of a Vec. Or maybe we could just not do any deduplication (just accumulate all the entities into the scene without regard for duplicates), and document it as a caveat / leave it to the users to make sure they don't have duplicate entities in their list of roots.

It would make this function much more practical and useful.

Would you prefer to be able to append to an existing scene? Or just creating from multiple entities

Well now that you have said it ... i guess both? 😆

I imagine single-entity scenes could be super useful for prefab-like serialization and usage.

But I agree it could also be useful to handle multiple at a time. Perhaps a method for a single root and another for multiple?

MrGVSV

Left some comments. One thing I think we should add, though, is a new example that showcases both creating a scene from a root entity and also filtering by component (the latter could maybe go in the existing scene.rs example if it's not too crowded).

MrGVSV · 2022-10-01T21:48:43Z

crates/bevy_hierarchy/src/hierarchy.rs

@@ -20,6 +20,22 @@ pub struct DespawnChildrenRecursive {
    pub entity: Entity,
 }

+/// List all descendants of a given entity


Could you expand on this? Specifically, is the returned Vec<Entity> a depth-first traversal down the hierarchy or breadth-first?

MrGVSV · 2022-10-01T21:50:28Z

crates/bevy_scene/src/dynamic_scene.rs

@@ -76,17 +78,60 @@ impl DynamicScene {
        scene
    }

+    /// Create a new dynamic scene from a given [`World`] at a given [`Entity`].


Nit:

Suggested change

/// Create a new dynamic scene from a given [`World`] at a given [`Entity`].

/// Create a new dynamic scene from a given [`World`] at a given [`Entity`] root.

MrGVSV · 2022-10-01T21:54:35Z

crates/bevy_scene/src/dynamic_scene.rs

+    /// Create a new dynamic scene from a given [`World`] at a given [`Entity`].
+    ///
+    /// All descendants entities from the given one will be extracted.
+    pub fn from_world_at_root(


I actually cast my vote for a simple from_root. Imo it's implicit that we're talking about worlds and entities here. If we want to be a little more specific, I think from_entity is fine too— although, it could be slightly confusing as to whether this recurses into children or not.

Next best would probably be from_entity_recursive.

MrGVSV · 2022-10-01T21:57:36Z

crates/bevy_scene/src/dynamic_scene.rs

+    /// All descendants entities from the given one will be extracted.
+    pub fn from_world_at_root(
+        world: &World,
+        root: Entity,


I imagine single-entity scenes could be super useful for prefab-like serialization and usage.

But I agree it could also be useful to handle multiple at a time. Perhaps a method for a single root and another for multiple?

MrGVSV · 2022-10-01T22:07:28Z

crates/bevy_scene/src/dynamic_scene.rs

+        let mut entities_of_interest = list_all_descendants(world, root);
+        entities_of_interest.push(root);


Nit: This is more of a stylistic thing I suppose, but should the root come first in this list?

Suggested change

let mut entities_of_interest = list_all_descendants(world, root);

entities_of_interest.push(root);

let mut entities_of_interest =

std::iter::once(root).chain(list_all_descendants(world, root));

MrGVSV · 2022-10-01T22:13:24Z

crates/bevy_scene/src/dynamic_scene.rs

    /// Write the dynamic entities and their corresponding components to the given world.
    ///
    /// This method will return a [`SceneSpawnError`] if a type either is not registered
    /// or doesn't reflect the [`Component`](bevy_ecs::component::Component) trait.
-    pub fn write_to_world(
+    pub fn write_to_world_with(


I think this should be a separate method so users that want to extract everything can don't need to add the extra world.resource::<AppTypeRegistry>() call.

MrGVSV · 2022-10-01T22:13:55Z

crates/bevy_scene/src/dynamic_scene.rs

    /// Write the dynamic entities and their corresponding components to the given world.
    ///
    /// This method will return a [`SceneSpawnError`] if a type either is not registered
    /// or doesn't reflect the [`Component`](bevy_ecs::component::Component) trait.


Documentation should include information about the registry filter.

MrGVSV · 2022-10-01T22:18:23Z

crates/bevy_scene/src/scene.rs


 /// To spawn a scene, you can use either:
 /// * [`SceneSpawner::spawn`](crate::SceneSpawner::spawn)
 /// * adding the [`SceneBundle`](crate::SceneBundle) to an entity
 /// * adding the [`Handle<Scene>`](bevy_asset::Handle) to an entity (the scene will only be
 /// visible if the entity already has [`Transform`](bevy_transform::components::Transform) and
 /// [`GlobalTransform`](bevy_transform::components::GlobalTransform) components)
-#[derive(Debug, TypeUuid)]
+#[derive(Debug, TypeUuid, Deref, DerefMut)]


Personally, I don't think the Deref/DerefMut derives are needed. It's probably better for users to explicitly be aware that what they're working with is basically just a plain ol' World. And yeah, it makes future additions to Scenes less problematic (e.g. versioning, etc.).

alice-i-cecile · 2022-10-10T22:51:03Z

Closed in favor of #6227.

# Objective - make it easier to build dynamic scenes ## Solution - add a builder to create a dynamic scene from a world. it can extract an entity or an iterator of entities - alternative to #6013, leaving the "hierarchy iteration" part to #6185 which does it better - alternative to #6004 - using a builder makes it easier to chain several extractions

# Objective - make it easier to build dynamic scenes ## Solution - add a builder to create a dynamic scene from a world. it can extract an entity or an iterator of entities - alternative to bevyengine#6013, leaving the "hierarchy iteration" part to bevyengine#6185 which does it better - alternative to bevyengine#6004 - using a builder makes it easier to chain several extractions

mockersf added 3 commits September 18, 2022 15:05

uniform api between Scene and DynamicScene

9e43c0d

it's prettier to use

1938d01

create a dynamic scene from a world and a root

daaddd6

mockersf mentioned this pull request Sep 18, 2022

Create a Dynamic Scene From a Query Filter #6004

Closed

mockersf added the A-Scenes Serialized ECS data stored on the disk label Sep 18, 2022

alice-i-cecile added the C-Feature A new feature, making something new possible label Sep 18, 2022

alice-i-cecile reviewed Sep 18, 2022

View reviewed changes

crates/bevy_scene/src/dynamic_scene.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Sep 18, 2022

View reviewed changes

crates/bevy_scene/src/dynamic_scene.rs Outdated Show resolved Hide resolved

better doc

f0554e9

alice-i-cecile reviewed Sep 18, 2022

View reviewed changes

crates/bevy_scene/src/dynamic_scene.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Sep 18, 2022

View reviewed changes

crates/bevy_scene/src/dynamic_scene.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Sep 18, 2022

View reviewed changes

crates/bevy_scene/src/dynamic_scene.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Sep 18, 2022

View reviewed changes

crates/bevy_scene/src/scene_spawner.rs Show resolved Hide resolved

mockersf added 3 commits September 18, 2022 16:28

move descendants listing to bevy_hierarchy

793f693

only filter once

70347fb

iterate on entities instead of on archetypes

8307f14

inodentry reviewed Sep 18, 2022

View reviewed changes

MrGVSV reviewed Oct 1, 2022

View reviewed changes

mockersf mentioned this pull request Oct 10, 2022

[Merged by Bors] - dynamic scene builder #6227

Closed

alice-i-cecile closed this Oct 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a DynamicScene from a world and an entity root #6013

Create a DynamicScene from a world and an entity root #6013

mockersf commented Sep 18, 2022 •

edited

Loading

alice-i-cecile Sep 18, 2022

alice-i-cecile Sep 18, 2022

mockersf Sep 18, 2022

inodentry Sep 18, 2022

mockersf Sep 18, 2022 •

edited

Loading

alice-i-cecile Sep 18, 2022

MrGVSV Oct 1, 2022

inodentry Sep 18, 2022

mockersf Sep 18, 2022

alice-i-cecile Sep 18, 2022

MrGVSV Oct 1, 2022

inodentry Sep 18, 2022

inodentry Sep 18, 2022

mockersf Sep 18, 2022

inodentry Sep 18, 2022

MrGVSV Oct 1, 2022

MrGVSV left a comment

MrGVSV Oct 1, 2022

MrGVSV Oct 1, 2022

MrGVSV Oct 1, 2022

MrGVSV Oct 1, 2022

MrGVSV Oct 1, 2022

MrGVSV Oct 1, 2022

MrGVSV Oct 1, 2022

MrGVSV Oct 1, 2022

alice-i-cecile commented Oct 10, 2022

	/// Create a new dynamic scene from a given [`World`] at a given [`Entity`].
	/// Create a new dynamic scene from a given [`World`] at a given [`Entity`] root.

		let mut entities_of_interest = list_all_descendants(world, root);
		entities_of_interest.push(root);

Create a DynamicScene from a world and an entity root #6013

Create a DynamicScene from a world and an entity root #6013

Conversation

mockersf commented Sep 18, 2022 • edited Loading

Objective

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockersf Sep 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrGVSV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Oct 10, 2022

mockersf commented Sep 18, 2022 •

edited

Loading

mockersf Sep 18, 2022 •

edited

Loading