-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Store resources as components on singleton entities (v2) #21346
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?
Changes from all commits
dd53970
449b132
8c4269c
1d62f80
96e758c
54e5f10
a990c99
1c27389
233f967
3e93e00
3f68bdc
ff679fe
1a517f8
d206bf8
7298223
5718b34
4025a5e
610d9ae
4297457
4098c11
f0b91c9
2fa342e
7faca83
e4b53dc
fcb593e
37c7f80
136e930
0706222
2a5da87
9220980
5e65a4f
cc33a19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,13 @@ | ||
( | ||
resources: { | ||
"scene::ResourceA": ( | ||
score: 1, | ||
4294967299: ( | ||
components: { | ||
"bevy_ecs::resource::ResourceComponent<scene::ResourceA>": (( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed: ideally we can avoid any breakage to the representation of resources in scenes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
---|---|---|
|
@@ -20,7 +20,7 @@ use crate::{ | |
}, | ||
lifecycle::ComponentHooks, | ||
query::DebugCheckedUnwrap as _, | ||
resource::Resource, | ||
resource::{Resource, ResourceComponent}, | ||
storage::SparseSetIndex, | ||
}; | ||
|
||
|
@@ -290,18 +290,7 @@ impl ComponentDescriptor { | |
/// | ||
/// The [`StorageType`] for resources is always [`StorageType::Table`]. | ||
pub fn new_resource<T: Resource>() -> Self { | ||
Self { | ||
name: DebugName::type_name::<T>(), | ||
// PERF: `SparseStorage` may actually be a more | ||
// reasonable choice as `storage_type` for resources. | ||
storage_type: StorageType::Table, | ||
is_send_and_sync: true, | ||
type_id: Some(TypeId::of::<T>()), | ||
layout: Layout::new::<T>(), | ||
drop: needs_drop::<T>().then_some(Self::drop_ptr::<T> as _), | ||
mutable: true, | ||
clone_behavior: ComponentCloneBehavior::Default, | ||
} | ||
Self::new::<ResourceComponent<T>>() | ||
} | ||
|
||
pub(super) fn new_non_send<T: Any>(storage_type: StorageType) -> Self { | ||
|
@@ -348,7 +337,6 @@ impl ComponentDescriptor { | |
pub struct Components { | ||
pub(super) components: Vec<Option<ComponentInfo>>, | ||
pub(super) indices: TypeIdMap<ComponentId>, | ||
pub(super) resource_indices: TypeIdMap<ComponentId>, | ||
// This is kept internal and local to verify that no deadlocks can occor. | ||
pub(super) queued: bevy_platform::sync::RwLock<QueuedComponents>, | ||
} | ||
|
@@ -587,8 +575,12 @@ impl Components { | |
|
||
/// Type-erased equivalent of [`Components::valid_resource_id()`]. | ||
#[inline] | ||
#[deprecated( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(the same applies to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're right, but I'll leave it to a clean-up PR. I really want to start wrapping this up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a pretty big footgun IMO and not something to clean up later on. |
||
since = "0.18.0", | ||
note = "Use valid_resource_id::<R>() or get_valid_id(TypeId::of::<ResourceComponent<R>>()) for normal resources. Use get_valid_id(TypeId::of::<R>()) for non-send resources." | ||
)] | ||
pub fn get_valid_resource_id(&self, type_id: TypeId) -> Option<ComponentId> { | ||
self.resource_indices.get(&type_id).copied() | ||
self.indices.get(&type_id).copied() | ||
} | ||
|
||
/// Returns the [`ComponentId`] of the given [`Resource`] type `T` if it is fully registered. | ||
|
@@ -613,7 +605,7 @@ impl Components { | |
/// * [`Components::get_resource_id()`] | ||
#[inline] | ||
pub fn valid_resource_id<T: Resource>(&self) -> Option<ComponentId> { | ||
self.get_valid_resource_id(TypeId::of::<T>()) | ||
self.get_valid_id(TypeId::of::<ResourceComponent<T>>()) | ||
} | ||
|
||
/// Type-erased equivalent of [`Components::component_id()`]. | ||
|
@@ -665,15 +657,12 @@ impl Components { | |
|
||
/// Type-erased equivalent of [`Components::resource_id()`]. | ||
#[inline] | ||
#[deprecated( | ||
since = "0.18.0", | ||
note = "Use resource_id::<R>() or get_id(TypeId::of::<ResourceComponent<R>>()) instead for normal resources. Use get_id(TypeId::of::<R>()) for non-send resources." | ||
)] | ||
pub fn get_resource_id(&self, type_id: TypeId) -> Option<ComponentId> { | ||
self.resource_indices.get(&type_id).copied().or_else(|| { | ||
self.queued | ||
.read() | ||
.unwrap_or_else(PoisonError::into_inner) | ||
.resources | ||
.get(&type_id) | ||
.map(|queued| queued.id) | ||
}) | ||
self.get_id(type_id) | ||
} | ||
|
||
/// Returns the [`ComponentId`] of the given [`Resource`] type `T`. | ||
|
@@ -705,7 +694,7 @@ impl Components { | |
/// * [`Components::get_resource_id()`] | ||
#[inline] | ||
pub fn resource_id<T: Resource>(&self) -> Option<ComponentId> { | ||
self.get_resource_id(TypeId::of::<T>()) | ||
self.get_id(TypeId::of::<ResourceComponent<T>>()) | ||
} | ||
|
||
/// # Safety | ||
|
@@ -724,7 +713,7 @@ impl Components { | |
unsafe { | ||
self.register_component_inner(component_id, descriptor); | ||
} | ||
let prev = self.resource_indices.insert(type_id, component_id); | ||
let prev = self.indices.insert(type_id, component_id); | ||
debug_assert!(prev.is_none()); | ||
} | ||
|
||
|
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).