build a map of entity iid to EntityInstance during asset load

[adtennant]

Simply to allow easier lookup for entities, e.g. from EntityRef fields.

It's a little brute-force and ideally the map should probably be a HashMap<String, &EntityInstance> to stop cloning the EntityInstance but bevy wasn't a fan of the lifetime - I also thought it was technically self-referencial but the compiler didn't seem upset by it.

It might be worth turning them into Handles but that seems a little overkill as there could be a lot inside a map and then it gives you the lookup against Assets<Handle<EntityInstance>> in any system that wants to use it.

[Trouv]

Thanks for the contribution! This is actually extremely similar to something I've been thinking about doing but haven't gotten around to - and I would prefer to implement just one of these solutions. My thought was to basically create a resource that maps entity iids to to that entity's indices in the asset. I was also thinking, in conjunction with this, there could be an EntityIid component added to entities that is treated as the hash. And finally, maybe a custom SystemParam that provides a convenient api for this sort of lookup instead of users having to deal with Assets<LdtkAsset> and this resource themselves.

#[derive(Clone, Debug, PartialEq, Eq, Hash, Deref, Component)]
pub struct EntityIid(String);

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct LdtkEntityIndices {
    world_index: usize,
    level_index: usize,
    layer_index: usize,
    entity_index: usize,
}

#[derive(Clone, Debug, PartialEq, Eq, Resource)]
pub struct LdtkEntityIndex {
    map: HashMap<Handle<LdtkAsset>, HashMap<EntityIid, LdtkEntityIndices>>,
}

#[derive(Clone, Debug, SystemParam)]
pub struct LdtkIndexer {
    assets: Res<Assets<LdtkAsset>>,
    index: Res<LdtkEntityIndex>
}

impl LdtkIndexer {
    // pub methods that provide convenient access *by reference* to levels, layers and entity instances for a given entity
    // some methods could be `_single()` and assume only one LdtkAsset exists since this is true for most users
}

The idea is that we won't be copying a decent sized type like EntityInstance that much, and that users can easily get access to the layer and level data an entity belongs to as well as the entity data itself. They already have easy access to these when spawning entities via the macro/registration system, but this makes it much easier for users that prefer the "blueprint" pattern of spawning entities to access this metadata, and for users to access this metadata well after spawning.

We could also maybe store an Option<Entity> on LdtkEntityIndices for making it easier for users to find already-spawned entities by entity iid. This will help considerably with #70.

What are your thoughts on this - do you think it would help you lookup entities in your use case?

[adtennant]

The approach you've suggested makes sense to me and would enable me to do what I need to. I think as a starting point just having an EntityIid component on every Entity spawned by the plugin would make a massive difference.

Are you looking for contributions in this area? I quickly threw up these PRs yesterday to kick off some discussions as I couldn't see a CONTRIBUTING file and frankly I knew they were half-baked.

Perhaps the EntityIid is a logical starting point as that isn't a breaking change and gives an immediate beenfit to people doing similar things.

[Trouv]

Sorry for the lack of CONTRIBUTING. I don't really have an official process for assigning tasks I suppose, I usually just make issues and assign them to myself when I begin working on them. Unfortunately, I don't have an issue filed for what I've just described here. That being said, I probably won't be doing feature work on this plugin for a couple weeks or so so feel free to work on it if you want to! No pressure of course, but contributions are always appreciated. EntityIid does sound like a good first step, I agree.

[adtennant]

Having quickly implemented adding EntityIid to everything, I am wondering if instead having EntityInstance not get removed when the type is mapped might be more elegant. It would open up the possibility of using Added<EntityInstance> in combination with the LdtkEntity trait as currently they are mutally exclusive.

That said, with the lookup approach then Added<EntityIid> opens up as a possibility, which means not needing to copy EntityInstance at all. On second thoughts - that's probably the better way of going.