Store resources as components on singleton entities (v2) #21346
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,13 @@ | ||
| ( | ||
| resources: { | ||
| 4294967299: ( | ||
| components: { | ||
| "bevy_ecs::resource::ResourceComponent<scene::ResourceA>": (( | ||
|
There was a problem hiding this comment. It is worth pointing out that from a user-facing perspective, this is strictly worse UX by a pretty wide margin. Harder to read, harder to write, harder to understand, takes up more space, error prone (any additional data you attach here will be replaced the second another scene tries to set the resource). Special casing resources in scenes makes a lot of sense I think. There was a problem hiding this comment. Agreed: ideally we can avoid any breakage to the representation of resources in scenes. There was a problem hiding this comment. That might be possible, but I would still like to support the entity notation. If / when people start attaching extra data to resource entities, that should be serializable / deserializable through scenes. The only solution I can think of is to store resources twice (both in entities and in resources). It would look something like this: What do you think of this? There was a problem hiding this comment. How would you reconcile the entity-notation version with this comment? |
||
| score: 1, | ||
| )), | ||
| "bevy_ecs::resource::IsResource": (), | ||
| "bevy_ecs::entity_disabling::Internal": (), | ||
| }, | ||
| ), | ||
| }, | ||
| entities: { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,15 @@ | ||
| //! Resources are unique, singleton-like data types that can be accessed from systems and stored in the [`World`](crate::world::World). | ||
|
|
||
| use core::ops::Deref; | ||
|
|
||
| use crate::{ | ||
| entity_disabling::Internal, | ||
| lifecycle::HookContext, | ||
| prelude::{Component, ReflectComponent}, | ||
| world::DeferredWorld, | ||
| }; | ||
| use bevy_reflect::{prelude::ReflectDefault, Reflect}; | ||
|
|
||
| // The derive macro for the `Resource` trait | ||
| pub use bevy_ecs_macros::Resource; | ||
|
|
||
|
|
@@ -73,3 +83,65 @@ pub use bevy_ecs_macros::Resource; | |
| note = "consider annotating `{Self}` with `#[derive(Resource)]`" | ||
| )] | ||
| pub trait Resource: Send + Sync + 'static {} | ||
|
|
||
| /// A component that contains the resource of type `T`. | ||
| /// | ||
| /// When creating a resource, a [`ResourceComponent`] is inserted on a new entity in the world. | ||
| /// | ||
| /// This component comes with a hook that ensures that at most one entity has this component for any given `R`: | ||
| /// adding this component to an entity (or spawning an entity with this component) will despawn any other entity with this component. | ||
| /// Moreover, this component requires both marker components [`IsResource`] and [`Internal`]. | ||
| /// The former can be used to quickly iterate over all resources through a query, | ||
| /// while the latter marks the associated entity as internal, ensuring that it won't show up on broad queries such as | ||
| /// `world.query::<Entity>()`. | ||
| #[cfg_attr(feature = "bevy_reflect", derive(Reflect), reflect(Component))] | ||
| #[derive(Component)] | ||
| #[require(IsResource, Internal)] | ||
| #[component(on_add = on_add_hook, on_remove = on_remove_hook)] | ||
| #[repr(transparent)] | ||
|
There was a problem hiding this comment. I'm not in love with the ResourceComponent wrapper. Why not implement Component directly for the resource? This would make it appear nicely (without a wrapper type) in scenes / brp / inspectors, it would make the component type id match the type of the resource, it would make it look nicer in queries, easier to consume when being queried for, would result in less codegen, fewer reflected types, etc, etc, etc. There was a problem hiding this comment. This was previously attempted, and caused problems with type safety and open questions around overlap between ReflectResource and ReflectComponent. I'm open to reverting back to that design: I didn't consider the codegen concerns. There was a problem hiding this comment. If you prefer an approach without the wrapper, you can review the first version (#20934). |
||
| pub struct ResourceComponent<R: Resource>(pub R); | ||
Trashtalk217 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| pub(crate) fn on_add_hook(mut deferred_world: DeferredWorld, context: HookContext) { | ||
| let world = deferred_world.deref(); | ||
| if world.resource_entities.contains(context.component_id) { | ||
| // the resource already exists and we need to overwrite it | ||
| let offending_entity = *world.resource_entities.get(context.component_id).unwrap(); | ||
| deferred_world.commands().entity(offending_entity).despawn(); | ||
Trashtalk217 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. I don't think silently despawning an existing resource entity / replacing it is something we should allow as part of normal flows. This would break any existing references to the resource entity, including things like relationships. It would also erase any additional context we might add to the entity (ex: marker components). I personally think we are better off despawning the new entity, losing the new value, and logging an error. We lose many of the benefits of representing resources as entities if we can't rely on the stability of their entity id or their components. (I am aware of how this behavior would interact with scenes, see my other comment) |
||
| } | ||
| // we update the cache | ||
| // SAFETY: We only update a cache and don't perform any structural changes (component adds / removals) | ||
Trashtalk217 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| unsafe { | ||
| deferred_world | ||
| .as_unsafe_world_cell() | ||
| .world_mut() | ||
Trashtalk217 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| .resource_entities | ||
| .insert(context.component_id, context.entity); | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn on_remove_hook(mut deferred_world: DeferredWorld, context: HookContext) { | ||
| let world = deferred_world.deref(); | ||
| // If the resource is already linked to a new (different) entity, we don't remove it. | ||
| if let Some(entity) = world.resource_entities.get(context.component_id) | ||
| && *entity == context.entity | ||
| { | ||
| // SAFETY: We only update a cache and don't perform any structural changes (component adds / removals) | ||
Trashtalk217 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| unsafe { | ||
| deferred_world | ||
| .as_unsafe_world_cell() | ||
| .world_mut() | ||
| .resource_entities | ||
| .remove(context.component_id); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// A marker component for entities which store resources. | ||
| #[cfg_attr( | ||
| feature = "bevy_reflect", | ||
| derive(Reflect), | ||
| reflect(Component, Default, Debug) | ||
| )] | ||
| #[derive(Component, Debug, Default)] | ||
| #[require(Internal)] | ||
| pub struct 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.
One of the motivators for this change is that we get "free" functionality for resources, in the context of things like entity inspectors. However I think this is a great example of why that "free" functionality would require additional custom handling for resources to make it "good".
An entity inspector would (logically) display this as something like:
And notably this would be hidden by default because it is Internal.
Is this actually better than having a separate resource section of the inspector that displays it as:
without needing to disable the Internal filter and manually filter down to IsResource?
Of course, you could build that functionality on top of the entity representation. But the ideal UX is very different from what we get by default, and the default UX doesn't really win us much in the majority of cases. That is also true for the scene case (see my other comment).