Trouv / bevy_ecs_ldtk

ECS-friendly ldtk plugin for bevy, leveraging bevy_ecs_tilemap
Other
677 stars 76 forks source link

feat: add `EntityIid` component which is added to all entities by default #194

Closed adtennant closed 1 year ago

adtennant commented 1 year ago

This is a first step on the road to having an easy way of looking up entity information from LdtkAsset.

Questions

Trouv commented 1 year ago
  • Should it be possible to disable this using LdtkSettings? I'm not sure it's worth worrying about as the memory usage should be minimal as the IID are always GUIDs. Perhaps a feature?

I think the LdtkIndexer stuff I mentioned makes sense to go behind a feature, but this EntityIid is small enough and generally-useful-enough that I think it's fine to make it non-optional.

  • Should this be a newtype? It makes sense to me and I can't see any reason for it to ever gain fields, but the other components within the crate all have named fields.

That's fine - I was pretty allergic to newtypes when I first wrote this plugin but no longer.

  • Should the EntityIid also carry its Handle<LdtkAsset>? I don’t think so personally as LevelSelection doesn’t have this concept.

I agree, it doesn't need it.

adtennant commented 1 year ago

@Trouv I've refactored components.rs into a folder, as I assume given time you'll want to break it down fully. It seemed neater than having entity_iid.rs floating around on its own.

adtennant commented 1 year ago

No worries about being pedantic, that is the point of doing code reviews 👍