Skip to content

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Trashtalk217
Copy link
Contributor

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.

Method or Code snippet Should it account for Resources? Is that how it works in this PR?
world.entities.len() No Yes, but there should probably a method to count entities, that excludes resources.
world.query::<()> Yes No, this returns every single entity (except disabled ones)
world.clear_entities() Yes No, and world.clear_resources() should also be changed.
world.iter_entities() and world.iter_entities_mut() Yes No

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.

@Trashtalk217 Trashtalk217 added A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Jun 18, 2025
Copy link
Contributor

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.

@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jun 18, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile left a 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:

  1. Use hooks to keep the resource storage / resource entities in sync.
  2. Use hooks to ensure resource entity uniqueness.
  3. 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.

@Trashtalk217
Copy link
Contributor Author

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 IsResource and ResourceEntity registered through Reflection and I don't know how Reflection works.

@Trashtalk217 Trashtalk217 added D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Help The author needs help finishing this PR. and removed D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 22, 2025
@alice-i-cecile
Copy link
Member

It's complaining that I don't have IsResource and ResourceEntity registered through Reflection and I don't know how Reflection works.

You're going to want to derive Reflect (gated behind a bevy_reflect feature), and then call .register_type for those types. Search to see where we do that for the other bevy_ecs component types!

github-merge-queue bot pushed a commit that referenced this pull request Jun 22, 2025
# 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>
@tbillington
Copy link
Contributor

I haven't followed any prior discussion on this, so apologies in advance!

I can't see a justification in the hackmd for entities.len() ignoring "resource entities". Is this just for test ergonomics? Perhaps a private test helper function can solve this rather than "lying" from what seems like a very fundamental API for lack of a better word.

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?

Trashtalk217 added a commit to Trashtalk217/bevy that referenced this pull request Jul 10, 2025
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>
@Trashtalk217
Copy link
Contributor Author

One of these days I'll get better at git
:\

@Trashtalk217 Trashtalk217 force-pushed the resource_entity_lookup branch from f4e2b35 to 042337f Compare July 10, 2025 22:25
@Trashtalk217 Trashtalk217 removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 12, 2025
@Trashtalk217
Copy link
Contributor Author

Trashtalk217 commented Jul 12, 2025

@tbillington
entity_count does give a 'wrong' count of the number of entities (although it is documented that this doesn't count Resources). The reason for this is twofold: first, there's test ergonomics. A lot of tests simply assume that a new world is empty and I expect that this is going to become less and less true in the future (components as entities for example, will increase the number of entities that exists in a world).
Secondly, throughout the World module, a distinction is made between resources and entities: iter_resources and iter_entities, or clear_entities and clear_resources. I've come to the conclusion that it's preferable to keep these concepts separate. It would be surprising to iterate through entities and find resources, similarly it is confusing to count entities and have resources be included in the count.

For the old behaviour, there's still world.entities().len(), world.entity_count() is a convenience method.

As for your second question: Why is a new world not empty? DefaultQueryFilters is a Resource added at world creation, that holds all the components that are automatically excluded from queries.

@@ -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>();
Copy link
Contributor

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>>()
Copy link
Contributor

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>();
Copy link
Contributor

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;
Copy link
Contributor

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.
Copy link
Contributor

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!

Copy link
Contributor

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.

Comment on lines +1714 to +1724
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);
}

Copy link
Contributor

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.

Suggested change
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());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Help The author needs help finishing this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants