bevy
bevy copied to clipboard
bevy_scene: Add `SceneFilter`
Objective
Currently, DynamicScene
s extract all components listed in the given (or the world's) type registry. This acts as a quasi-filter of sorts. However, it can be troublesome to use effectively and lacks decent control.
For example, say you need to serialize only the following component over the network:
#[derive(Reflect, Component, Default)]
#[reflect(Component)]
struct NPC {
name: Option<String>
}
To do this, you'd need to:
- Create a new
AppTypeRegistry
- Register
NPC
- Register
Option<String>
If we skip Step 3, then the entire scene might fail to serialize as Option<String>
requires registration.
Not only is this annoying and easy to forget, but it can leave users with an impossible task: serializing a third-party type that contains private types.
Generally, the third-party crate will register their private types within a plugin so the user doesn't need to do it themselves. However, this means we are now unable to serialize just that type— we're forced to allow everything!
Solution
Add the SceneFilter
enum for filtering components to extract.
This filter can be used to optionally allow or deny entire sets of components. With the DynamicSceneBuilder
, users have more control over how their DynamicScene
s are built.
To only serialize a subset of components, use the allow
method:
let scene = builder
.allow::<ComponentA>()
.allow::<ComponentB>()
.extract_entity(entity)
.build();
To serialize everything but a subset of components, use the deny
method:
let scene = builder
.deny::<ComponentA>()
.deny::<ComponentB>()
.extract_entity(entity)
.build();
Or create a custom filter:
let components = HashSet::from([type_id]);
let filter = SceneFilter::Allowlist(components);
// let filter = SceneFilter::Denylist(components);
let scene = builder
.with_filter(Some(filter))
.extract_entity(entity)
.build();
Open Questions
- [x] ~~
allow
anddeny
are mutually exclusive. Currently, they overwrite each other. Should this instead be a panic?~~ Took @soqb's suggestion and made it so that the opposing method simply removes that type from the list. - [x] ~~
DynamicSceneBuilder
extracts entity data as soon asextract_entity
/extract_entities
is called. Should this behavior instead be moved to thebuild
method to prevent ordering mixups (e.g..allow::<Foo>().extract_entity(entity)
vs.extract_entity(entity).allow::<Foo>()
)? The tradeoff would be iterating over the given entities twice: once at extraction and again at build.~~ Based on the feedback from @Testare it sounds like it might be better to just keep the current functionality (if anything we can open a separate PR that adds deferred methods for extraction, so the choice/performance hit is up to the user).- [ ] An alternative might be to remove the filter from
DynamicSceneBuilder
and have it as a separate parameter to the extraction methods (either in the existing ones or as addedextract_entity_with_filter
-type methods). Is this preferable?
- [ ] An alternative might be to remove the filter from
- [x] ~~Should we include constructors that include common types to allow/deny? For example, a
SceneFilter::standard_allowlist
that includes things likeParent
andChildren
?~~ Consensus suggests we should. I may split this out into a followup PR, though. - [x] ~~Should we add the ability to remove types from the filter regardless of whether an allowlist or denylist (e.g.
filter.remove::<Foo>()
)?~~ See the first list item - [x] ~~Should
SceneFilter
be an enum? Would it make more sense as a struct that contains anis_denylist
boolean?~~ With the addedSceneFilter::None
state (replacing the need to wrap in anOption
or rely on an emptyDenylist
), it seems an enum is better suited now - [x] ~~Bikeshed: Do we like the naming convention? Should we instead use
include
/exclude
terminology?~~ Sounds like we're sticking withallow
/deny
! - [x] ~~Does this feature need a new example? Do we simply include it in the existing one (maybe even as a comment?)? Should this be done in a followup PR instead?~~ Example will be added in a followup PR
Changelog
- Added the
SceneFilter
enum for filtering components when building aDynamicScene
- Added methods:
-
DynamicSceneBuilder::with_filter
-
DynamicSceneBuilder::allow
-
DynamicSceneBuilder::deny
-
- Removed methods:
-
DynamicSceneBuilder::from_world_with_type_registry
-
-
DynamicScene::from_scene
andDynamicScene::from_world
no longer require anAppTypeRegistry
reference
Migration Guide
-
DynamicScene::from_scene
andDynamicScene::from_world
no longer require anAppTypeRegistry
reference:// OLD let registry = world.resource::<AppTypeRegistry>(); let dynamic_scene = DynamicScene::from_world(&world, registry); // let dynamic_scene = DynamicScene::from_scene(&scene, registry); // NEW let dynamic_scene = DynamicScene::from_world(&world); // let dynamic_scene = DynamicScene::from_scene(&scene);
-
Removed
DynamicSceneBuilder::from_world_with_type_registry
. Now the registry is automatically taken from the given world:// OLD let registry = world.resource::<AppTypeRegistry>(); let builder = DynamicSceneBuilder::from_world_with_type_registry(&world, registry); // NEW let builder = DynamicSceneBuilder::from_world(&world);
I think I'm going to switch SceneFilter::Allowlist
and SceneFilter::Denylist
to take their own structs instead of HashSet<TypeId>
directly. My thought being that we may want to change the implementation or even add additional filter options in the future. It's probably best to close off that part of the API and force users to go through a more controlled FilterList
struct.
I think I'm going to switch
SceneFilter::Allowlist
andSceneFilter::Denylist
to take their own structs instead ofHashSet<TypeId>
directly. My thought being that we may want to change the implementation or even add additional filter options in the future. It's probably best to close off that part of the API and force users to go through a more controlledFilterList
struct.
On second thought, I don't think we really gain much by simply wrapping HashSet<TypeId>
. My original plan was to allow us to do things like this in the future:
struct Filter {
components: HashSet<TypeId>,
resources: HashSet<TypeId>,
// other metadata...
}
But it probably makes more sense to move that kind of metadata/logic to the container types:
struct DynamicSceneBuilder {
// ...
component_filter: SceneFilter,
resource_filter: SceneFilter,
// other metadata...
}
So I think it should be okay to keep this as a plain HashSet<TypeId>
for now. Any other thoughts?
This is just my opinion as an user: I think this solution is perfectly clear and overall a better approach than customizing the AppTypeRegistry directly, which I found confusing in my project while trying out scenes for the first time.
Regarding the second open question, I personally think the ordering mixup is a real footgun especially for newbies, who might be inclined to think there's no need for a particular order considering we're using a builder here and not a concrete DynamicScene. I can't seem to understand the point about the tradeoff though. If the extraction is moved on build
, why does the entity iterator need to be iterated twice? I'm assuming the extract_entity
method would simply save the iterator passed as argument on the DynamicSceneBuilder struct, then actually iter it on build
. What am I missing?
Regarding the third question, I believe there are a few components which are currently included in de/serialization but shouldn't. I'm thinking of ~~GlobalTransform~~ (I saw on Discord that GlobalTransforms aren't necessarily generated), ComputedVisibility, CalculatedSize and maybe others I don't know about. In my personal project, I serialized my scene first and only after checking the result I've noticed the presence of those components, which I then removed manually with the type registry. I think a specific constructor as you said would be a nice and easy solution, although I wonder what's the most common use case here: if denying those types is more common, wouldn't it be better to include them in the default deny list, whereas having a separate constructor (something like SceneFilter::blank()
? I'm bad with names) for special use cases? This might also allow adding future new types in this default list so that upgrading Bevy on a personal project wouldn't cause new types popping up when serializing scenes. Even for special use cases, it might be just better to still use the common deny list and specifically allow the type the user needs (for example, if the default list has GlobalTransform
but the user needs to serialize it, I imagine he could just do SceneFilter::default().allow::<GlobalTransform>()
).
I hope I helped. Thank you very much for your work!
I can't seem to understand the point about the tradeoff though. If the extraction is moved on
build
, why does the entity iterator need to be iterated twice? I'm assuming theextract_entity
method would simply save the iterator passed as argument on the DynamicSceneBuilder struct, then actually iter it onbuild
. What am I missing?
The main issue is that we're not able to store the iterator without running into lifetime issues. This makes sense if we consider the fact that a query's elements may or may not change between frames (i.e. entities added or removed). So the solution would be to consume the iterator and get the entities right away. Then in build
, we'd need to iterate over all of the stored entities to actually build the scene. This means we essentially iterate through the entities twice.
This might not be awful (relevant video), but it's worth consideration— especially if we expect a lot of entities.
I wonder what's the most common use case here: if denying those types is more common, wouldn't it be better to include them in the default deny list, whereas having a separate constructor (something like
SceneFilter::blank()
? I'm bad with names) for special use cases? This might also allow adding future new types in this default list so that upgrading Bevy on a personal project wouldn't cause new types popping up when serializing scenes. Even for special use cases, it might be just better to still use the common deny list and specifically allow the type the user needs (for example, if the default list hasGlobalTransform
but the user needs to serialize it, I imagine he could just doSceneFilter::default().allow::<GlobalTransform>()
).
Yeah that might make more sense and would match TypeRegistry
(whose Default
impl includes the standard registrations). Thanks for the suggestion!
Here's some random guy on the internet's opinions, as requested!
Bikeshed: Allow/Deny API names
I think allow
/deny
are perfect names for this. Saves 43% more electrons than include/exclude for the same clarity!
Move extract_entity behavior to build()
I actually think that allowing this ordering to have meaning is a feature not a bug. There could be times when within the same scene you would like components serialized on some entities but not others.
Coming up with an example on the spot, supposed there was a card game that ran over a network, where the cards all have a physical position in space, including on the table and in the player hands. You want to send that scene over the internet, but you don't want the players to know what cards are in the opponents hand or facedown on the table.
So when you send the scene to a player, you could do something like
.extract_entity(face_up_card)
.deny::<CardFace>()
.extract_entitiy(other_players_hand)
.extract_entitiy(face_down_card)
You can even use this to create special extraction methods that might use different filters than the rest of the scene.
fn extract_ui_entities(builder: DynamicSceneBuilder) -> DynamicSceneBuidler {
let old_filter = builder.filter()
return builder.with_filter(specialUiSceneFilter())
.extract_entity(..)
.with_filter(old_filter)
}
Default filters
I also like the idea of having SceneBuilder have a default filter. I think a default filter would probaby be closer to DenyList behavior, where all user-defined components would be allowed by default.
You'd have to make sure that the components filtered by default are okay with whatever feature flags are enabled though. Probably out of scope for now, but perhaps using an attribute on components to have them filtered by default could be a future improvement.
That said, the big open question then becomes what is expected behavior if somebody then uses allow()
or deny()
on the builder? Does it modify the default list, or define an entirely new list?
I think modifying the default list is ideal behavior, but add allow_all()
and deny_all()
methods to override it.
If CalculatedSize
is filtered out by default, adding .deny::<ComponentA>()
should still filter out CalculatedSize. If you only want ComponentA, you wouldn't be able to just do .allow::<ComponentA>()
, but it still makes sense to do .deny_all().allow::<ComponentA>()
to indicate that is the only component you want.
If we don't decide that entities should be extracted on build (previous section), we can add a with_default_filter()
method to to the builder to bring back the default filter between entity extractions.
If we do implement a default list SceneFilter::None
should probably become SceneFilter::Default
or go away entirely. Keeping it as Default prevents us from copying the default list each time SceneBuilder is initialized if they don't actually modify the filter or if they end up replacing it. The behavior of SceneFilter::None is easily matched with an empty DenyList.
Scene Filter API improvements
Even without a default component list, I think adding allow_all()
/deny_all()
methods to SceneFilter as constructors could improve the API. It can be a little confusing that creating an empty SceneFilter::DenyList
is actually equivalent to allow_all()
, and an empty SceneFilter::AllowList
is actually equivalent to deny_all()
.
Example:
let scene = builder
.with_filter(SceneFilter::allow_all()
.deny::<ComponentA>())
.extract_entity(entity)
.build()
We could add except
API to the filter in this case, since allow_all().except::<ComponentA>()
sounds more natural. But then we have things like SceneFilter::default().except::<ComponentA>()
where its behavior is ambiguous. Should it panic? I suppose we could check if the item is in the list or not and toggle it, but that increases latency and I'm not sure if it's that much better than just staying with allow_all().deny::<ComponentA>()
.
Move extract_entity behavior to build()
I actually think that allowing this ordering to have meaning is a feature not a bug. There could be times when within the same scene you would like components serialized on some entities but not others.
Coming up with an example on the spot, supposed there was a card game that ran over a network, where the cards all have a physical position in space, including on the table and in the player hands. You want to send that scene over the internet, but you don't want the players to know what cards are in the opponents hand or facedown on the table.
That's actually a really good point! We may want to change the filter between groups of entities, which makes this behavior actually useful.
I think we could support both by adding a extract_entity_deferred
method which uses the last filter (or maybe just a clone of the current filter when called). But that can actually be added in a separate PR.
Scene Filter API improvements
Even without a default component list, I think adding
allow_all()
/deny_all()
methods to SceneFilter as constructors could improve the API. It can be a little confusing that creating an emptySceneFilter::DenyList
is actually equivalent toallow_all()
, and an emptySceneFilter::AllowList
is actually equivalent todeny_all()
.
Yeah allow_all()
/deny_all()
methods definitely sound a lot nicer for the end user (especially considering the possible confusion you mentioned).
We could add
except
API to the filter in this case, sinceallow_all().except::<ComponentA>()
sounds more natural.
I think I'd prefer to just keep allow
/deny
. Like you pointed out, with except
you need to know the original state of the filter which adds confusion/complexity imo:
fn apply_filters(scene_builder: &mut DynamicSceneBuilder) {
// `except`:
// If `SceneFilter::Allowlist` -> denies `MyComponent`
// If `SceneFilter::Denylist` -> allows `MyComponent`
scene_builder.except::<MyComponent>();
// `deny`:
// Always denies `MyComponent`
scene_builder.deny::<MyComponent>();
}
I think we'll leave off the default type filtering for now. A lot of the types we might want to filter are in crates that aren't dependencies of bevy_scene
. We may have to add features to get a decently sized set of types, and that's probably best in its own PR.
Love these improvement ! Just my 2 cents
- Having had to deal with the
AppTypeRegistry
a lot recently, I love not having to pass the World around, which made things often more complicated than need be. -
allow/deny
seem perfectly clear - adding a separate example for filtering makes the most sense to me: I find having examples for specific features more clear than putting too much information into a single one. It also makes sense for users to discover the features in an approachable way.
- adding a separate example for filtering makes the most sense to me: I find having examples for specific features more clear than putting too much information into a single one. It also makes sense for users to discover the features in an approachable way.
Thanks for the feedback! This makes sense. I'll create a followup PR with a proper example once this gets merged.
@MrGVSV are you able to rebase and complete this? I'd like to move this forward, but the conflicts are non-trivial and the suggestions are good :)
@MrGVSV are you able to rebase and complete this? I'd like to move this forward, but the conflicts are non-trivial and the suggestions are good :)
Sure thing! I'll try to do that either tonight or tomorrow. Just need to rebase and apply the changes discussed in #scenes-dev.
Rebased and updated to include changes from #6846 that allowed resources to be included in scenes. This meant the following methods were added:
-
DynamicSceneBuilder::with_resource_filter
-
DynamicSceneBuilder::allow_resource
-
DynamicSceneBuilder::deny_resource
-
DynamicSceneBuilder::allow_all_resources
-
DynamicSceneBuilder::deny_all_resources
These were made separate from the component methods so users could have much more control over the filtering between resources and components (e.g. denylist for resources and allowlist for components) and to improve the overall clarity of what each method does.
I also updated the PR description to reflect these changes and to include a couple of followup tasks.
@MrGVSV can you update the docs this weekend? If so, I'll merge this Monday :)
It feels like this issue would be fixed by https://github.com/bevyengine/bevy/pull/5781 and adding methods to create a register from an existing one. Would this PR still has an advantage if that were done, or would it be adding two ways to do the same thing?
It feels like this issue would be fixed by #5781 and adding methods to create a register from an existing one. Would this PR still has an advantage if that were done, or would it be adding two ways to do the same thing?
I think that's a good callout. I think we definitely could continue using AppTypeRegistry
if we relied on something like #5781, however, there are a few arguments against it:
- There's a bit more overhead that would come with a registry as a filter. We really only care that a type is allowed or denied. But creating a temporary registry involves generating lots of
TypeRegistration
structs, which will come with more of a runtime cost using #5781 (unless the created registry filter is stored in a resource likeAppTypeRegistry
). - It could make dynamic filtering a little more complicated. Rather than simply needing a
TypeId
, we would either require the concrete type or the type-erasedTypeRegistration
. - We'd still need to rely on the world's
AppTypeRegistry
since there's a chance the necessary type data is not auto-registered (i.e. not marked#[reflect(Component)]
or#[reflect(Resource)]
). So in this way, it doesn't really matter if the filter is a registry or not. - The ergonomics could take a hit. If you had a query of known components and wanted all of them except a select few, you wouldn't be able to create a denylist. Instead, you'd have to create a new registry with all the other components.
- Similarly, we wouldn't be able to define default denylists (e.g. denying all components and resources that shouldn't be serialized).
- Also along those line, methods for filtering are limited by the methods on the registry. If we wanted to create a new method, it would probably have to live on
DynamicSceneBuilder
(we currently have additional less-common methods onSceneFilter
). - It sorta goes against the "single registry per world" idea. Not a huge issue, though, just something to think about.
Those are what I can think of, at least haha. The last reason is that this PR provides a better API while #5781 is still in review.
I created a followup PR based on this branch to add a dedicated example: https://github.com/bevyengine/bevy/pull/8955.
- There's a bit more overhead that would come with a registry as a filter. We really only care that a type is allowed or denied. But creating a temporary registry involves generating lots of
TypeRegistration
structs, which will come with more of a runtime cost using bevy_reflect: Recursive registration #5781 (unless the created registry filter is stored in a resource likeAppTypeRegistry
).
But it still acts as a filter as things need to be registered
- It could make dynamic filtering a little more complicated. Rather than simply needing a
TypeId
, we would either require the concrete type or the type-erasedTypeRegistration
.
That could be worked around by adding better APIs to derive a new type registry from an existing one.
- We'd still need to rely on the world's
AppTypeRegistry
since there's a chance the necessary type data is not auto-registered (i.e. not marked#[reflect(Component)]
or#[reflect(Resource)]
). So in this way, it doesn't really matter if the filter is a registry or not.
So you mean with this PR, the user would define a filter when creating their scene, and another hidden one (the type registry) would also be used without it being visible?
- The ergonomics could take a hit. If you had a query of known components and wanted all of them except a select few, you wouldn't be able to create a denylist. Instead, you'd have to create a new registry with all the other components.
That seems like an easy fix by adding that to the type registry rather than needing a whole new concept
- Similarly, we wouldn't be able to define default denylists (e.g. denying all components and resources that shouldn't be serialized).
Same, that can be fixed on the type registry api
- Also along those line, methods for filtering are limited by the methods on the registry. If we wanted to create a new method, it would probably have to live on
DynamicSceneBuilder
(we currently have additional less-common methods onSceneFilter
).
That's probably OK? Or it could live in the type registry?
- It sorta goes against the "single registry per world" idea. Not a huge issue, though, just something to think about.
Scenes are another world, so that's ok
Those are what I can think of, at least haha. The last reason is that this PR provides a better API while #5781 is still in review.
I think that to go the way of this PR, we need first to truly make the type registry global and hide it from the user when creating a scene so that they don't have to care which registry the world they're working with currently has.
The alterantive is to not add a new concept like scene filter but to improve the api on the type registry
- There's a bit more overhead that would come with a registry as a filter. We really only care that a type is allowed or denied. But creating a temporary registry involves generating lots of
TypeRegistration
structs, which will come with more of a runtime cost using bevy_reflect: Recursive registration #5781 (unless the created registry filter is stored in a resource likeAppTypeRegistry
).But it still acts as a filter as things need to be registered
- We'd still need to rely on the world's
AppTypeRegistry
since there's a chance the necessary type data is not auto-registered (i.e. not marked#[reflect(Component)]
or#[reflect(Resource)]
). So in this way, it doesn't really matter if the filter is a registry or not.So you mean with this PR, the user would define a filter when creating their scene, and another hidden one (the type registry) would also be used without it being visible?
I think it would help if we broke down registry-based filters for a moment.
For those that don't know, we currently have a "global"[^1] TypeRegistry
, which I'll distinguish by its resource type, AppTypeRegistry
. The current way we create scene filters is by creating a separate TypeRegistry
, registering the types we want to filter, and passing that to our builder.
And, from what I gather, the main reason to use a TypeRegistry
here is that it means that the user doesn't need to care about the state of the AppTypeRegistry
since the necessary data is guaranteed to exist on the local registry filter (assuming all types are either auto-registering the necessary data or the user does so manually).
While that API does avoid relying on the "hidden" AppTypeRegistry
when creating our DynamicScene
, it actually becomes an issue when loading it. There are two reasons why that is:
- Firstly, in most cases reflection-based deserialization relies on a registry to deserialize its data. Unfortunately, users have no way to configure which registry is used by the
SceneLoader
asset loader. - Secondly, the
SceneSpawner
resource responsible for handling applying a scene once loaded, again relies on theAppTypeRegistry
(note:SceneSpawner
is not required to apply a scene, it's just the mechanism used to automatically apply scenes).
This means that, unless we force users to manually deserialize and spawn in their scenes, they will always need to not only be aware of the main AppTypeRegistry
but ensure that it is a complete superset of any filter they use as well (if they plan on serializing it anyway).
We could add mechanics for syncing the registry filters back up to the AppTypeRegistry
internally (though, this could still be an issue if that syncing is deferred) or creating our filter from the actual AppTypeRegistry
(and somehow preventing "new" registrations which would cause the filter to become out of sync with the main AppTypeRegistry
), but these solutions still do not prevent the user from needing to care about the AppTypeRegistry
.
I suppose, if the argument is to make the AppTypeRegistry
more visible, then it does have some merit there. However, it still faces the same issues as the SceneFilter
as outlined above and I'd even argue that it's slightly worse since it could mislead users into thinking their filter will allow them to roundtrip their scenes from a serialized format.
And again, the TypeRegistry
comes with more overhead, both in memory usage and speed.
- It could make dynamic filtering a little more complicated. Rather than simply needing a
TypeId
, we would either require the concrete type or the type-erasedTypeRegistration
.That could be worked around by adding better APIs to derive a new type registry from an existing one.
- The ergonomics could take a hit. If you had a query of known components and wanted all of them except a select few, you wouldn't be able to create a denylist. Instead, you'd have to create a new registry with all the other components.
That seems like an easy fix by adding that to the type registry rather than needing a whole new concept
- Similarly, we wouldn't be able to define default denylists (e.g. denying all components and resources that shouldn't be serialized).
Same, that can be fixed on the type registry api
- Also along those line, methods for filtering are limited by the methods on the registry. If we wanted to create a new method, it would probably have to live on
DynamicSceneBuilder
(we currently have additional less-common methods onSceneFilter
).That's probably OK? Or it could live in the type registry?
I'm not sure we really want to conflate the registry with filtering. Yes, it can be used as a filter, but that's just a byproduct of its actual purpose: storing dynamic type information, mainly for (de)serialization.
It doesn't make much sense outside of the context of scenes to treat it like a filter, so I don't think we should cater its API for that one specific use case.
One nice thing about SceneFilter
is that you can allow or deny whatever components you want, all on the same filter (e.g. allowing a component when extracting one entity, then denying it for the remaining entities, leaving the other filtered components intact). But this concept doesn't exist and imo shouldn't exist on the TypeRegistry
. It would likely be a footgun to add the ability for users to remove registrations and could cause issues for third party libraries who might use it to uphold certain invariants (i.e. the existence of this type data implies the existence of this other one because my crate explicitly registers both).
At best, users would need to create a new type registry per filter and then indicate whether or not it's an allowlist or a denylist. SceneFilter
has a clean, clear, and consistent API dedicated to these operations that make it easy to use and understand. And while the methods on DynamicSceneBuilder
could hide any potential complexity for both SceneFilter
and TypeRegistry
, I think SceneFilter
wins out because of its simple allow/deny API outside of those methods. Setting a manual TypeRegistry
filter could be a lot more tedious and confusing, especially for larger and more complicated filters.
Also worth noting here is that wrapping the TypeRegistry
in a dedicated filter newtype, to me, defeats the whole purpose of keeping it as a TypeRegistry
, since again, we're forced to rely on the main AppTypeRegistry
anyways. However, we'd likely still need to do this or provide a set of standalone functions in order to generate default denylists (which can't be created from within bevy_reflect
).
Lastly, should requirements or design change, we wouldn't need to continue messing with the TypeRegistry
API to support scenes. Creating a dedicated filter type helps separate concerns and keep things tidy.
I think that to go the way of this PR, we need first to truly make the type registry global and hide it from the user when creating a scene so that they don't have to care which registry the world they're working with currently has.
The alterantive is to not add a new concept like scene filter but to improve the api on the type registry
Unfortunately, apart from auto-registering types or some future built-in Rust solution, I don't think there's any hope for a user to not have to be at least marginally aware of the registry when working with scenes, regardless of the filtering mechanism used.
[^1]: It's not really a global object, but rather a resource tied to a world (see https://github.com/bevyengine/bevy/issues/6101).
I will say that I could see there being confusion if there is a component missing in my scene after load/unload, where I might think there is something wrong with the filter instead of wrong with my registry. But when that happens I'd probably check the documentation and hopefully find information about the registry in the scene filter documentation.
When I used the scene code with the registry, I thought it was weird that it's asking for it as input when the world I'm getting the registry from is already a parameter. I didn't even think that there was some other use for the registry, to act as a filter or anything like that. Having a parameter named filter definitely makes that purpose more clear.
I stumbled across this PR after spending a couple of weeks reading the code for bevy_save
, moonshine_save
, and the 0.10.1 DynamicSceneBuilder
, and trying to get a entity-based save working for a map editor. The key piece that bevy_save
and moonshine_save
add to DynamicSceneBuilder
is primarily the ability to filter which components are saved for the entities.
@MrGVSV does a great job of explaining why a filter is preferable to dedicated a TypeRegistry
for saving, but I want to add to it.
Background
Right now in my editor you can save Tilesets (single Entity with a Tileset
component) and Maps (many Entities in a hierarchy with a handful Components that should be saved). As the editor grows, I'll need to be able to export additional things like individual/groups of NPCs, Items, etc for use in other maps, all in their own files. This is going to result in a lot of different filters based on what I need to save.
Creating an individual TypeRegistry
for each of these save types feels counter-intuitive, as what I really want to do is limit the components the builder will output. TypeRegistry
does not feel like the tool for that, but SceneFilter
does.
The best way I can describe why is that DynamicSceneBuilder::from_world_with_type_registry
feels like a clever shortcut to this functionality by leveraging existing code within DynamicSceneBuilder
that pulled reflection data from a TypeRegistry
(specifically AppTypeRegistry
). On the other hand SceneFilter
is explicitly built for the purpose of filtering components saved to a scene.
Tangent
For concerns about non-Reflect
types not appearing in output, if an allowed type is added to the filter, but not present in the TypeRegistry
, we should be able to detect that for Denylist
and warn!
.
Another Tangent
This would also be incredibly helpful to me if it makes it into 0.11; without it I'm gonna have to do something cursed.