-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Store Resources as components on singleton entities #20934
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
base: main
Are you sure you want to change the base?
Store Resources as components on singleton entities #20934
Conversation
…ce_entity_lookup
…ce_entity_lookup
…ce_entity_lookup
… into resource_entity_lookup
…ce_entity_lookup
chescock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your work pushing this forward! I imagine it's frustrating to be asked to completely rewrite it, and then to rewrite it back the first way again. But having an actual implementation of both approaches really is valuable for evaluating the tradeoffs!
And now, on to the nitpicking...
|
|
||
| #[test] | ||
| #[should_panic] | ||
| fn filtered_resource_mut_conflicts_write_with_resmut() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did these tests get removed? The behavior they're checking seems important to preserve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think deleting the filtered_resource_mut_conflicts_write_with_resmut test was a mistake and I deleted filtered_resource_reflect because I had a hard time understanding the test and since ReflectResource was changed so much I didn't know how to fix it.
I'll look into this again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've restored filtered_resource_mut_conflicts_write_with_resmut, but not filtered_resource_reflect. The test fundamentally doesn't make sense with the new ReflectResource implementation. I'm 90% sure that I'm going to redo FilteredResources at a later date, but that's a separate PR.
| if let Some(entity) = self.resource_entities.get(component_id) | ||
| && let Ok(entity_ref) = self.get_entity(*entity) | ||
| { | ||
| return entity_ref.contains_id(component_id) && entity_ref.contains::<IsResource>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why check for IsResource here? The hooks should prevent duplicates, and the resource_entities map is the authoritative source of whether this was a "resource entity" or not.
... ah, it's because we need that check in Res parameters so that they don't conflict with queries that use Without<IsResource> default query filters. ... oh, but you aren't actually checking that in UnsafeWorldCell::get_resource_with_ticks!
I think you need to check for the IsResource component consistently. Otherwise a malicious user could remove the component, and then get aliasing between queries with Without<IsResource> and resource access APIs.
It might help to have a method like UnsafeWorldCell::get_resource_entity(self, component_id: ComponentId) -> Option<UnsafeEntityCell> that will encapsulate doing the lookup on resource_entities and checking that the entity exists and has the component_id and IsResource components.
We may also want to cache the component_id of IsResource on the World somehow, or reserve a fixed value, in order to avoid the hash lookup from the TypeId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... a malicious user could ...
While all of the things you say are true: this is a game engine. Our users are game developers, not hackers. I'll look into this, but we can't guardrail every single corner of Bevy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While all of the things you say are true: this is a game engine. Our users are game developers, not hackers. I'll look into this, but we can't guardrail every single corner of Bevy.
Hah! Fair :). Though maybe I should apply Hanlon's razor and say that a foolish user could do those things :).
If the consequences here were a panic or weird behavior then I wouldn't worry, but this can cause Undefined Behavior if we get it wrong, and I think Bevy is usually pretty careful not to allow UB in safe code.
I think it's really just the checks in Res and ResMut that are important for safety, though, since those need to not alias with our friend Query<EntityMut, Without<IsResource>>. But if we change those, then it would just be surprising if World::get_resource found a resource that Res did not.
A hypothetical future implementation of Res<R> in terms of Single<R, With<IsResource>> would also check for IsResource, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the changes to Res and ResMut I don't think that this should cause UB anymore. Btw: I'm not using Single<> there because cart felt that QueryState was unnecessarily large (it's two component ids now).
Having
|
|
That said, (iteration over) broad queries should never occur in engine / library code outside of very niche applications like networking. These have a linear performance cost with the total number of entities, regardless of the type of entity, and should be refactored whenever possible. Evaluation of the current cases:
|
|
@alice-i-cecile My initial reaction was also that I also don't understand how that conflicts. We're not accessing broad data there, and if the component is missing we don't access anything on the resource entities. Sure we'll iterate through different entities but the shouldn't generate a conflict I think? @Trashtalk217 am I misunderstand here? This was a very useful exploration though, thank you! |
|
For If generic code like that winds up being useful for resources without any other changes, that seems like an argument in favor of allowing |
| } | ||
|
|
||
| #[test] | ||
| fn filtered_resource_reflect() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this test break? ... Oh, I see, ReflectResource no longer has a way to get a resource directly from a world!
ReflectComponent::get isn't an exact replacement for ReflectResource::get, since ReflectResource::get started from the world while ReflectComponent::get needed a specific entity.
I think we need to preserve that functionality somehow, so that reflection can look up resources through the resource_entities map. One option is to restore ReflectResource as its own thing, and another option is to add resource-like methods to ReflectComponent. If we do the latter, we should probably add #[deprecated] methods to ReflectResource to teach users about the name change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the few points where I decided to draw a line for the API. When implementing the v2, where resources were wrapped in ResourceComponent<...>, one major advantage was that any given struct can be annotated with both Resource and Component. But, as cart pointed out, this is confusing. If resources are 'just' components, how are they components in two ways?
A similar reasoning went into making ReflectResource shadow ReflectComponent exactly. Given that a Resource both derives the Resource and Component Trait, it's possible to both reflect(Component) as well as reflect(Resource).
This alone is kind of confusing, but in my opinion, it would be stranger if they both had a different interface.
Additionally, most of the time, the necessary entity data is nearby (see also the changes to bevy_remote), and reflection is deep enough into the internals that we can expect users to work with resources-as-components.
A better solution - in my opinion - is to have FilteredResources return FilteredEntityRefs instead of a FilteredResources which do work with reflect_resource.reflect(...), but that's not a change I'm making in this PR.
| } | ||
|
|
||
| component_access_set.add_unfiltered_resource_read(component_id); | ||
| let mut filter = FilteredAccess::default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to make a corresponding change to FilteredResources and FilteredResourcesMut so that they correctly conflict with queries.
The has_read/write_all_resources() branches would probably do filter.read_all_components(); filter.and_with(*is_resource_id); to declare access to all components on entities with IsResource, while the other branch would use the code you have here in a loop.
Alternately, it might make sense to modify add_unfiltered_resource_read/write() itself so that the code could be shared. I guess that would also affect NonSend, but requesting additional access like that should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was able to change filtered_resource.rs and the builder structs to make this happen. Everything now doubly registers access, both as a component as well as a resource, but that can be streamlined later.
cart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is on the right track!
| ComponentDescriptor::new_resource::<T>() | ||
| }) | ||
| } | ||
| self.register_component::<T>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider deprecating this in favor of register_component
| /// * [`ComponentsRegistrator::register_non_send()`] | ||
| #[inline] | ||
| pub fn register_resource_with_descriptor( | ||
| pub fn register_non_send_with_descriptor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant with register_component_with_descriptor. We should consider removing it.
crates/bevy_ecs/src/storage/mod.rs
Outdated
| /// Backing storage for resources. | ||
| pub resources: Resources<true>, | ||
| /// Backing storage for `!Send` resources. | ||
| pub non_send_resources: Resources<false>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this new world, I think NonSend deserves a "reframe", as these are no longer "resources" in the same way. Ex: "NonSend data", or in this case Resources<false> -> NonSends
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current plan around NonSend was to just delete them and make users use thread locals. This is because we want to refactor the winit loop and accounting for moving the non send storage out of the world was making things too complicated. All the in engine uses have already been replaced with thread locals already. So we could just remove them if we want to merge this soon. But it might be better to deprecate them for 0.18 first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with the specifics of thread locals, but on the discord I have seen people vocal about keeping it. From what I've heard it's something few users need, but for specific usecases - like scripting - it's the indispensable. I am renaming doing the rename as suggested, but removing NonSendData is out of scope for this PR.
| /// [`bevy_reflect::TypeRegistration::data`]. | ||
| #[derive(Clone)] | ||
| pub struct ReflectResource(ReflectResourceFns); | ||
| pub struct ReflectResource(ReflectComponent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People building reflection features should be able to assume that if a component implements Reflect and registers itself, that "component reflection" features will be present. Resources are components, so forcing reflection consumers to special case them when building features feels off.
I don't want to table this for later, because if we land this PR now, we would probably release 0.18 in a state where developers need to change their usage patterns, then change them again in 0.19.
However we might also want additional Resource reflection functionality / it does make sense to reflect(Resource). Additionally that ensures we don't break existing code. Buuut we also don't really want to require reflect(Resource, Component). Ideally reflect(Resource) could imply reflect(Component), but we don't currently have the functionality. I'll add that really quick, then you can take a dependency here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .is_some_and(ResourceData::is_present) | ||
| if let Some(entity) = self.resource_entities.get(component_id) | ||
| && let Ok(entity_ref) = self.get_entity(*entity) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we can probably remove the IsResource check here. Functionally the "resource entities" cache is behaving as the source of truth here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we can probably remove the IsResource check here. Functionally the "resource entities" cache is behaving as the source of truth here.
I think we need the check in Res and ResMut for soundness, and then we'll want it here for consistency. My thought process is:
We want to be able to use ResMut<R> and EntityMut in the same system, so we want to ensure that Query<EntityMut, Without<IsResource>> does not conflict with ResMut<R>.
The IsResource component can be removed in safe code. Doing that is weird, so we can panic or do something weird, but we can't cause UB.
Therefore, Res<R> and ResMut<R> must ignore resource entities that don't have the IsResource component. Otherwise they could cause aliasing with a Query<EntityMut, Without<IsResource>>.
And if Res<R> fails to find a resource, then contains_resource::<R> should return false for consistency.
A slightly different approach would be to remove an entity from the "resource entities" cache if its IsResource component is removed, probably by using an on_replace hook on IsResource. That would ensure that the cache was the source of truth. You'd need to get the ComponentId somehow, so you'd probably want to store it in the IsResource component. But storing it there might be useful anyway for dynamic resource access!
|
|
||
| /// Create a new dynamic scene from a given world. | ||
| pub fn from_world(world: &World) -> Self { | ||
| let resource_entities: Vec<Entity> = world.resource_entities().values().copied().collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in love with this whole "filter out resource entities only to insert the resource entities" approach. But it will require rethinking how "resource filters" work so I'm cool with tabling it.
| continue; | ||
| } | ||
|
|
||
| if resource_entities.contains(&entity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't going to cut it preformance-wise. We might have hundreds of thousands of entities and hundreds of resources.
Resources are entities and extract_entities as an operation shouldn't care about which entities are passed in. It should be the responsibility of the caller to determine what entities should be extracted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would lead to a possible conflict between extract_resources and extract_entities. What happens if you both extract a resource as well as its entity? Both would be in the scene file, so when reading that back in, which should get priority?
I really wanted to tackle the bevy_scene seperately from this PR, maybe we can put up a warning in the method docs for now?
crates/bevy_scene/src/scene.rs
Outdated
| world.spawn_empty().id() | ||
| }; | ||
|
|
||
| reflect_resource.copy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This notably won't carry over any other components attached to the resource. We're losing one of the primary benefits of the "resources as entities" thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really wanted to redo bevy_scene completely. However, after your review of #21346, where you didn't want to change the scene format in a major way, I mostly reverted all of the changes. I've come to the conclusion that I really want a scene file to 'just be entities' and maybe some metadata.
// everything is an entity and just goes here, including resources
4294967297: (
components: {
"bevy_ecs::name::Name": "joe",
"bevy_transform::components::global_transform::GlobalTransform": ((1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0)),
// ...
"scene::ComponentB": (
value: "hello",
),
},
),
4294967298: (
components: {
"scene::ResourceA": (
x: 3.0,
y: 4.0,
),
"bevy_ecs::resource::Is_Resource": (),
},
),
We would have to solve the issue of two resources with the same ComponentId being loaded from two different scene files, but I think that's managable. With this, carrying over other components is easy.
In any case, I expected the design work to be in some other PR in the same cycle, and instead opted for keeping it as-is.
| component_access_set.add_unfiltered_resource_read(component_id); | ||
| let mut filter = FilteredAccess::default(); | ||
| filter.add_component_read(component_id); | ||
| filter.add_resource_read(component_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer it if we took the time to simplify Access in this PR. It is a key piece of this port, and I'm personally more interested in landing this "right" than landing it quick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer it if we took the time to simplify
Accessin this PR. It is a key piece of this port, and I'm personally more interested in landing this "right" than landing it quick.
The tricky part here is that WorldQuery types like AccessChanged<A> can declare resource access (#16843). That resource access is outside of the entities matched by the query, so if we remove the separation between component and resource access, then we need a different way to express that. One option is to support queries having multiple FilteredAccesses, such as by doing #21557.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take a look at this again, but most of the 'why didn't you also do this in this PR' comes down to those things being non-trivial and me not wanting to balloon this PR more than necessary.
I will look at this again tomorrow though.
# Objective The proposed ["Resources as entities"](#20934 ) PR makes Resource implement Component. This creates a situation where we need to do: ```rust #[derive(Resource, Reflect)] #[reflect(Resource, Component)] struct Thing; ``` I'm of the mind that we should add the ability for TypeData to have "dependencies". Ex `reflect(Resource)` implies `reflect(Component)`. For "subtrait" types, I think supporting this is logical / users would appreciate it. ## Solution 1. Add a new `FromType<T>::insert_dependencies` function with a default impl (which makes this a non-breaking change). This does kind of overload the `FromType` trait (ex: a name like `GetTypeData` might be better with this new context), but this is a pretty niche trait / piece of functionality, and I like the idea of not breaking people. 2. Add a new `TypeRegistration::register_type_data<T, V>` function, which initializes the TypeData `T` for a given type `V` , inserts that type data, and also inserts any dependent type data using `insert_dependencies`. 3. Adjust the `Reflect` macro to use `register_type_data` instead of `insert(FromType::<Self>::from_type())` This makes it possible to do the following: ```rust impl<R: Resource + FromReflect + TypePath> FromType<R> for ReflectResource { fn from_type() -> Self { ReflectResource } fn insert_dependencies(type_registration: &mut TypeRegistration) { type_registration.register_type_data::<ReflectComponent, R>(); } } ``` Which then allows dropping `reflect(Component)`: ```rust #[derive(Resource, Reflect)] #[reflect(Resource)] struct Thing; ``` ## Testing I added a unit test 😜
This is part of #19731.
Resources as Components
Motivation
More things should be entities. This simplifies the API, the lower-level implementation and the tools we have for entities and components can be used for other things in the engine. In particular, for resources, it is really handy to have observers, which we currently don't have. See #20821 under 1A, for a more specific use.
Current Work
This removes the
resourcesfield from the world storage and instead store the resources on singleton entities. For easy lookup, we add aHashMap<ComponentId, Entity>toWorld, in order to quickly find the singleton entity where the resource is stored.Because we store resources on entities, we derive
ComponentalongsideResource, this means thatturns into
This was also done for reflections, meaning that
becomes
In order to distinguish resource entities, they are tagged with the
IsResourcecomponent. Additionally, to ensure that they aren't queried by accident, they are also tagged as being internal entities, which means that they don't show up in queries by default.Drawbacks
Resourceand aComponent, becauseResourceexpands to also implementComponent, this means that this throws a compiler error as it's implemented twice.ReflectComponentyou need to importbevy_ecs::reflect::ReflectComponentevery time you use#[reflect(Resource)]. This is kind of unintuitive.Future Work
Accessin the ECS, to only deal with components (and not components and resources).Res<Resource>toSingle<Ref<Resource>>(or something similair).ReflectResource.