-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
RFC: Minimal Implementation of Indexed Components
#10671
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
Conversation
The generated |
examples/ecs/indexing.rs
Outdated
for (entity, component) in self.changed.iter() { | ||
self.index.update(entity, Some(component.clone())); | ||
} |
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 will require iterating over all entities that have a T
component every time a Index
may be out of sync, which IMO isn't ideal. AFAIK bevy_mod_index
explored this possibility and it wasn't very efficient.
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 this can definitely be improved. #5097 will naturally improve this problem as well, and by adding a filter query onto the Index
, I'm hoping to get performance good enough when users opt-in to indexing to make it worthwhile. Right now, it just needs to be more ergonomic and more performant than what a user could write themselves in a reasonable amount of effort.
@laundmo, your perspectives here will be valuable :) IMO we should try to pull together some benchmarks early in this process to validate performance under a few scenarios. |
We may need #5097 to accelerate this. |
the more i think about it, the more what most people want out of indexes/indexed components is very different from what i'd need or have experience with. getting entities by component values is very different to getting entities by how their values relate to other entities. this seems to implement the common case of getting them by their own components value. |
This is my current goal just to keep scope tight. I believe that if we had access to this kind of But that's all much further in the future. Right now, I know that |
I agree. Right now, the only way I can see to make an Ideally, an |
Yep, for this PR, I would focus on making this a useful and reliable primitive, regardless of whether or not it's used to implement relations in the future. |
e0ae998
to
50c7e6b
Compare
50c7e6b
to
d6eed58
Compare
Ensures access to plugin system without modifying the ECS crate.
13562b3
to
19be538
Compare
Replaced with a simple demonstration of a graph orthogonal to the standard `Parent` / `Child` relationships. Items can have a `Parent` and/or an `Owner`. `Owner` is analogous to `Parent`, and with `Index`, `Owned` (analog of `Children`) is not required.
Is this still under evaluation? Because this basic index functionality that is described is something that pops up quite frequently in networking code. We are visualizing data from http APIs, and this is a feature we would like the engine to provide for us if possible |
Objective
Goals
Index
system parameter which will provide an automatically managed way to retrieve anEntity
byComponent
value. The planned public API for this is:Index
Index::get
method for retrieving all entities indexed against a particular valueIndex::iter
method for retrieving all entities grouped by their index valueQuery
type, allowing for a filter.Non-Goals (for now!)
Parent
orChildren
components)Join<Left, Q1, Q2, Index<PlayerId>>
Details
A user would first register an index during app creation:
This will allow users to access the
Index<PlayerId>
system parameter:An
Index
will then be used to provide access to anEntity
based on component value through a methodget
:This is what I consider to be the core functionality required to make an
Index
worth using at all.Current Implementation Details
Index
is aSystemParam
bundling access to several relevant items from the ECS. It is currently generic over 3 parameters,T
,F
, andI
.:T
: TheComponent
thisIndex
is interested in (e.g.,Transform
)F
: A filter query. This can be used to restrict the indexing to only a subset of componentsT
(e.g.,With<PlayerFlag>
)I
: TheIndexer
, which describes how to get a usable index from a componentT
.F
andI
are both optional, utilising a defaultIndexer
provided for components if they are able to implementHash + Clone + Eq
.Hash
andEq
are required because the bidirectional map is built on aHashMap
.Clone
is required in order to create a constant time bijection. It may be possible to relax this requirement with an alternate implementation of a bidirectional map.Because
Index
includes aQuery<'w, 's, (Entity, Ref<'static, T>), (Changed<T>, F)>
, it already flags to the scheduler that this parameter requires immutable access to the componentT
, prevent mutable access from an alternate system. One current pain point is the performance ofChanged<T>
, which walks over allT
(O(n)
). #5097 could improve this performance, but it is also a potentially contentious change.In order to catch removals, a
RemovedComponents<'w, 's, T>
parameter is included. Because this is an event reader, to read the events requires mutable access, makingIndex
itself require mutable access. This is also a pain point, since it will require all systems using the index to operate sequentially, even if they could in principle run in parallel. I do believe it would be possible to remove the mutable access requirement fromIndex
using a custom implementation ofSystemParam
that hides these items using interior mutability, but this is a future goal for now.To ensure all removals and changes are witnessed, adding an
Index
adds a once-per-frame (Update
) system which updates the backend bidirectional map. This is unfortunate, and needs to be revisited.To ensure the
Index
is up-to-date within a frame (e.g., in-between mutable accesses ofT
), reading theIndex
triggers an update check. If thethis_run
Tick
fromSystemChangeTick
is not equal to the value currently stored with theIndex
, the map is updated. This should ensure the update only occurs at most once after mutation.Performance 🏠🐘
To be blunt, I don't believe it is currently feasible to make an index with performance comparable to Bevy's current (and excellent) ECS query model. It's already very well optimised, and if your algorithm can work with queries in the order they come in, I don't think indexing will improve anything for you at all. Caching is incredibly friendly to the SoA storage that Bevy uses for in-order access, and indexing is inherently for out-of-order access.
However, I think for certain algorithms, and certain stages of development, the ergonomics of accessing an entity by a value outweighs the negatives. This is why my focus for this PR is "what API should an
Index
have?". I believe that agreeing on this API will create a fixed target that can be optimised for far better than trying to tackle this problem in the reverse direction.It may turn out that the only way to get a performant index is to modify the underlying
Table
/SparseSet
types (or create a whole new alternative), but that represents such a large body of work, that I feel it should only be undertaken if there's an API for users that will benefit from its addition.Changelog
TBA
Migration Guide
There should be zero changes to existing APIs, with this being a purely additive PR.
Notes
This is still heavily work-in-progress and I would highly encourage feedback from anyone with a vested interest in how this is implemented to provide feedback, desired goals, and non-goals. I am willing to make major changes to the current proposed design based on feedback so please feel free to provide it!
See Also
Rollback
component