-
-
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 <[email protected]>
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 <[email protected]>
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? |
crates/bevy_ecs/src/query/builder.rs
Outdated
@@ -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?
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.
Not here, it probably has something to do with the fact we're querying EntityMutExcept
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.
Huh, that's not what I would have expected. ... I see, if a query has read access to a disabled component, then we ignore the filter. So it's not just EntityMutExcept
; even ordinary EntityMut
is enough to disable the filters.
@alice-i-cecile @NiseVoid Do we expect Query<EntityMut>
to ignore all default query filters, or is this a bug? I thought the intention was to ignore filters if the component was "mentioned", but EntityMut
has access to all components without really mentioning them.
Edit: Oh, and EntityMutExcept<IsResource>
doesn't ignore the default query filter, because it doesn't have read access to IsResource
, even though it "mentions" it.
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.
yeah, this part of the code is real weird
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.
@alice-i-cecile @NiseVoid Do we expect Query to ignore all default query filters, or is this a bug? I thought the intention was to ignore filters if the component was "mentioned", but EntityMut has access to all components without really mentioning them.
This is a bug :(
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.
@alice-i-cecile @NiseVoid Do we expect Query to ignore all default query filters, or is this a bug? I thought the intention was to ignore filters if the component was "mentioned", but EntityMut has access to all components without really mentioning them.
This is a bug :(
I have an idea on how to fix this, by ignoring unbounded access in Access::contains()
but then adding archetypal access to Entity(Ref|Mut)Except
. It will get some edge cases like Query<(EntityRef, Option<&Disabled>)>
wrong, but I think it works on all reasonable cases. I'll try to push up a PR tomorrow or Wednesday!
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.
Fix for default query filters on EntityMut
is here: #20163
Appreciate the explanation!
I get where you're coming from, to me this seems like a perspective thing. As a user of bevy, that makes sense. But from an engine or tooling perspective it's awkward. I feel like using "entity" to refer to all of these things is setting everyone up for confusing going forward. I imagine conversations like "Is X resource an entity?" - "Well yes but no, it is implemented using entities but you shouldn't treat it like an entity and it won't show up in a vanilla query". Is there different language we can use to refer to these "engine/internal/hidden" entities vs what a user would expect to interact with aka "user land" entities? Aside, I'm curious if we expect 3rd party libraries to be able to register entities that behave like the resource entities do. Eg for editor purposes. |
In my head what you're doing is implementing a kind of layering. Akin to physics or render layers but "hardcoded" so to speak. So in the engine you'd not use a layer mask to do internal entity management stuff, but users are implicitly using a default layer mask to exclude "engine" entities. |
@tbillington With regards to names: I would prefer an 3rd Party plugins could use the same idea. Because this PR mostly just uses default query filters, which anyone can use. Editor support was a consideration when that feature was introduced. The |
Agreed on the use of |
Would we want to mark registered systems ( |
@@ -51,9 +51,10 @@ pub fn world_entity(criterion: &mut Criterion) { | |||
for entity_count in RANGE.map(|i| i * 10_000) { | |||
group.bench_function(format!("{entity_count}_entities"), |bencher| { | |||
let world = setup::<Table>(entity_count); | |||
let offset = world.resource_count(); |
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 so fragile: I really don't like this.
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 exactly correct. I've justified it to myself by noting that creating entities from raw numbers is really fragile, so this is not really that bad in comparison.
It's not great either.
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 suppose we could use the lower level entities.len()
, which would be more robust to adding internal entities that are not resources, but I think that the offset stuff is gonna have to stay.
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.
setup
calls world.spawn_batch
, which returns an iterator that yields the actual Entity
values. Would it work to stick those in a Vec<Entity>
and iterate that instead of using integers and Entity::from_raw
?
@@ -815,10 +819,19 @@ mod tests { | |||
|
|||
assert_eq!( |
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 test sucks lol.
@@ -785,7 +789,7 @@ mod tests { | |||
|
|||
let (scene, deserialized_scene) = roundtrip_ron(&world); | |||
|
|||
assert_eq!(1, deserialized_scene.entities.len()); | |||
assert_eq!(3, deserialized_scene.entities.len()); |
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 needs a comment.
@@ -611,6 +613,8 @@ mod tests { | |||
registry.register::<MyEntityRef>(); | |||
registry.register::<Entity>(); | |||
registry.register::<MyResource>(); | |||
registry.register::<IsResource>(); | |||
registry.register::<ResourceEntity<DefaultQueryFilters>>(); |
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.
Does this mean we need to register all of our ResourceEntity<Foo>
types?
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.
Okay, I'm on board with this broad strategy, and like the way that you've split this out. That said, I am not very happy with what this has done to the modified tests, and would also like to add these entities to our default query filters immediately.
Proposed plan:
- Make a PR for
Internal
now, adding observers and one shot systems to it initially. - Do your best to clean up the failing tests and benchmarks revealed in this PR to make them less brittle.
- Rebase this work on those changes, add
Internal
to your resource entities via required components, and get a much cleaner diff.
…0163) # Objective Don't ignore default query filters for `EntityRef` or `EntityMut`. Currently, `Query<EntityRef>` will include entities with a `Disabled` component, even though queries like `Query<()>` or `Query<Entity>` would not. This was noticed in #19711 (comment). ## Solution Change `Access::contains` to completely ignore read access and just look at filters and archetypal access. Filters covers `With`, `Without`, `&`, and `&mut`, while archetypal covers `Has` and `Allows`. Note that `Option<&Disabled>` will no longer count as a use of `Disabled`, though.
# Objective Currently, benchmarks access the world's entities in a very unsafe way, which doesn't hold up if we add more internal entities. Part of #19711. ## Solution Have the setups return a `Vec<Entity>` we can iterate over. ## Testing Not needed.
# Objective As we move more stuff to entities, it's a good idea to keep these entities quasi-private. We do not want to confuse users by having to explain everything as being an entity. This came out of #19711. ## Solution This PR introduces the concept of internal entities, entities marked by the `Internal` component, that are filtered out by queries through `DefaultQureyFilters` and also don't show up for `World::entity_count()`. ## Testing Added a test.
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.