bevy_scene: Add SceneFilter

[MrGVSV]

Objective

Currently, DynamicScenes 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:

1. Create a new AppTypeRegistry
2. Register NPC
3. 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/resources. With the DynamicSceneBuilder, users have more control over how their DynamicScenes 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();

Similar operations exist for resources:

View Resource Methods

To only serialize a subset of resources, use the allow_resource method:

let scene = builder
  .allow_resource::<ResourceA>()
  .extract_resources()
  .build();

To serialize everything but a subset of resources, use the deny_resource method:

let scene = builder
  .deny_resource::<ResourceA>()
  .extract_resources()
  .build();

Or create a custom filter:

let resources = HashSet::from([type_id]);

let filter = SceneFilter::Allowlist(resources);
// let filter = SceneFilter::Denylist(resources);

let scene = builder
  .with_resource_filter(Some(filter))
  .extract_resources()
  .build();

Open Questions

• allow and deny 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.
• DynamicSceneBuilder extracts entity data as soon as extract_entity/extract_entities is called. Should this behavior instead be moved to the build 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 added extract_entity_with_filter-type methods). Is this preferable?
• Should we include constructors that include common types to allow/deny? For example, a SceneFilter::standard_allowlist that includes things like Parent and Children? Consensus suggests we should. I may split this out into a followup PR, though.
• 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
• Should SceneFilter be an enum? Would it make more sense as a struct that contains an is_denylist boolean? With the added SceneFilter::None state (replacing the need to wrap in an Option or rely on an empty Denylist), it seems an enum is better suited now
• Bikeshed: Do we like the naming convention? Should we instead use include/exclude terminology? Sounds like we're sticking with allow/deny!
• 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

Followup Tasks

• Add a dedicated SceneFilter example
• Possibly add default types to the filter (e.g. deny things like ComputedVisibility, allow Parent, etc)

---

Changelog

• Added the SceneFilter enum for filtering components and resources when building a DynamicScene
• Added methods:
  • DynamicSceneBuilder::with_filter
  • DynamicSceneBuilder::allow
  • DynamicSceneBuilder::deny
  • DynamicSceneBuilder::allow_all
  • DynamicSceneBuilder::deny_all
  • DynamicSceneBuilder::with_resource_filter
  • DynamicSceneBuilder::allow_resource
  • DynamicSceneBuilder::deny_resource
  • DynamicSceneBuilder::allow_all_resources
  • DynamicSceneBuilder::deny_all_resources
• Removed methods:
  • DynamicSceneBuilder::from_world_with_type_registry
• DynamicScene::from_scene and DynamicScene::from_world no longer require an AppTypeRegistry reference

Migration Guide

• DynamicScene::from_scene and DynamicScene::from_world no longer require an AppTypeRegistry 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);

[MrGVSV]

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.

[MrGVSV]

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.

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?

[Carbonhell]

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!

[MrGVSV]

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?

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 has GlobalTransform but the user needs to serialize it, I imagine he could just do SceneFilter::default().allow::<GlobalTransform>()).

Yeah that might make more sense and would match TypeRegistry (whose Default impl includes the standard registrations). Thanks for the suggestion!

[Testare]

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>().

[MrGVSV]

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 empty SceneFilter::DenyList is actually equivalent to allow_all(), and an empty SceneFilter::AllowList is actually equivalent to deny_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, since allow_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>();
}

[MrGVSV]

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.

[kaosat-dev]

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.

[MrGVSV]

• 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.

[cjhowedev]

Should this be a trait implementation? We could move this into an impl FromWorld for DynamicScene since it doesn't need the second argument anymore.

[MrGVSV]

Good point!

[MrGVSV]

Hm, I just checked the FromWorld trait definition and it looks like it actually takes &mut World. So looks like we can't do this :/

[cjhowedev]

Maybe we could impl From<&World> for DynamicScene instead?

[MrGVSV]

Possibly. I'm also starting to wonder if doing From impls has a negative effect on discoverability. Constructors directly on the struct are clearly visible from the documentation, but the documented From impls can be hard to miss.

Maybe we save these changes for a followup PR to allow for further discussion (if any)?

[cjhowedev]

Should this be a trait implementation? We could move this into an impl From<&Scene> for DynamicScene since it doesn't need the second argument anymore.

[MrGVSV]

Yeah this makes sense as well. I'll do this too!

[MrGVSV]

I also decided to hold off on this. If anything, we can save this for a future PR.

[alice-i-cecile]

Yeah, I'm fine to leave this to follow-up.

[alice-i-cecile]

@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]

@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.

[MrGVSV]

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.

[alice-i-cecile]

Hmm: this behavior here is surprising to me. I was expecting this to be additive / incremental, for easy tweaking.

But after some thought, there's no sensible way to have both a list of "allow this" and "deny that": what do you do about unlisted types?

[MrGVSV]

Oops I think this is outdated. We did move to an incremental behavior: allowing a type in a denylist simply removes it from that list. I'll update the docs to match!

[alice-i-cecile]

Why should this be TypeId rather than ComponentId? The scene filter tool would be useful with dynamic components, and the component/resource types need to be registered already.

I can live with this being moved to follow-up work though.

[MrGVSV]

I believe my reasoning was purely an ergonomics one. I wanted this kind of behavior:

filter
  .allow::<Foo>()
  .allow::<Bar>()
  .allow::<Baz>();

And I'm not sure if it would be possible to achieve that by relying on ComponentId since it requires access to the world (please correct me if I'm wrong).

[alice-i-cecile]

Ah hmm. Right, that's much trickier.

I think in a follow-up then we should add two more enum variants that store ComponentId for the more advanced cases at the expense of ergonomics.

[alice-i-cecile]

A few quibbles, but nothing that can't be done in follow-up work.

A dedicated example demonstrating this would be particularly nice, but I don't want to block this further.

[alice-i-cecile]

@MrGVSV can you update the docs this weekend? If so, I'll merge this Monday :)

[mockersf]

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?

[MrGVSV]

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:

1. 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 like AppTypeRegistry).
2. 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-erased TypeRegistration.
3. 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.
4. 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.
5. Similarly, we wouldn't be able to define default denylists (e.g. denying all components and resources that shouldn't be serialized).
6. 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 on SceneFilter).
7. 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.

[MrGVSV]

I created a followup PR based on this branch to add a dedicated example: #8955.

[mockersf]

1. 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 like AppTypeRegistry).

But it still acts as a filter as things need to be registered

2. 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-erased TypeRegistration.

That could be worked around by adding better APIs to derive a new type registry from an existing one.

3. 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?

4. 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

5. 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

6. 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 on SceneFilter).

That's probably OK? Or it could live in the type registry?

7. 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

[MrGVSV]

1. 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 like AppTypeRegistry).

But it still acts as a filter as things need to be registered

3. 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"¹ 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:

1. 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.
2. Secondly, the SceneSpawner resource responsible for handling applying a scene once loaded, again relies on the AppTypeRegistry (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.

2. 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-erased TypeRegistration.

That could be worked around by adding better APIs to derive a new type registry from an existing one.

4. 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

5. 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

6. 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 on SceneFilter).

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.

Footnotes

1. It's not really a global object, but rather a resource tied to a world (see https://github.com/bevyengine/bevy/issues/6101). ↩

[Testare]

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.

[dmlary]

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.

[cart]

I think this is the right path forward for scenes as they exist today and the patterns we're currently using.

That being said, I think the current state of scene definitions is suboptimal:

• It is easy to accidentally include a "runtime only" component in a scene by forgetting to exclude it.
• The information about whether or not to include components like ComputedSize in a given entity should exist in the declaration of that entity's types, not floating around in a global filter registration (ex: the Bundle, the individual Components, or some yet-to-be-designed Prefab-like thing). I suspect we will land on something that would make this separation implicit (and therefore impossible to get wrong). Ex: by creating runtime-only things from the Prefab-like-thing-that-is-yet-to-be-designed.
• The pattern of treating the "runtime state" of a "normal" app world as a scene (while interesting) seems like it might need to go. Having the "scene state vs runtime state" separation makes things much cleaner I think.

All of that is a conversation for a later date though. For now, this makes the current approach to things way more usable.

I also do understand the argument to use TypeRegistries instead of creating a new SceneFilter concept. The "do we need this new concept" reflex is a good one to have. I think we could make TypeRegistries work for this, but I also agree that it would be contorting them for something that they weren't made for (or optimized for). I think keeping them separate concepts is the right call.