Fix ignored entity pivot

[Koopa1018]

This PR modifies calculate_transform_from_entity_instance to use plain ldtk_pixel_coords_to_translation rather than the pivoted version. It also updates the function's test suite to reflect the different results it now gives.

Theoretically this PR will fix #290, but I've never worked with this codebase, so I have no idea if what I've done here is enough to actually fix the bug, nor do I know what else may need updating. (I know the enemies in the Platformer example show up displaced now.)

[Trouv]

Thanks for the contribution!

nor do I know what else may need updating

No worries. The examples definitely should be updated to match their previous behavior.

Also, since this is a breaking change, a new section in the migration guide is in order (at book/src/how-to-guides/migration-guides/migrate-from-0.8-to-0.9.md)

[Koopa1018]

Arright, I've done what I can with the examples. Most appear fine, and traitless was easy enough to fix, but I have no idea where to even look in the platformer example....
My only leads are that when the example begins running, the skulls slowly rise into the air as they move, until they reach this height:

and that the boss (who does not move) has a lowered hitbox:

Migration guide is going smoothly. Not totally sure how to demonstrate the changed translation/pivot behavior in a code example, though....

Also: I notice the CI format check failed. I've never contributed to a project with CI, so I'm not sure what that means, even after checking the details. Are these formatting issues that I need to fix?

[Trouv]

Also: I notice the CI format check failed. I've never contributed to a project with CI, so I'm not sure what that means, even after checking the details. Are these formatting issues that I need to fix?

Yeah it'll need to be fixed before this is merged. For the format check in particular, it's just a matter of running rustfmt: https://github.com/rust-lang/rustfmt

Though it's something small enough that I'd do it myself if that was the only issue preventing merge.

BTW, some unsolicited advice: I recommend setting up your editor to automatically run rustfmt on save. It's so handy writing a ton of dirty code then hitting save and watching it prettify before your eyes

[Koopa1018]

Can I get some help with the platformer example? It seems like to get the colliders to spawn right, it would require wider-reaching redesigns than I signed on for (that or I'm just totally missing a property that would fix everything...).

[Trouv]

Thanks again for your work on this. The scope has definitely increased a lot, sorry about that.

One of the comments in this review does address a particular strategy for updating the platformer example. However, it's a lot of work, so I don't expect you to do it necessarily. I can happily pick up wherever you leave off.

[Koopa1018]

Arright, so I reorganized the platformer example so I could make heads or tails of it. Now there's a concrete problem in front of me: I'm not sure how to spawn colliders as children within the LdtkEntity framework. If I had an Entity ref, no problem, but...

Think I'm gonna have to leave this as draft until #177 finally hits. Until then, might as well submit the reorganized platformer example as a separate PR so it doesn't become hell to merge down the road....

[Trouv]

Thanks for all your hard work on this. I have a lot more time for this project lately and I'm still interested in getting this PR in. Sorry for the wait.

It could use a rebase, your platformer refactor has gotten in in the meantime, and some of the migration guide stuff will need to refer to newer versions of the crate (0.13 and 0.14 instead of 0.9 and 0.10).

Now there's a concrete problem in front of me: I'm not sure how to spawn colliders as children within the LdtkEntity framework. If I had an Entity ref, no problem, but...

I'm not 100% sure what this has to do with this PR, is it a blocker? normally for things like this you do have to do post-processing instead of just LdtkEntity + macros

[Koopa1018]

Hey, sorry I took so long to get back to this!

I've been out of the Bevy ecosystem for a while. The new Sprite architecture (Anchor being a requirement of Sprite, rather than a field) threw me for a loop, since the anchor can no longer be assigned in Sprite's bundle_entity function...

I'm inclined to think the simplest way to make this work would be with a post-process function (that overwrites any freshly Added<Anchor>), but that also has the drawback of overwriting user-defined Anchors. I could design it to only overwrite non-default() Anchors, but that'd still overwrite any Anchor which is equal to default(). That'd be a weird, counterintuitive edge case; definitely not ideal to put that burden on users.
(EDIT: That'd also be undesirable for traitless spawning, wouldn't it....)

Branch currently has the post-process system, but I'd be happy to look into the derive macro and see what I can do to make anchors even more automatic.

(I also want to write a test case to verify that this postpro system actually works, but integration testing is something I'm not super comfortable with yet.)

---

W.r.t. spawning colliders as children. I believe the problem was that Rapier colliders couldn't be offset to align with pivot-adjusted sprites except by spawning them as children. I'll try post-processing systems.

[Koopa1018]

Dug into the macro. Looks like the best I can do is add an #[anchor] field attribute like the #[sprite] one. Would've liked to add a warning if a sprite has no anchor, but alas, manual compiler warnings are a nightly-only feature....

I suppose that might be for the best. If users have to manually tag their anchor fields, this whole pivot feature is effectively opt-in, which makes backwards compatibility lots easier.

Thinking of updating the macro and removing the post-process function. Thoughts?

[Trouv]

That's an interesting problem. There are some design decisions in this plugin that really need some rethinking since the introduction of required components.

I did think about your idea. If we were to do something like that I'd suggest a smaller change, adding some

pub fn ldtk_entity_anchor(entity_instance: &EntityInstance) -> Anchor {
    todo!()
}

to be used like

#[derive(Bundle, LdtkEntity)]
struct MyBundle {
    #[sprite_sheet]
    sprite: Spite,
    #[with(ldtk_entity_anchor)]
    anchor: Anchor,
}

However, I realized technically Transforms have a similar problem. You don't include them on the bundle but the system that spawns your entity adds it anyway, and assumes that you would want the transform to just be its position in the LDtk world (with some nasty adjustments). It could be made to make a similar assumption and add an Anchor. I suppose ideally it would only add the anchor if the entity already had a sprite component, but that might not be possible to tell in the position it's in. It would overwrite user-defined anchors, just like user-defined transforms, but that's not immediately a dealbreaker in my mind. Lmk your thoughts on this, I'll think about it some more too.

[Koopa1018]

Hmm, I hadn't considered using the existing annotations 😆

Looks like if we insert the anchor during spawn_level but before the call to evaluate(...), we leave users the ability to overwrite anchors by defining them in bundle_entity. Spriteless entities will still get an Anchor, which is ugly, but probably harmless? This is the method I'd prefer--it's automatic until the user needs to think about it, then steps out of their way when they need to override it.

Wondering if it might be worth it to make a custom EntityCommand to remove unneeded Anchor components...a post-process system would work, but it's difficult to guarantee that our post-process system would run Entirely Before or Entirely After user post-process systems.

If we don't want to do this, then it'd probably be easiest for users if we just impl LdtkEntity for Anchor and have them flag it #[ldtk_entity]:

#[derive(Bundle, LdtkEntity)]
struct MyBundle {
    #[sprite_sheet]
    sprite: Spite,
    #[ldtk_entity]
    anchor: Anchor,
}

...rather than making them remember which exact function to plug into #[with(...)].

Actually, I may end up doing impl LdtkEntity for Anchor anyway, just in case a user wants to be really explicit for some reason.

[Trouv]

Looks like if we insert the anchor during spawn_level but before the call to evaluate(...), we leave users the ability to overwrite anchors by defining them in bundle_entity.

Great point. I'm okay with this, and actually, it may make sense in the future to do that for Transforms as well. After all, the main reason I did transforms post-evaluate instead of pre-evaluate in the first place was because the old SpriteSheetBundle had a Transform on it. But now that it's a mere required component instead of part of the bundle added in evaluate, it too could be added pre-evaluate, and the plugin becomes slightly more flexible.

There may be some places in documentation where the behavior of Transforms is explained, may be worth explaining how Anchor also has similar behavior (but pre-evaluate). No need to make the change for Transform in this PR though, I'd prefer that as a separate small PR.