search

Trouv / bevy_ecs_ldtk

ECS-friendly ldtk plugin for bevy, leveraging bevy_ecs_tilemap
Other
641 stars 73 forks source link

feat: add `get_entity_instance` to `LdtkAsset` #195

Closed adtennant closed 9 months ago

adtennant commented 1 year ago

This builds on the work in https://github.com/Trouv/bevy_ecs_ldtk/pull/194. I've split into two PRs in order to keep discussion focused, and because I think https://github.com/Trouv/bevy_ecs_ldtk/pull/194 in isolation still gives some benefits if this PR takes a little longer to get over the line. Unfortunately I can only set the base branch to one which exists in this repo (rather than my fork) so the diff includes all the other changes for now.

I went down this route rather than creating a SystemParam as discussed in https://github.com/Trouv/bevy_ecs_ldtk/pull/192 as it felt more natural for it to live alongside get_level and it massively simplifies the implementation.

I'm going to give some thought to how this can be extended to enable https://github.com/Trouv/bevy_ecs_ldtk/issues/70 as that is closely related to my actual use-case.

Questions

Trouv commented 1 year ago

I went down this route rather than creating a SystemParam as discussed in https://github.com/Trouv/bevy_ecs_ldtk/pull/192 as it felt more natural for it to live alongside get_level and it massively simplifies the implementation.

Hmm I see what you mean.

I'm going to give some thought to how this can be extended to enable https://github.com/Trouv/bevy_ecs_ldtk/issues/70 as that is closely related to my actual use-case.

Yeah, editing the asset data is a no-no, and the spawned Entitys for an ldtk world naturally changes as the levels load/unload. I think, going with the approach you've laid out here, having a separate feature where we map EntityIids to Entitys in a resource makes sense. Option<Entity> didn't really feel like it totally fit in EntityIndices when I originally suggested it anyway.

  • Should the traitless example be updated to use this? Full trait less will still work the same, but with this you can mix both approaches. It removes a potential footgun.

I think using this in an example is a good idea, but I'm fine with leaving that for a future PR. I'm not sure if traitless is the right one to update - its intention is to basically be a --no-default-features version of the basic example.

adtennant commented 1 year ago

@Trouv I’ll rebase this in top of main when I have some time tomorrow now that https://github.com/Trouv/bevy_ecs_ldtk/pull/194 is merged.

Do you have any feedback specific to these changes that needs addressing? Or would you rather wait until there is a clean diff and discuss further?

adtennant commented 1 year ago

Bumping this back to draft whilst we resolve what this should look like, @Trouv would you prefer me to create an issue to discuss this?

Trouv commented 9 months ago

Bumping this back to draft whilst we resolve what this should look like, @Trouv would you prefer me to create an issue to discuss this?

I've been reworking the asset types and this should be a little bit easier now. I finally created an issue for this and am closing this PR in favor of it: #251. Thanks again, @adtennant