Fix for SceneEntityMapper + hooks panic #15405
Conversation
alice-i-cecile
added
S-Blocked
alice-i-cecile
changed the title
jrobsonchase
force-pushed
the
regression_test_14300
branch
2 times, most recently
from
4fc295e to
a8357f9
Compare
jrobsonchase
changed the title
jrobsonchase
force-pushed
the
regression_test_14300
branch
from
a8357f9 to
913e8cc
Compare
|
@alice-i-cecile Turned this into an actual fix in addition to the new test. Ready for review! |
jrobsonchase
force-pushed
the
regression_test_14300
branch
from
913e8cc to
eae723f
Compare
alice-i-cecile
added
S-Needs-Review
jrobsonchase
force-pushed
the
regression_test_14300
branch
from
eae723f to
7422176
Compare
alice-i-cecile
requested review from
janhohenheim,
Shatur,
cBournhonesque and
MrGVSV
| @@ -183,6 +183,9 @@ impl<'m> SceneEntityMapper<'m> { | |||
|
|
|||
| /// Creates a new [`SceneEntityMapper`], spawning a temporary base [`Entity`] in the provided [`World`] | |||
| pub fn new(map: &'m mut EntityHashMap<Entity>, world: &mut World) -> Self { | |||
| // We're going to be calling methods on `Entities` that require advance | |||
| // flushing, such as `alloc` and `free`. | |||
| world.flush_entities(); | |||
There was a problem hiding this comment.
Are there any perf consequences to flushing more often?
There was a problem hiding this comment.
Unsure. I assume that an unnecessary flush is cheap, or at least as inexpensive as checking if a flush is needed, but I don't know that for certain.
| let mut scene_world = World::new(); | ||
| scene_world.insert_resource(reg.clone()); | ||
| scene_world.spawn((B(Entity::PLACEHOLDER), A)); | ||
| let scene = DynamicScene::from_world(&scene_world); |
There was a problem hiding this comment.
Would it be possible to have a regression test that doesn't involved DynamicScene? I'm not super clear on the internals, but it looks like it might be possible to reproduce the issue with just A and B, and no DynamicScene?
There was a problem hiding this comment.
Technically, this can be tested entirely in bevy_ecs with SceneEntityMapper. I can add that one too 👍
There was a problem hiding this comment.
But is the Scene part important? Can't we trigger the panic just with MapEntities + hooks?
Oh i guess the only internal EntityMapper we have in bevy is SceneEntityMapper..
There was a problem hiding this comment.
Technically, this can be tested entirely in bevy_ecs with SceneEntityMapper. I can add that one too 👍
This would be very helpful to make sure it doesn't regress during the BSN work.
jrobsonchase
force-pushed
the
regression_test_14300
branch
from
7422176 to
d036d0f
Compare
alice-i-cecile
added
S-Ready-For-Final-Review
Objective
DynamicScene::write_to_worldwithMapEntitiesandComponentHooks#14300Fixes #14300
Solution
SceneEntityMapperrelies on operations onEntitiesthat require flushing in advance, such asallocandfree. Previously, it wasn't callingworld.flush_entities()itself and relied on its caller having flushed beforehand. This wasn't an issue before observers and hooks were released, since entity reservation was happening at expected times. Now that hooks and observers are a thing, they can introduce a need to flush.We have a few options:
The first option for this case seemed trickier to reason about than I wanted, since it involved the
BundleInserterand itsUnsafeWorldCell, and the second is generally harder to track down. The third seemed the most straightforward and conventional, since we can see a flush occurring at the start of a number ofWorldmethods. Therefore, we're lettingSceneEntityMapperbe in charge of upholding its own invariants and callingflush_entitieswhen it's created.Testing
Added a new test case modeled after #14300