Make the MapEntities trait generic over Mappers, and add a simpler EntityMapper #11428
Conversation
cBournhonesque
added
A-ECS
I agree that this is not acceptable; there must be a better solution, maybe something like: where Also I really don't like the names |
cBournhonesque
changed the title
| } | ||
|
|
||
| /// Similar to `EntityMapper`, but does not allocate new [`Entity`] references in case we couldn't map the entity. | ||
| pub struct SimpleEntityMapper<'m> { |
There was a problem hiding this comment.
It's never used inside Bevy, I would remove it and left its implementation to users.
There was a problem hiding this comment.
Yeah I'm mostly not sure on why you would want this behavior. If that can be clearly demonstrated with docs (and tested) I'd be fine to ship it.
There was a problem hiding this comment.
That's the behaviour I'd prefer to have for networking, as I don't want/need the complexities associated with the existing EntityMapper. (Having to use EntityMapper::world_scope() and needing access to &mut World)
I will remove this from the PR as users can implement it themselves
There was a problem hiding this comment.
Sounds good. I'd happily accept a PR to add it with docs, but I agree, it can be split out from this.
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
Just needs formatting now :) I'm going to merge this on Monday to leave time for other opinions to trickle in, but this should be in 0.13. |
|
Exciting! |
Co-authored-by: UkoeHB <[email protected]>
|
Applied the last suggestion: I think it was a modest improvement. Merging now :) |
# Objective Fixes: #11549 Add a doctest example of what a custom implementation of an `EntityMapper` would look like. (need to wait until #11428 is merged) --------- Co-authored-by: Charles Bournhonesque <[email protected]> Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Hennadii Chernyshchyk <[email protected]>
…tityMapper (bevyengine#11428) # Objective My motivation are to resolve some of the issues I describe in this [PR](bevyengine#11415): - not being able to easily mapping entities because the current EntityMapper requires `&mut World` access - not being able to create my own `EntityMapper` because some components (`Parent` or `Children`) do not provide any public way of modifying the inner entities This PR makes the `MapEntities` trait accept a generic type that implements `Mapper` to perform the mapping. This means we don't need to use `EntityMapper` to perform our mapping, we can use any type that implements `Mapper`. Basically this change is very similar to what `serde` does. Instead of specifying directly how to map entities for a given type, we have 2 distinct steps: - the user implements `MapEntities` to define how the type will be traversed and which `Entity`s will be mapped - the `Mapper` defines how the mapping is actually done This is similar to the distinction between `Serialize` (`MapEntities`) and `Serializer` (`Mapper`). This allows networking library to map entities without having to use the existing `EntityMapper` (which requires `&mut World` access and the use of `world_scope()`) ## Migration Guide - The existing `EntityMapper` (notably used to replicate `Scenes` across different `World`s) has been renamed to `SceneEntityMapper` - The `MapEntities` trait now works with a generic `EntityMapper` instead of the specific struct `EntityMapper`. Calls to `fn map_entities(&mut self, entity_mapper: &mut EntityMapper)` need to be updated to `fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M)` - The new trait `EntityMapper` has been added to the prelude --------- Co-authored-by: Charles Bournhonesque <[email protected]> Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: UkoeHB <[email protected]>
# Objective Fixes: bevyengine#11549 Add a doctest example of what a custom implementation of an `EntityMapper` would look like. (need to wait until bevyengine#11428 is merged) --------- Co-authored-by: Charles Bournhonesque <[email protected]> Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Hennadii Chernyshchyk <[email protected]>
Objective
My motivation are to resolve some of the issues I describe in this PR:
&mut WorldaccessEntityMapperbecause some components (ParentorChildren) do not provide any public way of modifying the inner entitiesThis PR makes the
MapEntitiestrait accept a generic type that implementsMapperto perform the mapping.This means we don't need to use
EntityMapperto perform our mapping, we can use any type that implementsMapper. Basically this change is very similar to whatserdedoes. Instead of specifying directly how to map entities for a given type, we have 2 distinct steps:MapEntitiesto define how the type will be traversed and whichEntitys will be mappedMapperdefines how the mapping is actually doneThis is similar to the distinction between
Serialize(MapEntities) andSerializer(Mapper).This allows networking library to map entities without having to use the existing
EntityMapper(which requires&mut Worldaccess and the use ofworld_scope())Migration Guide
The existing
EntityMapper(notably used to replicateScenesacross differentWorlds) has been renamed toSceneEntityMapperThe
MapEntitiestrait now works with a genericEntityMapperinstead of the specific structEntityMapper.Calls to
fn map_entities(&mut self, entity_mapper: &mut EntityMapper)need to be updated tofn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M)The new trait
EntityMapperhas been added to the prelude