Let Component::map_entities defer to MapEntities

[Testare]

Objective

The objective of this PR is to enable Components to use their MapEntities implementation for Component::map_entities.

With the improvements to the entity mapping system, there is definitely a huge reduction in boilerplate. However, especially since (Entity)HashMap<..> doesn't implement MapEntities (I presume because the lack of specialization in rust makes HashMap<Entity|X, Entity|X> complicated), when somebody has types that contain these hashmaps they can't use this approach.

More so, we can't even depend on the previous implementation, since Component::map_entities is used instead of MapEntities::map_entities. Outside of implementing Component and Component::map_entities on these types directly, the only path forward is to create a custom type to wrap the hashmaps and implement map entities on that, or split these components into a wrapper type that implement Component, and an inner type that implements MapEntities.

Solution

The solution was to allow adding #[component(entities)] on the component itself to defer to the MapEntities implementation

#[derive(Component)]
#[component(entities)]
struct Inventory {
    items: HashMap<Entity, usize>
}

impl MapEntities for Inventory {
    fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
        self.items = self.items
           .drain()
           .map(|(id, count)|(entity_mapper.get_mapped(id), count))
           .collect();
    }
}

Testing

I tested this by patching my local changes into my own bevy project. I had a system that loads a scene file and executes some logic with a Component that contains a HashMap<Entity, UVec2>, and it panics when Entity is not found from another query. Since the 0.16 update this system has reliably panicked upon attempting to the load the scene.

After patching my code in, I added #[component(entities)] to this component, and I was able to successfully load the scene.

Additionally, I wrote a doc test.

Call-outs

Relationships

This overrules the default mapping of relationship fields. Anything else seemed more problematic, as you'd have inconsistent behavior between MapEntities and Component.

API bikeshedding

I initially set out to make it so you could just add #[entities] to the type directly, which seemed to the most logical way to express this. However, all component derive attributes are nested in #[component(..)], I assume to prevent collisions, so I followed this for this attribute as well. I'm open to other ideas for attribute name though, or just using #[entities] instead.

[Shatur]

I like the idea, but I found that users are quite confusing by having separate mapping method inside Component and MapEntities in general.

Maybe we could just require MapEntities for Component? I remember that the mentioned downside was more complicated manual Component implementation, but maybe this change worth it in the end? 🤔

[Testare]

I don't think that would work because I don't think #[derive(Component)] can tell when you manually implement MapEntities, so either you would have to add #[derive(MapEntities)] to all Components that don't manually implement it (including ones that contain no entities at all), or you would have to automatically impl MapEntities for all of them, and I'm once again stuck without a method to implement custom map_entities behavior for components with EntityHashMap in them.

Perhaps the simplification could be that you add #[component(map_entities)] for all components that require Entity mapping, and it always defers to MapEntities's method instead of brewing its own (Or if the attribute is not present, it no-ops). Then the #[entities] tag can be used for the auto generated MapEntities trait instead if the user chooses to use that, or they can implement it themselves.

#[derive(Component)]
struct NeedsNoEntity(usize);

#[derive(Component, MapEntities)]
#[component(map_entities)]
struct AutomaticMapping(#[entities] Vec<Entity>);

// Above would work for relationships too

#[derive(Component)]
#[component(map_entities)]
struct ManualMapping(HashMap<Entity, Entity>);

impl MapEntities for ManualMapping {
  // ...
}

It is a little more work for the common case, but it might be less confusing than the separate implementations. Then again, it adds more boilerplate, and we're getting close to returning to #[derive(Reflect)] #[reflect(MapEntities)], which was the ergonomics that was changed in the first place.

[Shatur]

Ah, I see the problem!

I think #[component(map_entities)] looks a bit more clear to me. But I would like to hear more opinions 🤔

[cBournhonesque]

I also much prefer the version with #[component(map_entities)], I find the first version a little bit confusing.

[Testare]

To be clear, by the #[component(map_entities)] version, you're talking about the proposal in my last comment, not just bike shedding the name, right?

I'm going to assume so and I'll try to update the PR tomorrow

[Testare]

Doubts about #[component(map_entities)] approach (Plan B)

As I've been working on this PR, I realize that this second approach might be a bit of a footgun, especially with regard to relationship components. It seems to actually add quite a bit of boilerplate to simpler components, adding MapEntities derive AND a #[component(map_entities)] attribute. It's also now it's possible to do this:

#[derive(Component, MapEntities)]
struct EntityPtr(#[entity] Entity);

Which seems like it should work automatically, and including the MapEntities derive allows using the #[entity], but this actually won't work because #[component(map_entities)] is not present.

I've also come to appreciate in documentation for the current appraoch, how MapEntities is now implemented primarily on collection types, on the building blocks FOR components, rather than the components themselves. The average user doesn't need to worry about implementing another trait, just making sure their component is properly annotated and then saving/transfering components is sort of taken care of for them automatically. Put in other words, each Component implementing type is assumed to safe to save/reload, and MapEntities types are there to make it easy.

And finally, less importantly, but migration for this will be a bit of a headache. Compiler errors will warn when you use #[entities] without #[derive(MapEntities)] now, but that won't immediately tell them they also need to add #[component(map_entities)]. It also won't tell them anything for relationship components, which will now also need these things if these relationships are saved to a file or anything.

Of course, this major footgun is probably something the new bevy linter could help uncover. I think once I'm done with implementing the second approach I'll push it here so you can see for yourselves and see if you have less reservations about this approach than I do.

Alternatives

Plan C: #[entity_keys] and #[entity_values]

I think honestly the largest gap that caused me to try and implement this change was that map types (EntityMap and HashMap) don't support it.

I think probably the easiest way to do that is have a new trait like MapEntityKeys and MapEntityValues, and we can implement those for the types that need it.

impl <V> MapEntityKeys for HashMap<Entity, V> {
...
}

impl <K> MapEntityValues for HashMap<K, Entity> {
...
}

impl MapEntities for HashMap<Entity, Entity> {
...
}

Then we could just use #[entity_keys] and #[entiy_values] to indicate these!

#[derive(Component)]
pub struct Example {
   #[entity_keys] foo: HashMap<Entity, usize>,
   #[entity_values] bar: HashMap<String, Entity>,
   #[entity_keys] #[entity_values] foobar2: HashMap<Entity, Entity>,
   #[entities] foobar: HashMap<Entity, Entity>,
}

This preserves the separate purposes between Components and MapEntities. If we want to go with this plan, we could probably create a separate issue/PR for it.

Plan D: #[component(map_entities = <FN>)])

Perhaps there are components that still require more nuance to how they map. Instead of defering to a MapEntities implementation, we can manually specify a function we want to use for Component::map_entities using this pattern, similar to component hooks.

This also combines easily with the other plans: We could just have #[component(map_entities)] to use the MapEntities implementation by default, and we can still use #[entity_keys] and #[entity_values] for map types.

Aside

I also found out during writing this PR that MapEntities derives don't automatically include relationship targets. Is this intentional?

[Shatur]

Makes sense!
I would probably go with the plan D then 🤔