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

Observers and hooks doesn't pair well with mapped entities #14465

Closed
Shatur opened this issue Jul 24, 2024 · 1 comment · Fixed by #15422
Closed

Observers and hooks doesn't pair well with mapped entities #14465

Shatur opened this issue Jul 24, 2024 · 1 comment · Fixed by #15422
Labels
A-ECS Entities, components, systems, and events A-Networking Sending data between clients, servers and machines A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@Shatur
Copy link
Contributor

Shatur commented Jul 24, 2024

Bevy version

v0.14.0

What you did

I have a component that stores an entity. The component implements and reflects MapEntities, so on scene deserialization it automatically maps entities inside it.

What went wrong

When I implement on_add hook or OnAdd observer for such component, it triggers before MapEntities which results in invalid entity access.
It happens because we insert components here:

reflect_component.copy(
&self.world,
world,
scene_entity.id(),
*entity,
&type_registry,
);

But entities mapped later:
for registration in type_registry.iter() {
if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() {
map_entities_reflect.map_all_entities(world, entity_map);
}

I think we should map entities before the insertion.

@Shatur Shatur added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jul 24, 2024
@alice-i-cecile alice-i-cecile added A-Scenes Serialized ECS data stored on the disk A-Networking Sending data between clients, servers and machines A-ECS Entities, components, systems, and events S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Needs-Triage This issue needs to be labelled labels Jul 24, 2024
@Shatur
Copy link
Contributor Author

Shatur commented Jul 24, 2024

I would like to add a comment by @mockersf from Discord:

that's going to be not fun to fix, we need to add the entities before we can map them... so I guess the fix would be to spawn empty entities, do the mapping, then inserting the components...

@Shatur Shatur changed the title Observers doesn't pair well with mapped entities Observers and hooks doesn't pair well with mapped entities Jul 25, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 1, 2024
…15422)

Previous PR #14549 was closed in
error and couldn't be reopened since I had updated the branch
:crying_cat_face:

# Objective

Fixes #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>
robtfm pushed a commit to robtfm/bevy that referenced this issue 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
Labels
A-ECS Entities, components, systems, and events A-Networking Sending data between clients, servers and machines A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
Archived in project
2 participants