Skip to content
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

Store resources as components on singleton entities #17485

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Jan 21, 2025

Objective

Various ECS features (hooks, observers, immutable components, relationships) don't work with resources.
Users (and engine devs) rightfully expect that they would, and are frustrated that they don't.

While we could replicate these features (and all future ECS features) to support resources, this is a serious source of complexity and slows down implementation.

Additionally, various patterns (networking and serialization in particular) would like to operate generically over data, regardless of whether it's a resource or a component.

Solution

  • make Resource require a Component trait bound, and make the Resource derive also derive Component
  • add a ResourceEntity<R> component, which marks an entity as holding a resource of type R. This component is "unique": only entity with this component (for a given R) can exist at once (using hooks)
  • add a IsResource marker component, used for all resource entities, regardless of their type. This will mostly be useful to allow inspectors to sort resources into their own list.
  • store the resource data of type R as a component on R
  • add a ComponentId -> Entity lookup for resources to the World
  • spawn an entity with the right data when inserting a resource
  • despawn the matching entity when removing a resource
  • look up the matching entity when fetching resource data from the World
  • rewrite the internal logic of Res and ResMut to look up entity data
  • rewrite dynamic system building logic to ensure that it works under the new model
  • use the same strategy as ReflectComponent to account for immutable components
  • migrate dynamic resource logic
  • remove the now-unused internals for working with resources

Note: we fully expect this to slow down resource look-ups relative to the previous tightly optimized implementation. While we can likely claw back some of that performance (see future work), we think that the simpler code base and added features are worth it.

Potential blockers

  • spawning entities doesn't track the caller, and I can't figure out how to pass that information in
  • NonSend data uses the resource storages and API. We can either support NonSend components or move it out of the world

Controversial choices

Please argue with these if you disagree!

  1. The Resource derive also derives Component. This is somewhat implicit, and of questionable Rust style. This is much less breaking for existing users, and less confusing for beginners.
  2. The Resource trait (and all existing APIs) continue to exist. As discussed in this HackMD, we think that this better communicates intent and eases learning, even though all of the methods could just use bare entity-component APIs
  3. Components on resource entities will show up in queries by default. While the new default query filters feature would let us exclude them, being able to operate generically is useful for code that can be flexibly used as either a resource or component.
  4. The resource marker components are publicly constructable, and rely on hooks to ensure correctness. We could instead refuse to expose any constructors, and avoid the use of hooks by auditing and testing all call sites.
  5. Resource data is stored directly as R on the resource entity, instead of inside of a wrapper type. This allows users to operate abstractly over types which can be used as either resources or components more easily.

Future work

  1. By storing the Entity of each resource type in the ECS itself (possibly as part of a components-as-entities push), we could make looking up resources substantially faster, as we wouldn't need to query for matching entities.
  2. We could remove or greatly strip down the ReflectResource trait, as all resources are now components.

Testing

TODO

Migration Guide

TODO

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible 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-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 21, 2025
@ItsDoot ItsDoot added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Jan 21, 2025
@ItsDoot
Copy link
Contributor

ItsDoot commented Jan 21, 2025

There are performance implications here w.r.t using World::try_query/_filtered that should be benchmarked, even if we decide not to tackle them in this PR.

@alice-i-cecile
Copy link
Member Author

[17:38]Alice 🌹: After a nice cup of tea and a bike ride, I think that trying to avoid caching the ComponentId -> Entity lookup for resources is silly and going to bite us in terms of perf
[17:40]Alice 🌹: So that means:

Don't use component hooks, instead make the marker types unconstructable.
Add a field to world for the resource_map: HashMap<ComponentId, Entity> lookup
Write to that field when inserting and removing resources
Read from that field during lookups
[17:40]Alice 🌹: No queries needed
[17:40]Alice 🌹: It's good to keep around the marker components (at least the generic resource one), but that'll be entirely an end-user / inspector nicety

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 22, 2025
@alice-i-cecile
Copy link
Member Author

Alright, [Cart and the SME-ECS], I've seen enough of the resources-as-components work to have a good sense of what it entails. Doing it all at once is going to be unreviewable (and prone to nasty merge conflicts), so I'd like to try and split this out once I have sign-off on the core idea from y'all.

Why should we do this

  1. There are a growing number of ECS features that work with components that don't work with resources (hooks, observers, immutable components, relations...). Many of these have very good use cases!
  2. This duplication is onerous to maintain.
  3. Various high-level tools (inspectors, scenes, BRP) could be simplified if they were allowed to treat everything as component data.

Why shouldn't we do this

  1. We can probably get faster resource lookups, insertion and removal if we special-case this. I don't think this is anywhere near the hot path though.
  2. The mental model becomes a bit less clear for users, since "resources are components". Keeping the Resource trait (and the resource-specific methods) around helps a lot.
  3. Jondolf's pretty labelling tool will need a special-case to not mark all resources as components.

Proposed migration path

Each step would be its own PR.

  1. Define our marker components, store a lookup of resource entities and spawn/remove resource entities (without storing resource data on them).
  2. Make Resource: Component, and have the derive macro automatically derive Component + read its attributes.
  3. Remove or shrink ReflectResource, since it's now redundant.
  4. Resolve NonSend data, one way or another.
  5. Make sure caller information can be passed into entity spawning.
  6. Insert and fetch resource data on the components.
  7. Remove all of the resource internals (before this, they're marked with expect(dead_code)).

Open questions

  1. What do we do with NonSend data? Currently, these are stored as resources. We can either a) move them out of the World (a long-standing goal of Joy) or b) support NonSend components (requested by spectria-limina).
  2. Do we want to do this in the 0.16 cycle, or shelve it for now?
  3. Should resources show up in queries for components of the same type? We can newtype the data (which would break component hooks), or add a default query filter.

Asked in https://discord.com/channels/691052431525675048/1331346448662397059/1331813217537757238

@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! and removed S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 23, 2025
@alice-i-cecile
Copy link
Member Author

Alright, that was fun :) I don't think this is worth pursuing just yet (at least for me!): the NonSend resources are going to prevent us from actually finishing this project, and supporting NonSend component just doubles down on what is IMO (and in that of other SME-ECS) a serious technical mistake.

That said, I do really think this is the right long-term direction for Bevy! If you'd like to adopt and fight for this, the plan above seems fundamentally sound and I would be happy to mentor you. It will almost certainly be easier just to redo and copy-paste this work, rather than trying to rebase these changes.

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 C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Blocked This cannot move forward until something else changes X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants