Trouv / bevy_ecs_ldtk

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

`GridCoords` and `FieldValue` points using different origins #124

Open vivax3794 opened 1 year ago

vivax3794 commented 1 year ago

Problem

I have a entity that needs it grid cords and also has a component (ldtk attribute) that contains a vector of points.

image

now looking at that I can see that LDtk uses the upper left of the grid as 0, 0 (and +y going down), now if we just use the value of a FieldValue::Points directly that is what we get.

but if we use #[grid_coords], it uses the bottom left (with y+ going up), which is actually nice as it matches bevy, BUT the fact that these two are different is REALLY annoying to work around.

because if I want them to be the same I need to modify the value from the field based on the grid height, which I don't have access to in a From<EntityInstace> implementation, and I need it as my levels are of different heights.

I looked at this plugins source code, and I can see that it does a conversion for GridCoords: https://github.com/Trouv/bevy_ecs_ldtk/blob/66df96849a8bcd6cdf0f3a52271939a816d98087/src/utils.rs#L102-L104 https://github.com/Trouv/bevy_ecs_ldtk/blob/66df96849a8bcd6cdf0f3a52271939a816d98087/src/utils.rs#L116-L134

Possible solutions

Now I could implement the LdtkEntity trait manually, which would have access to all then needed information, but that seems like it is a bit overkill for something this simple that I feel like should be possible with the From<...> Implementation, there is #47 which might make this a bit simpler, but honestly I feel like the FieldValue::Points/FieldValue::Point should already use the same coordinate system as GridCoords

I understand this makes the implementation harder, as from a glance at the code the parsing of those seems to be deeply nested in Serde de-serialization, which means we cant access the world height at that point, meaning we would in theory have to iterate over the field values and "fix" this before constructing the entity

another solution is to allow components to have more access to the world, which is of course hard to do in a From implementation, but you could use a custom trait, or maybe even extend #47 to components .... somehow?

Trouv commented 1 year ago

Yeah, I think manually implementing LdtkEntity in general is a bit of a pain, and it's the only solution here. However, I feel like changing the way these values are serialized isn't good. I have some interest in honestly representing the LDtk data in those serde types, and swapping the coordinate direction in those types would not only be difficult (as you mentioned), but it would also "misrepresent" the LDtk data.

47 would probably help a lot but honestly I'm no closer to figuring out how to implement it than the day I filed it lol.

Since the introduction of "entity references" in LDtk 1.0, I've wanted to have some field instance support in the LdtkEntity derive macro. Something like #[field_instance("my_field_identifier")]. We could probably cover some common use cases, including logic for swapping the coordinate direction, in a macro like this.