Provide a MapEntities implementation that doesn't require mutable access to the EntityMapper

[cBournhonesque]

What problem does this solve or what need does it fill?

The MapEntities trait (https://github.com/bevyengine/bevy/blob/main/crates/bevy_ecs/src/entity/map_entities.rs#L46)

pub trait MapEntities {
    fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M);
}

requires a mutable access to the EntityMapper: &mut M

This is because the SceneEntityMapper might generate a new Entity, as seen here: https://github.com/bevyengine/bevy/blob/main/crates/bevy_ecs/src/entity/map_entities.rs#L81

but there are a lot of cases where you might want to just map an entity using an immutable HashMap, in which case you only need a &dyn EntityMapper.

I run into cases where the parallelism of my systems is lowered because I am forced to provide a mutable access to the EntityMapper even though a non-mutable access would be okay. I would appreciate having a way to call map_entities without requiring a mutable access to entity_mapper

What solution would you like?

Maybe the MapEntities trait could have 2 functions map_entities_mut and map_entities? I haven't thought this through.

[Shatur]

It would be convenient, but not sure how to solve it...
Having map_entities_mut and map_entities will require users to provide both implementations which is not convenient :(

[aristaeus]

+1 for this being convenient, I'm currently cloning EntityMapper to avoid having to think about how to satisfy the borrow checker in https://github.com/aristaeus/bevy_mod_desync

[aristaeus]

Thinking about this more, we can do something like Fn/FnMut in the standard library:

• Provide MapEntities and MapEntitiesMut, with a blanket impl<T: MapEntities> MapEntitesMut for T
• Rename all current usage to map_entities_mut etc (would result in a lot of short term churn)
• Users only need to implement one trait

I can prepare a PR if others think this is a good strategy

[cBournhonesque]

This seems like a good idea to me!
And for things like the SceneEntityMap, we would only implement the MapEntitiesMut trait?

[aristaeus]

Yes, exactly. I'll work on a PR in the next couple of days.

[Shatur]

And for things like the SceneEntityMap, we would only implement the MapEntitiesMut trait?

SceneEntityMap is a mapper, MapEntities(Mut) is for components.

I think splitting into two traits and providing a blanked imp won't solve the problem. All components should be used in scenes, so they all need MapEntitiesMut that call map_entity_mut on a mapper, so blanked impl that calls map_entity won't work for them. And if you implement MapEntitiesMut, you won't be able to implement MapEntities due to the blanked impl.

[aristaeus]

Yeah, I ran into some trouble trying this. Splitting EntityMapper and adding a blanket impl is easy, but there's no correct way to write down MapEntities(Mut).

I've noticed that components don't really care whether the entity mapper is mutable or not, so a more controversial option is to pad out the trait like so;

trait MapEntities {
    fn map_entities_internal(&mut self, entity_mapper: MutErasedEntityMapper);

    fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &M) {
        self.map_entities_internal(...);
    }

    fn map_entities_mut<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
        interior_mutableise_entity_mapper();
        self.map_entities_internal(...);
    }
}

Components can them impl map_entities_internal, and users can choose whether or not they need mutability in context. map_entities may need some way of conveying that mapping failed because an entity needed to be created and couldn't be?

[Shatur]

Interior mutability would solve the problem.
But you can keep a single non-mutable method and require mappers use interior mutability if needed.