Replace VisitEntities with MapEntities#18432
Merged
cart merged 7 commits intobevyengine:mainfrom Mar 21, 2025
Merged
Conversation
cBournhonesque
approved these changes
Mar 20, 2025
Brezak
reviewed
Mar 20, 2025
eugineerd
reviewed
Mar 20, 2025
Contributor
|
I've opened up a pull request on this pull request to implement |
Shatur
approved these changes
Mar 20, 2025
Contributor
Shatur
left a comment
There was a problem hiding this comment.
I'm excited about this change!
Member
Author
I'm going to hold off on merging this, as I think I'd like to walk back the |
mockersf
pushed a commit
that referenced
this pull request
Mar 23, 2025
# Objective There are currently too many disparate ways to handle entity mapping, especially after #17687. We now have MapEntities, VisitEntities, VisitEntitiesMut, Component::visit_entities, Component::visit_entities_mut. Our only known use case at the moment for these is entity mapping. This means we have significant consolidation potential. Additionally, VisitEntitiesMut cannot be implemented for map-style collections like HashSets, as you cant "just" mutate a `&mut Entity`. Our current approach to Component mapping requires VisitEntitiesMut, meaning this category of entity collection isn't mappable. `MapEntities` is more generally applicable. Additionally, the _existence_ of the blanket From impl on VisitEntitiesMut blocks us from implementing MapEntities for HashSets (or any types we don't own), because the owner could always add a conflicting impl in the future. ## Solution Use `MapEntities` everywhere and remove all "visit entities" usages. * Add `Component::map_entities` * Remove `Component::visit_entities`, `Component::visit_entities_mut`, `VisitEntities`, and `VisitEntitiesMut` * Support deriving `Component::map_entities` in `#[derive(Coomponent)]` * Add `#[derive(MapEntities)]`, and share logic with the `Component::map_entities` derive. * Add `ComponentCloneCtx::queue_deferred`, which is command-like logic that runs immediately after normal clones. Reframe `FromWorld` fallback logic in the "reflect clone" impl to use it. This cuts out a lot of unnecessary work and I think justifies the existence of a pseudo-command interface (given how niche, yet performance sensitive this is). Note that we no longer auto-impl entity mapping for ` IntoIterator<Item = &'a Entity>` types, as this would block our ability to implement cases like `HashMap`. This means the onus is on us (or type authors) to add explicit support for types that should be mappable. Also note that the Component-related changes do not require a migration guide as there hasn't been a release with them yet. ## Migration Guide If you were previously implementing `VisitEntities` or `VisitEntitiesMut` (likely via a derive), instead use `MapEntities`. Those were almost certainly used in the context of Bevy Scenes or reflection via `ReflectMapEntities`. If you have a case that uses `VisitEntities` or `VisitEntitiesMut` directly, where `MapEntities` is not a viable replacement, please let us know! ```rust // before #[derive(VisitEntities, VisitEntitiesMut)] struct Inventory { items: Vec<Entity>, #[visit_entities(ignore)] label: String, } // after #[derive(MapEntities)] struct Inventory { #[entities] items: Vec<Entity>, label: String, } ```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
There are currently too many disparate ways to handle entity mapping, especially after #17687. We now have MapEntities, VisitEntities, VisitEntitiesMut, Component::visit_entities, Component::visit_entities_mut.
Our only known use case at the moment for these is entity mapping. This means we have significant consolidation potential.
Additionally, VisitEntitiesMut cannot be implemented for map-style collections like HashSets, as you cant "just" mutate a
&mut Entity. Our current approach to Component mapping requires VisitEntitiesMut, meaning this category of entity collection isn't mappable.MapEntitiesis more generally applicable. Additionally, the existence of the blanket From impl on VisitEntitiesMut blocks us from implementing MapEntities for HashSets (or any types we don't own), because the owner could always add a conflicting impl in the future.Solution
Use
MapEntitieseverywhere and remove all "visit entities" usages.Component::map_entitiesComponent::visit_entities,Component::visit_entities_mut,VisitEntities, andVisitEntitiesMutComponent::map_entitiesin#[derive(Coomponent)]#[derive(MapEntities)], and share logic with theComponent::map_entitiesderive.ComponentCloneCtx::queue_deferred, which is command-like logic that runs immediately after normal clones. ReframeFromWorldfallback logic in the "reflect clone" impl to use it. This cuts out a lot of unnecessary work and I think justifies the existence of a pseudo-command interface (given how niche, yet performance sensitive this is).Note that we no longer auto-impl entity mapping for
IntoIterator<Item = &'a Entity>types, as this would block our ability to implement cases likeHashMap. This means the onus is on us (or type authors) to add explicit support for types that should be mappable.Also note that the Component-related changes do not require a migration guide as there hasn't been a release with them yet.
Migration Guide
If you were previously implementing
VisitEntitiesorVisitEntitiesMut(likely via a derive), instead useMapEntities. Those were almost certainly used in the context of Bevy Scenes or reflection viaReflectMapEntities. If you have a case that usesVisitEntitiesorVisitEntitiesMutdirectly, whereMapEntitiesis not a viable replacement, please let us know!