Change ReflectMapEntities to operate on components before insertion #14549

jrobsonchase · 2024-07-31T12:09:27Z

Objective

Solution

Alters ReflectMapEntities to operate more like MapEntities by making it operate on the component itself rather than the World post-insertion. As a result, Observers never see the in-between state where the scene component has been added, but hasn't yet had its entities mapped.

Also brings it more in-line with MapEntities by accepting a generic DynEntityMapper rather than relying on the EntityHashMap and SceneEntityMapper implementation.

Testing

New unit test for Observers + DynamicScenes
New unit test for Observers + Scenes
Open to suggestions for other tests!

Migration Guide

The existing map_entities and map_all_entities methods have been renamed to map_world_entities and map_all_world_entities respectively and marked as deprecated.
- Figured I'd leave these in some form since the new method isn't exactly a drop-in replacement.
- ~~This is in a separate commit so that they'll be easy to fully remove later.~~ Edit: Oops, I forgot I did this and squashed it away. Oh well, should still be easy to remove them if/when desired.

github-actions · 2024-07-31T12:09:44Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

alice-i-cecile · 2024-07-31T12:59:05Z

Let me know when this is ready for review :) I'm going to want a regression test or two: the logic here is tricky but important to get right.

jrobsonchase · 2024-07-31T13:32:17Z

Hmm, I added a test which correctly fails before/succeeds after my change, but in the process I noticed that I'm now causing 'dynamic_scene::tests::components_not_defined_in_scene_should_not_be_affected_by_scene_entity_map' has overflowed its stack 🙃

jrobsonchase · 2024-07-31T13:40:58Z

Ope, I'm just an idiot 😅 Forgot to use the wrapped EntityMapper rather than the outer UnDynEntityMapper.

jrobsonchase

Mostly happy with this, apart from all of the extra component copying that ends up happening. We might be able to get rid of the outer one in the event that we don't have to map entities via some copy-on-write shenanigans, but I'm not sure about the one incurred from the call to the concrete FromReflect and the subsequent write back to the &mut dyn Reflect.

crates/bevy_ecs/src/entity/map_entities.rs

crates/bevy_ecs/src/identifier/mod.rs

jrobsonchase · 2024-07-31T13:46:10Z

crates/bevy_ecs/src/reflect/map_entities.rs

        });
    }
 }

-impl<C: Component + MapEntities> FromType<C> for ReflectMapEntities {
+impl<C: Component + MapEntities + Reflect + FromReflect> FromType<C> for ReflectMapEntities {


This is slightly more restrictive than previously, but I'm not sure that it really matters since anything reflecting MapEntities should already be Reflect + FromReflect.

I think it's fine. Ideally we'd only use Reflect and do a similar trick as ReflectComponent, but I’m not sure that's possible without world access.

World access shouldn't actually be a problem, at least for the two places we're currently using this. I actually had a &mut World parameter previously before swapping the EntityHashMap<Entity> for the &mut dyn DynEntityMapper.

Would also need access to the type registry, which also shouldn't be an issue, but then the arguments are starting to feel cluttered.

Yeah, works just fine. I could go either way 🤷

crates/bevy_scene/src/dynamic_scene.rs

jrobsonchase · 2024-07-31T13:49:23Z

crates/bevy_scene/src/scene.rs

+                    reflect_component.apply_or_insert(
+                        &mut world.entity_mut(entity),
+                        &*component,
                        &type_registry,
                    );


I think this is equivalent to the previous reflect_component.copy, but I'm not 100% certain. I don't use regular Scenes all that often. Or at all, really.

crates/bevy_ecs/src/reflect/map_entities.rs

MrGVSV · 2024-07-31T15:06:22Z

crates/bevy_ecs/src/reflect/map_entities.rs

+            map_entities: |component, entity_mapper| {
+                let mut concrete = C::from_reflect(&*component).unwrap();
+                concrete.map_entities(&mut UnDynEntityMapper(entity_mapper));
+                component.apply(&concrete as _);


The only possible concern here is if the user has mapped fields marked with #[reflect(ignore)]. The original implementation didn't rely on reflection so it was fine, but this is a new "hidden" requirement. We should probably document this somewhere.

I feel like that's expected? If a field is invisible to reflection in general, it seems logical that it would be invisible to reflected traits.

Hm fair point. And when used with scenes specifically I guess those fields won't even be serialized which means it's less of an issue than I originally thought.

crates/bevy_ecs/src/reflect/map_entities.rs

jrobsonchase · 2024-07-31T18:13:02Z

@alice-i-cecile Ready for review!

I'm satisfied that this change fixes the original scenes/observers/mapped entities issue, but I wonder if there isn't a more fundamental problem with observers and their immediacy. This fixes the issue of individual components being in an inconsistent state, but I think the overall scene will still be inconsistent when the observers fire, e.g. a Trigger<OnAdd, Parent> observer may still see an empty parent entity if the child is written before its parent.

alice-i-cecile · 2024-07-31T23:20:26Z

Agreed, there's something deeper here that's bothering me but I haven't figured out exactly what yet.

@cart's idea from #14520 (comment) to use a topologically sorted order of resolution might help this problem as well.

jrobsonchase · 2024-08-01T00:14:01Z

Yeah, that sounds like it would definitely help, but it also feels like another band-aid.

In my mind, the real issue is that observers very much look like systems, which have always run on either side of sync points and thus could never observe intermediate state. But in reality, observers can see this intermediate state since they run immediately. Things like cmd.entity(e).insert(Foo).insert(Bar) vs cmd.entity(e).insert(Bar).insert(Foo) didn't previously have much semantic difference, since all systems (and commands later in the queue) only ever saw the final state where they both existed. Queueing component hooks and triggers as extra Commands rather than immediate calls could be a possible solution with the added benefit of avoiding stack overflows resulting from too many nested triggers.

Edit: Delved a little more deeply, and I think I'm conflating a couple of issues. The stack overflow is due to how command application recurses on world.flush() rather than anything trigger-related, and the only observers that are immediately invoked are the On(Add|Remove|Insert|etc.) ones as far as I can tell. So in general, observers behave like systems and fire post-sync, or at least obey the expected queueing semantics, but the component lifecycle ones insert themselves mid-command-queue.

mrchantey · 2024-09-12T23:48:25Z

Can we plz schedule this PR for an upcoming milestone?
AFIK this is what allows us to deserialize scenes that involve MapEntities and Obsevers, which I guess will be increasingly common.
I believe it closes:

Panic calling DynamicScene::write_to_world with MapEntities and ComponentHooks #14300
- my one, currently scheduled for 0.14.3 milestone
Observers and hooks doesn't pair well with mapped entities #14465
Panic when a despawning Entity's observer spawns an Entity #14467
- to be confirmed whether its the same bug

jrobsonchase · 2024-09-16T15:20:00Z

Happy to get this rebased and mergeable, but I still think it's just a band-aid. Systems with exclusive World access and Commands shouldn't have to worry about how they order operations just to appease some observer, and observers shouldn't have to care about how things are ordered within a command/exclusive system. I expect exclusive World access to be, well, exclusive, and observers/hooks getting to run in the middle of them feels an awful lot like concurrent shared access. I get that it's all technically kosher according to the borrow checker's rules, but it still breaks the expected mental model. I wrote up a similar issue about mid-exclusive command application, but I think even mid-exclusive read-only observer/hook access is wrong.

alice-i-cecile · 2024-09-16T16:08:56Z

Yeah, I'd like to get this merged :) I disagree with you on "should hooks and observers operate during exclusive world access", but that's a conversation for another thread.

alice-i-cecile · 2024-09-16T19:29:26Z

@mrchantey can I please have your review here?

mrchantey

Extra test coverage looks great and its really well documented. I left one comment about documenting panics, not sure if thats our style :)

mrchantey · 2024-09-17T12:29:39Z

crates/bevy_scene/src/dynamic_scene.rs

+            // Fetch the entity with the given entity id from the `entity_map`.
+            let entity = *entity_map
+                .get(&scene_entity.entity)
+                .expect("should have previously spawned an empty entity");


I'm not 100% on Bevy docs style, are we meant to declare these panics in the function docs?

Yes, it would be nice to add a #Panics section to the docs.

My understanding is that panics only need to be documented if they can be triggered by the caller due to invariants not being upheld. That isn't the case for this panic as it signals a bug in the method internals.

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk labels Jul 31, 2024

jrobsonchase force-pushed the pre_map_entities branch 3 times, most recently from 8a82089 to f8552ce Compare July 31, 2024 12:56

alice-i-cecile added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 31, 2024

jrobsonchase force-pushed the pre_map_entities branch 2 times, most recently from f410d21 to d18b79c Compare July 31, 2024 13:30

jrobsonchase force-pushed the pre_map_entities branch from d18b79c to 0063b1b Compare July 31, 2024 13:39

jrobsonchase commented Jul 31, 2024

View reviewed changes

jrobsonchase force-pushed the pre_map_entities branch from 0063b1b to 13b2c68 Compare July 31, 2024 13:54

jrobsonchase marked this pull request as ready for review July 31, 2024 13:55

MrGVSV reviewed Jul 31, 2024

View reviewed changes

crates/bevy_ecs/src/reflect/map_entities.rs Outdated Show resolved Hide resolved

jrobsonchase force-pushed the pre_map_entities branch 5 times, most recently from 00acaaa to 31b8c97 Compare July 31, 2024 18:03

jrobsonchase force-pushed the pre_map_entities branch from 31b8c97 to d0a3349 Compare August 1, 2024 15:43

alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Aug 5, 2024

alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Aug 5, 2024

alice-i-cecile added this to the 0.15 milestone Sep 13, 2024

jrobsonchase force-pushed the pre_map_entities branch 2 times, most recently from b7a52aa to 6ed33e8 Compare September 16, 2024 16:36

mrchantey approved these changes Sep 17, 2024

View reviewed changes

jrobsonchase added 2 commits September 17, 2024 12:01

Add test for scene/observer/mapped entity interaction

0bc5226

Change ReflectMapEntities to operate on components before insertion

a8d1796

jrobsonchase force-pushed the pre_map_entities branch from 6ed33e8 to a8d1796 Compare September 17, 2024 16:01

alice-i-cecile requested a review from cart September 17, 2024 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change ReflectMapEntities to operate on components before insertion #14549

Change ReflectMapEntities to operate on components before insertion #14549

jrobsonchase commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024

alice-i-cecile commented Jul 31, 2024

jrobsonchase commented Jul 31, 2024

jrobsonchase commented Jul 31, 2024

jrobsonchase left a comment

jrobsonchase Jul 31, 2024

MrGVSV Jul 31, 2024

jrobsonchase Jul 31, 2024 •

edited

Loading

jrobsonchase Jul 31, 2024

jrobsonchase Jul 31, 2024

MrGVSV Jul 31, 2024

jrobsonchase Jul 31, 2024

MrGVSV Jul 31, 2024

jrobsonchase commented Jul 31, 2024

alice-i-cecile commented Jul 31, 2024

jrobsonchase commented Aug 1, 2024 •

edited

Loading

mrchantey commented Sep 12, 2024

jrobsonchase commented Sep 16, 2024 •

edited

Loading

alice-i-cecile commented Sep 16, 2024

alice-i-cecile commented Sep 16, 2024

mrchantey left a comment

mrchantey Sep 17, 2024

alice-i-cecile Sep 17, 2024

jrobsonchase Sep 17, 2024

Change ReflectMapEntities to operate on components before insertion #14549

Are you sure you want to change the base?

Change ReflectMapEntities to operate on components before insertion #14549

Conversation

jrobsonchase commented Jul 31, 2024 • edited Loading

Objective

Solution

Testing

Migration Guide

github-actions bot commented Jul 31, 2024

alice-i-cecile commented Jul 31, 2024

jrobsonchase commented Jul 31, 2024

jrobsonchase commented Jul 31, 2024

jrobsonchase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrobsonchase Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrobsonchase commented Jul 31, 2024

alice-i-cecile commented Jul 31, 2024

jrobsonchase commented Aug 1, 2024 • edited Loading

mrchantey commented Sep 12, 2024

jrobsonchase commented Sep 16, 2024 • edited Loading

alice-i-cecile commented Sep 16, 2024

alice-i-cecile commented Sep 16, 2024

mrchantey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrobsonchase commented Jul 31, 2024 •

edited

Loading

jrobsonchase Jul 31, 2024 •

edited

Loading

jrobsonchase commented Aug 1, 2024 •

edited

Loading

jrobsonchase commented Sep 16, 2024 •

edited

Loading