-
-
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?
Store resources as components on singleton entities (v2) #21346
Conversation
This reverts commit 8c4269c.
the |
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.
Ooh, this is exciting! I like the ResourceComponent<R: Resource>
approach! It looks good overall, and most of my comments are style nits.
I think the world_mut()
in the hooks is unsound, though, so we'll need to fix something there.
I'm also curious about the reasons for despawning the whole entity when replacing the resource or in resource_scope
. I had been expecting to preserve the entity and just add and remove the component, but I haven't thought through what the tradeoffs are between the two approaches.
Co-authored-by: Chris Russell <[email protected]>
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.
Exciting changes!
|
||
/// Type-erased equivalent of [`Components::valid_resource_id()`]. | ||
#[inline] | ||
#[deprecated( |
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.
- If I'm understanding this PR correctly, the behaviour of this function changes? So if I had a code
get_valid_resource_id(resource_type_id)
and upgraded bevy, the function call would start returning unexpected results, correct? In that case, I think it would be better to remove this function completely instead of just deprecating it (users will have to change the code anyways). - If I just have a
TypeId
and don't know the typeR
, is there still some way for me to check if it's registered? (for example, I could get the type IDs from iterating over the wholeTypeRegistry
registrations and checking forReflectResource
type data). If not, one option would be to add these function toReflectResource
.
(the same applies to get_resource_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 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 comment
The 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.
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 haven't kept up much with the changes from 0.17, so I did not do a deep dive review for this code. But at a glance, I don't see any major issues with the implementation.
My knowledge here is purely from my experimentation in my own ecs prototypes. At least, I think that's why my review was requested, so I'm reviewing more the concept than the code for now.
Bevy has kind of put itself in a difficult place because we present entities and resources fundamentally differently to users: Res
vs Single
, etc. From my research, an ideal world would have no distinction: a "resource" is just a component where it just so happens that there can only ever be one of them. Then, its some pretty thin utilities over the top, and you're done.
I haven't been following this feature super closely, but from what I can tell, that's not the direction Bevy wants to take–IIUC, mostly for compatibility reasons. That's why we have to make sure resources don't appear in queries, etc. That is, the hard part here is making sure that a resource entity doesn't happen to also have a Transform
component. In my prototype, I see no reason to disallow that, but I can understand the compatibility concerns for Bevy. (Ex: The Bevy way is to have a TargetedEnemy(Option<Entity>)
resource, not a ThisEntityIsTargeted
resource that also lives on the targeted entity.)
So the question is: how do we have it both ways? How can resources be on entities, but not be treated like entities? I think this pr (again, at a glance) does a pretty good job of walking that line.
My only real concern is that this might make it harder to create and use resources that do not correspond to rust types. That might not be an issues, but IIRC, it was supported before, and it's worth mentioning.
But I don't see anything "bad" about this approach in concept, given the compatibility concerns.
Hopefully that all made sense. There's a lot of moving parts 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 happy with this approach, and the diff is generally quite good. I would also like to get this merged quickly then move on to removing the old resources data storage, but we should address the soundness problems first.
I don't have the skills/time to do that very effectively myself though.
@alice-i-cecile I also want to get this merged, but removing the old resource data storage is sadly still quite a ways away. This is because non-send resources also use the resources data storage, and there's still some open questions about faciliating non-send data. |
Co-authored-by: Alice Cecile <[email protected]>
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 looks good! I'd like the safety rules around getting mutable access through ResourceCache
to be clarified before I approve, but I think it's already sound (although it may be easier to prove that if we make UnsafeWorldCell::resource_entities()
an unsafe fn
).
Does this need a migration guide? The things in the PR description under "Other Changes" sound like they're all breaking changes.
crates/bevy_ecs/src/resource.rs
Outdated
{ | ||
let cache = deferred_world.as_unsafe_world_cell().resource_entities(); | ||
// SAFETY: We only update a cache and don't perform any structural changes (component adds / removals) | ||
unsafe { &mut *cache.0.get() }.insert(context.component_id, context.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 think you meant remove
here:
unsafe { &mut *cache.0.get() }.insert(context.component_id, context.entity); | |
unsafe { &mut *cache.0.get() }.remove(context.component_id); |
Should we have some unit tests that would have caught that? Although, hmm, what does go wrong if we don't remove
here? All of the lookups already do separate checks that the entity exists and has the right component on it. I guess it's just a memory leak?
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.
Given all of the checks we do, I don't think this hook is actually necessary, but it's nice to keep the cache up to date. Good catch!
crates/bevy_ecs/src/resource.rs
Outdated
pub struct ResourceCache(SyncUnsafeCell<SparseSet<ComponentId, Entity>>); | ||
|
||
impl ResourceCache { | ||
fn inner(&self) -> &SparseSet<ComponentId, 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.
If this can really be a safe function, then it might be easier to make it an impl Deref
and impl DerefMut
. That would avoid the need for helper methods that do fn foo(&self) { self.inner().foo() }
.
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.
Looks good! I left some suggestions to expand on the tricky SAFETY
comments.
Co-authored-by: Chris Russell <[email protected]>
…evy into resource-as-components-v2
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.
Goodness this is a big PR and quite hard to review, but I tried my best!
.map(bevy_ecs::archetype::ArchetypeEntity::id), | ||
.map(bevy_ecs::archetype::ArchetypeEntity::id) | ||
.filter(|id| { | ||
world | ||
.get_entity(*id) | ||
.is_ok_and(|entity| !entity.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.
This check could instead be done on the archetype, right?
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.
Yes
crates/bevy_ecs/src/resource.rs
Outdated
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(); | ||
if context.entity != offending_entity { | ||
deferred_world.commands().entity(offending_entity).despawn(); | ||
} | ||
} |
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.
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(); | |
if context.entity != offending_entity { | |
deferred_world.commands().entity(offending_entity).despawn(); | |
} | |
} | |
if let Some(&offending_entity) = world.resource_entities.get(context.component_id) | |
&& offending_entity != context.entity | |
{ | |
// the resource already exists and we need to overwrite it | |
deferred_world.commands().entity(offending_entity).despawn(); | |
} |
mostly a style thing, but why search for the component twice :)
/// # Panics | ||
/// If `SEND` is false, this will panic if a value is present and is not accessed from the | ||
/// original thread it was inserted in. | ||
#[expect( |
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.
Since these are pub(crate)
, is there a reason these are marked as dead_code
rather than removed?
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 removed them.
/// The value referenced by `value` must be valid for the given [`ComponentId`] of this world. | ||
#[inline] | ||
#[track_caller] | ||
pub unsafe fn insert_resource_by_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.
Could this receive some docs telling the user that this should be a ResourceComponent<R>
and not an R
(which, iiuc, could be a component as well, but when used here would just spawn an entity with that component with no way of addressing the entity), maybe with a link to resource_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.
Technically, because ResourceComponent<_>
is #[transparent]
this is not necessary. Passing in a pointer with R
, still works.
crates/bevy_scene/src/scene.rs
Outdated
// Resources archetype | ||
for (component_id, resource_data) in self.world.storages().resources.iter() { | ||
if Some(component_id) == self_dqf_id { | ||
for (component_id, scene_entity) in self.world.resource_entities().iter() { |
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 duplicate since the resources are part of the archetype entities iterated and copied below?
Which would also mean that the default query filters actually do get copied over and overwritten in the parent world.
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.
Good find, I overlooked that.
fn main() { | ||
App::new() | ||
.add_plugins(DefaultPlugins) | ||
.register_type::<ResourceComponent<ResourceA>>() |
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 would certainly be outside of the scope of this PR, but:
This is required because the scene serializer wants all components on resource entities as PartialReflect
, right?
R
should be auto-registered, so maybe the scene serializer could just serialize the inner R
instead of the ResourceComponent<R>
?
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.
Yes, but that would cause some other issues. The most important of which is that when deserializing a scene, we have to run the ResourceComponent
hooks in order to reconstruct the ResourceCache
. If we only serialize R
, that doesn't happen automatically, and we have to add a seperate path for resources that wrap every R
into a ResourceComponent
and runs the hooks. This is not impossible, but I found this to be far easier.
And getting auto-registration to work for ResourceComponent<ResourceA>
is my first priority after this is merged.
.add_plugins(AssetPlugin::default()) | ||
.add_plugins(ScenePlugin); | ||
.add_plugins(ScenePlugin) | ||
.register_type::<ComponentA>(); |
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.
Note: I was trying to figure out why this would be necessary since nothing this PR changes should have had any effect on this, but it turns out that the CI runs tests with bevy_app/reflect_auto_register
and bevy_ecs/reflect_auto_register
, via root's default features which turn on registration of derived types in app and ecs respectively, which make this register_type
superfluous; however when running individual tests via your dev environment of choice or by running tests from the bevy_scene
directory, those features aren't enabled and this test fails, even before this PR.
Personally, I think tests should run for the crates without needing features from "above", so this is a good change but it confused me for a bit.
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.
Looks much better in terms of safety than the other PR. I still see a couple of nits, but nothing that's back-to-the-drawing-board level of blocking.
|
||
/// Type-erased equivalent of [`Components::valid_resource_id()`]. | ||
#[inline] | ||
#[deprecated( |
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 seems a pretty big footgun IMO and not something to clean up later on.
I'm sorry for being so terse @SkiFire13, I'm really tired and annoyed from all the nitpicks and I only addressed the important comments. |
// SAFETY: | ||
// - The IsResource component id matches | ||
// - The constructor constructs an IsResource | ||
// - The id is valid because the component was just registered |
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 id
comes from the caller, which might have just registered it but there's no guarantee or documentation that this needs to be the case. That's why I previously suggested that this function should be made unsafe
to track the prerequisite that id
is a valid component id. The argument that it was just registered can be put on the callers instead.
(This is the last "nit" I promise. I know it feels minor, but these kind of hidden safety requirements can quickly pile up and cause bugs. I recently rewrote the whole required components logic because of this and found multiple bugs while making safety requirements explicit, so they are not something so minor)
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.
Thank you, I've added an assertion to fix this issue.
This is largely identical to #20934, except we store resource data on a
ResourceComponent<R: Resource>
component on a singleton entity. This has the benefit of sidestepping the annoying double-derive problem of having to both deriveComponent
andResource
for each resource.For more information check out the original PR description #20934.
Closes #20934, #19711, #17485 and is part of #19731.
Other Changes
Deprecate
Components
methodsThis PR deprecates
Components::get_valid_resource_id(type_id: TypeId)
, andComponents::get_resource_id(type_id: TypeId)
. This is because resources (excluding non-send) are not registered with theirTypeId
anymore. Instead they're registered by theTypeId
ofResourceComponent<SomeResource>
. This changes the API as follows:becomes
This becomes confusing, as the method name suggests we're dealing with resources, while in order to use it correctly, we must already be aware that resources are actually hidden behind a component. The same is true for the
get_valid_resource_id
method. Both are being deprecated in favour of eitherresource_id
andvalid_resource_id
for when the type is available, orget_id
orget_valid_id
when it isn't. Using the latter two does require the user to wrap the type inResourceComponent<_>
in order for it to work correctly.Registering a Resource Type
When registering a resource type manually through
app.register_type::<R>()
or through theAppTypeRegistry
, a user must wrap the type inResourceComponent<R>
in order for it to be properly registered.However, when using the untyped API's like
get_resource_by_id
, the user can simplyread::<R>()
sinceResourceComponent<_>
is transparent.Resources implement
MapEntities
by defaultIn order to have
MapEntities
working with resources,#[derive(Resource)]
now automatically implementsMapEntities
. This makes it such thatno longer compiles. Instead write: