Trouv / bevy_ecs_ldtk

ECS-friendly ldtk plugin for bevy, leveraging bevy_ecs_tilemap
Other
664 stars 75 forks source link

Use the default instance of LdtkEntity, not of its fields #119

Closed HugoPeters1024 closed 11 months ago

HugoPeters1024 commented 1 year ago

I've run into the issue where it is not clear to me at which point I should define with what values I want to spawn my entities. For example, I have a block blundle with a SpriteSheetBundle, a rapier 2d Collider and RigidBody, as well as a general marker struct:

#[derive(Bundle, LdtkEntity)]
struct BlockBundle {
    block: Block,

    body: RigidBody,

    collider: Collider,

    #[bundle]
    #[sprite_sheet_bundle]
    sprite: SpriteSheetBundle
}

I can very conveniently derive LdtkEntity because each individual field has the Default trait. However, should I want to configure the body to generally be RigidBody::Fixed it is not enough to simply implement a Default trait for the BlockBundle itself, since this is not considered by the deriving machinery of LdtkEntity.

Therefore I was wondering if there is already an anticipated path that accomplishes what I want (other than creating an on create system that corrects these values, that feels fragile and wasteful to me) and if not, to suggest requiring a Default trait on the bundle itself and using that to derive LdtkEntity. If users do not which to alter the beheavior they can simple derive Default along with LdtkEntity and continue enjoying the semantics of the current situation.

Happy to hear anyone's thoughts and my compliments to all who contributed, it is a great project :)

(P.S. should there be consensus about implementing my suggestion, I'd be happy to give it a shot and gain some experience)

Trouv commented 1 year ago

This is a good suggestion. Using the bundle's Default vs the Default of the individual components is not a decision that we should make lightly.

As for what you can do now - you can always manually implement LdtkEntity rather than using the derive impl. It's a bit verbose though due to the method signature, which is annoying.

For the sake of modularity, I find myself implementing From<EntityInstance> or LdtkEntity for individual components, and then giving them the #[from_entity_instance] or #[ldtk_entity] attributes respectively. However, I've never found this solution to be very ergonomic or enjoyable, as you technically should be checking the value of the entity's identifier in these manual implementations, especially if you intend to use that component in multiple bundles.

I don't really remember why I decided to use the Default impl of the individual fields rather than the Default impl of the bundle. I do remember explicitly making that decision though. There must have been some reason I went the way I did, but I probably didn't consider the value potential you've brought up here. I'll give this some more thought.

elenakrittik commented 1 year ago

Hello. Any update on this? Seems like i ran into the exact same issue as the OP.

Intuitively i though that the note about using defauls of bundles' fields is kind of a typo and tried impling Default for my player bundle to set it's rigid body as kinematic, but that (obviously) didn't work. I made a few attempts to do the same, but ultimately re-read docs and "got" the point. I tried impling Default for a "collision bundle" that contained the rigid body component afterwards and then including that bundle as a field in the player bundle, but for some reason that didn't work either (maybe bundles are flattened before being processed?). Again, i tried some variations and none of them worked, and now with a thought around lines of "surely it's me being an egghead once again", i go to the issue tracker and find this.

Thanks to your suggestion to manually impl LdtkEntity i was finally able to make my player not infinitely fall due to being RigidBody::Dynamic, but i still think that using bundle's Default impl should be at least an opt-in feature, if not the default behavior. If you can give me a hint on where the current behavior is located (i tried looking up, but got lost pretty quickly), i can submit a PR. TIA.

EDIT: Looks like i found it: https://github.com/Trouv/bevy_ecs_ldtk/blob/59618fe2f406caddd433ec435cff0a2156775c5c/macros/src/ldtk_entity.rs#L101

EDIT: Published a crate that provides a temporary solution for this: https://crates.io/crates/bevy_ecs_ldtk_default

Trouv commented 1 year ago

Yeah, after some thought, using the bundle's default, not the individual fields, is correct .

If you can give me a hint on where the current behavior is located (i tried looking up, but got lost pretty quickly), i can submit a PR.

That would be much appreciated

Trouv commented 11 months ago

Implemented in #222