-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Add entities alongside resources #19711
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?
Add entities alongside resources #19711
Conversation
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
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 like this as an incremental step forward. In this PR, I would like to:
- Use hooks to keep the resource storage / resource entities in sync.
- Use hooks to ensure resource entity uniqueness.
- Add
IsResource
to our default query filters.
With that in place, I think we can probably merge this safely as an incremental, mildly useful step forward.
Due to a couple of issues, I've decided to simplify this PR to the bare minimum. There are so many tests that assume that the world starts off with zero entities, that I've decided to only address that in this PR. So I removed the component hook (it will return) and used the simplest cache. In particular I need a bit of help with the bevy_scene tests. It's complaining that I don't have |
You're going to want to derive |
# Objective There is a lot of `world.entities().len()`, especially in tests. In tests, usually, the assumption is made that empty worlds do not contain any entities. This is about to change (#19711), and as such all of these tests are failing for that PR. ## Solution `num_entities` is a convenience method that returns the number of entities inside a world. It can later be adapted to exclude 'unexpected' entities, associated with internal data structures such as Resources, Queries, Systems. In general I argue for a separation of concepts where `World` ignores internal entities in methods such as `iter_entities()` and `clear_entities()`, that discussion is, however, separate from this PR. ## Testing I replaced most occurrences of `world.entities().len()` with `world.num_entities()` and the tests passed. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
I haven't followed any prior discussion on this, so apologies in advance! I can't see a justification in the hackmd for I'm also unsure why a blank, newly created world would have entities. I assume there's a reason for it I'm unaware of but this PR doesn't give rationale for it. If we no longer assume a "default" world will be empty, will there be a world constructor that does create an empty world? |
There is a lot of `world.entities().len()`, especially in tests. In tests, usually, the assumption is made that empty worlds do not contain any entities. This is about to change (bevyengine#19711), and as such all of these tests are failing for that PR. `num_entities` is a convenience method that returns the number of entities inside a world. It can later be adapted to exclude 'unexpected' entities, associated with internal data structures such as Resources, Queries, Systems. In general I argue for a separation of concepts where `World` ignores internal entities in methods such as `iter_entities()` and `clear_entities()`, that discussion is, however, separate from this PR. I replaced most occurrences of `world.entities().len()` with `world.num_entities()` and the tests passed. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
One of these days I'll get better at git |
f4e2b35
to
042337f
Compare
@tbillington For the old behaviour, there's still As for your second question: Why is a new world not empty? |
@@ -169,6 +170,7 @@ impl World { | |||
|
|||
// This sets up `Disabled` as a disabling component, via the FromWorld impl | |||
self.init_resource::<DefaultQueryFilters>(); | |||
self.register_disabling_component::<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 should probably be added in impl FromWorld for DefaultQueryFilters
alongside the registration for Disabled
. That way, users can do DefaultQueryFilters::from_world(world)
to recreate the default value.
@@ -485,6 +487,7 @@ mod tests { | |||
|
|||
let mut query = QueryBuilder::<(FilteredEntityMut, EntityMutExcept<A>)>::new(&mut world) | |||
.data::<EntityMut>() | |||
.filter::<Without<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.
These should no longer be necessary now that you have a default query filter, right?
@@ -2175,6 +2177,7 @@ mod tests { | |||
let mut df = DefaultQueryFilters::empty(); | |||
df.register_disabling_component(world.register_component::<C>()); | |||
world.insert_resource(df); | |||
world.register_disabling_component::<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.
These tests might be more robust if they start with the default DefaultQueryFilters
instead of an empty()
one. So, just calling
world.register_disabling_component::<C>();
instead of creating and inserting df
manually.
reflect(Component, Default, Debug) | ||
)] | ||
#[derive(Component, Default, Debug)] | ||
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.
Would it be valuable to stick the ComponentId
of the resource in here so we can map in the other direction? I guess that would mean you couldn't use required components, though.
.resource_entities | ||
.contains_key(&component_id) | ||
{ | ||
// Since we don't know the type, we use a placeholder type. |
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.
Do we need to handle the case where someone uses the untyped API to insert a typed resource? I can imagine things like the editor or BRP needing to do that. This will add ResourceEntity::<TypeErasedResource>
even if the resource has a type.
You might need to register the ComponentId
of the appropriate ResourceEntity<R>
type when you register the resource, and then add it dynamically here (which should be possible, since it's a ZST). I think you could store the extra ComponentId
inside the resource's ComponentDescriptor
. That would even let you register a unique ComponentId
for each untyped resource!
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.
Do we need to handle the case where someone uses the untyped API to insert a typed resource? I can imagine things like the editor or BRP needing to do that. This will add
ResourceEntity::<TypeErasedResource>
even if the resource has a type.
Wait, no, I'm wrong, those will use reflection, which is built on the typed API.
So, there's technically a hole here, but user code would have to go out of its way to use the untyped API in order to trigger it, so it's probably not a big deal.
if !self | ||
.components | ||
.resource_entities | ||
.contains_key(&component_id) | ||
{ | ||
let entity = self.spawn(ResourceEntity::<R>::default()).id(); | ||
self.components | ||
.resource_entities | ||
.insert(component_id, 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 these could use entry
to avoid the double lookup.
if !self | |
.components | |
.resource_entities | |
.contains_key(&component_id) | |
{ | |
let entity = self.spawn(ResourceEntity::<R>::default()).id(); | |
self.components | |
.resource_entities | |
.insert(component_id, entity); | |
} | |
self.components | |
.resource_entities | |
.entry(component_id) | |
.or_insert_with(|| self.spawn(ResourceEntity::<R>::default()).id()); |
Objective
First step towards resources as components as described in #17485.
Solution
We add an entity alongside every Resource. We do not store any data on it yet, just to see if anything breaks. We also added a small cache that links ComponentIDs to entities for Resources.
Testing
I should probably add a test that checks how good the cache works.
Discussion
As outlined in this Hackmd doc, we want to keep the Resources conceptually separate from 'normal' entities, and as such for every single method that deals with entities we can play a fun gameshow game: "Does this method account for Resources?"
Here's a table with my opinion of how it should work. The third column tells you if this is how it currently works in this PR.
world.entities.len()
world.query::<()>
world.clear_entities()
world.clear_resources()
should also be changed.world.iter_entities()
andworld.iter_entities_mut()
This table can easily be expanded, but the more we add, the less of a good idea it is to tackle this in a single PR.