Change ReflectMapEntities to operate on components before insertion #15422
Conversation
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
jrobsonchase
force-pushed
the
pre_map_entities
branch
from
ac52703 to
8a4c02d
Compare
alice-i-cecile
added
A-ECS
jrobsonchase
force-pushed
the
pre_map_entities
branch
from
8a4c02d to
9922071
Compare
Contributor
Author
|
I've rebased this on #15425 since it's gaining traction and I like its approach way better than what I had here previously. |
jrobsonchase
force-pushed
the
pre_map_entities
branch
7 times, most recently
from
51198ef to
55767d1
Compare
Contributor
Author
|
Welp, this has now been un-rebased on #15425 after realizing the approach there wouldn't work for |
Contributor
Author
|
@alice-i-cecile This is ready for review once more! |
jrobsonchase
force-pushed
the
pre_map_entities
branch
2 times, most recently
from
148b667 to
fe173a1
Compare
jrobsonchase
force-pushed
the
pre_map_entities
branch
from
e16c114 to
5315590
Compare
This makes EntityMapper object-safe and obviates the need for the `Dyn` variant.
alice-i-cecile
approved these changes
Oct 1, 2024
alice-i-cecile
added
the
M-Migration-Guide
Shatur
suggested changes
Oct 1, 2024
Contributor
Shatur
left a comment
Shatur
left a comment
There was a problem hiding this comment.
Looks good!
Left a small suggestion.
Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com>
Shatur
approved these changes
Oct 1, 2024
alice-i-cecile
added
S-Ready-For-Final-Review
robtfm
pushed a commit
to robtfm/bevy
that referenced
this pull request
Oct 4, 2024
…evyengine#15422) Previous PR bevyengine#14549 was closed in error and couldn't be reopened since I had updated the branch :crying_cat_face: # Objective Fixes bevyengine#14465 ## Solution `ReflectMapEntities` now works similarly to `MapEntities` in that it works on the reflected value itself rather than the component in the world after insertion. This makes it so that observers see the remapped entities on insertion rather than the entity IDs from the scene. `ReflectMapEntities` now works for both components and resources, so we only need the one. ## Testing * New unit test for `Observer`s + `DynamicScene`s * New unit test for `Observer`s + `Scene`s * Open to suggestions for other tests! --- ## Migration Guide - Consumers of `ReflectMapEntities` will need to call `map_entities` on values prior to inserting them into the world. - Implementors of `MapEntities` will need to remove the `mappings` method, which is no longer needed for `ReflectMapEntities` and has been removed from the trait. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Hennadii Chernyshchyk <genaloner@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Previous PR #14549 was closed in error and couldn't be reopened since I had updated the branch 😿
Objective
Fixes #14465
Solution
ReflectMapEntitiesnow works similarly toMapEntitiesin that it works on the reflected value itself rather than the component in the world after insertion. This makes it so that observers see the remapped entities on insertion rather than the entity IDs from the scene.ReflectMapEntitiesnow works for both components and resources, so we only need the one.Testing
Observers +DynamicScenesObservers +ScenesMigration Guide
ReflectMapEntitieswill need to callmap_entitieson values prior to inserting them into the world.MapEntitieswill need to remove themappingsmethod, which is no longer needed forReflectMapEntitiesand has been removed from the trait.