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

Create a Dynamic Scene From a Query Filter #6004

Closed

Conversation

Carter0
Copy link
Contributor

@Carter0 Carter0 commented Sep 17, 2022

Objective

This PR copies over the scene_from_query_components function from iyes_scene_tools. I am not the author of that code I am merely copying it over.

This allows users to do something like...

let my_scene = DynamicScene::from_query_filter::<(With<ComponentA>, Without<ComponentB>)>(
    &mut world,
    &type_registry,
);

This is my first Bevy PR!

Cargo.toml Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Enhancement A new feature A-Scenes Serialized ECS data stored on the disk and removed A-Assets Load files from disk to use for things like images, models, and sounds labels Sep 17, 2022
/// filter provided. All components that impl `Reflect` will be included.
pub fn from_query_filter<F>(world: &mut World) -> Self
where
F: ReadOnlyWorldQuery + 'static,
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting bound; we don't require ReadOnlyWorldQuery on the F in Query<Q, F>. Perhaps we should though?

@mockersf
Copy link
Member

I don't think creating a scene from a query filter is a good api. I would prefer to have an api like in #6013 to extract part of the world from a scene and a root, mainly because:

  • the query filter is not hierarchy aware
  • the query filter is not reflect aware

@Carter0
Copy link
Contributor Author

Carter0 commented Sep 18, 2022

I don't think creating a scene from a query filter is a good api. I would prefer to have an api like in #6013 to extract part of the world from a scene and a root, mainly because:

  • the query filter is not hierarchy aware
  • the query filter is not reflect aware

I don't know that much about the implementation issues you are describing. But I think this could be a useful feature. I could imagine myself making a savable component or something similar. Then I would just update my save file using a filter for anything with that component.

@Carter0 Carter0 marked this pull request as ready for review September 18, 2022 19:06
@alice-i-cecile
Copy link
Member

I could imagine myself making a savable component or something similar. Then I would just update my save file using a filter for anything with that component.

So, this seems like a reasonable solution! There are two problems here, that Francois is attempting to point out:

  1. There's no guarantee that all of the components on these entities are reflected correctly and stored in the type registry.
  2. There's no guarantee that our hierarchies won't break; we may get children but not parents or vice-versa.

I think the first needs to be fixed in some form, but the second can be cautioned against.

let mut query = world.query_filtered::<Entity, F>();

let type_registry = world
.get_resource::<AppTypeRegistry>()
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should probably force users to pass in this type explicitly, in order to clarify which type registry they want to use.

Copy link
Contributor Author

@Carter0 Carter0 Sep 18, 2022

Choose a reason for hiding this comment

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

Wouldn't there just be one type registry per world? Or per app? How could there be multiple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see whats going on.

In order for a component to be saved to a scene, it needs to be registered in the type registry. If a user tried to use this method without registering their types first, then the scene would not be created with that component on it.

@mockersf
Copy link
Member

mockersf commented Sep 18, 2022

The type registry works actually as a filter of which component you can extract, but unlike a QueryFilter all the components in one can be extracted. With a QueryFilter you can ask for components that are impossible to extract.

There is one built in in the main world with all registered components, but building one with just the components is easy, see the second example in #6013.

The API proposed in this PR is pretty much equivalent to https://docs.rs/bevy/latest/bevy/prelude/struct.DynamicScene.html#method.from_world. Rather than specify a QueryFilter with your components, you setup a type registry with your components.

This proposed API:

DynamicScene::from_query_filter::<(With<ComponentA>, With<ComponentB>)>(&mut world);

What already exist in Bevy:

let mut atr = AppTypeRegistry::default();
atr.write().register::<ComponentA>();
atr.write().register::<ComponentB>();
DynamicScene::from_world(&world);

The current API is a little more verbose, but you can't specify an invalid filter with it.

@mockersf
Copy link
Member

mockersf commented Sep 18, 2022

Never mind my comment above, I rechecked the doc you wrote. Your PR extracts all components, but only for entities in the filter 👍

Then adding a type registry would allow you to chose which components to extract

@Carter0 Carter0 closed this Sep 18, 2022
@Carter0 Carter0 reopened this Sep 18, 2022
@Carter0 Carter0 changed the title Adding scene_from_query_filter to bevy Create a Dynamic Scene From a Query Filter Sep 18, 2022
@alice-i-cecile
Copy link
Member

Closed in favor of #6227.

bors bot pushed a commit that referenced this pull request Oct 12, 2022
# 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
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
# 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
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# 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
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
# 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
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# 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
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-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants