Skip to content

Conversation

@molikto
Copy link
Contributor

@molikto molikto commented Jan 4, 2026

Objective

  • When saving a DynamicScene, should be able to make the Entitys more stable by providing the reverse entity map.

Solution

  • add with_entity_map to DynamicSceneBuilder

Testing

  • added unit tests

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Scenes Serialized ECS data stored on the disk D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 4, 2026
Copy link
Contributor

@kfc35 kfc35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic looks good. Thanks for the test.

What’s the need to have this functionality? Just curious as someone who is new to scenes. I’m assuming it makes it easier to compare two scenes if you’re changing something in between extractions?

I don’t have much context to scene work to determine whether this is controversial or anything, so that’s a caveat!

component_filter: SceneFilter,
resource_filter: SceneFilter,
original_world: &'w World,
entity_map: Option<&'w mut EntityHashMap<Entity>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Not sure if anyone is a fan of too verbose names, but I wonder if the name world_to_scene_entity_map or world_to_scene_map would be better here. I appreciate how succinct entity_map is though, so I may be in the minority


// Map entities in the component if an entity map is provided
if let Some(entity_map) = self.entity_map.as_mut()
&& let Some(map_entities) = type_registration.data::<ReflectComponent>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it more accurate for this to be named like so?

Suggested change
&& let Some(map_entities) = type_registration.data::<ReflectComponent>()
&& let Some(refl_component) = type_registration.data::<ReflectComponent>()

.find(|e| e.entity == original_entity.entity)
.expect("Entity should be present in recreated scene");

assert_eq!(original_entity.entity, recreated_entity.entity);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this is a redundant check since recreated_entity will assert that there is true for at least one of the &original_scene.entities?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Scenes Serialized ECS data stored on the disk C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants