Fix ignored entity pivot #294
Conversation
This is the basic ingredient needed to fix Trouv#290. There's a lot of things which will potentially break if this is done in isolation, so there's a lot more work ahead.
Comment it up, and also disambiguate use of the word "pivot." It always helps to do this when I'm first trying to understand a codebase.
Feels kind of silly testing behavior that's just "match the coordinates and flip the Y," but then again, the scale is worth testing.
There was a problem hiding this comment.
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)
Match usually implies multiple values need to be handled explicitly. When it's like this--everything or nothing--an if let expresses the intent better.
Now *this* is how the pivot should be used! Not placing the entity--placing the sprite *relative* to the entity :)
The whole point of this one is that it doesn't use the code I fixed. Have to apply the fix manually, then. (Confirmed working now.)
|
Arright, I've done what I can with the examples. Most appear fine, and 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? |
To ease migration process, a function which produces the same value as translation() did before the anchor/pivot fix.
This is a breaking change, and thus must be noted in the 0.8-0.9 migration guide. Draft because I can't quite figure out what to put in the first example.
Yeah it'll need to be fixed before this is merged. For the format check in particular, it's just a matter of running 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 |
I'm looking at what it did so I can update my coding style. I'm seeing: - Rust Analyzer's in-line type display misled me about how long line 88 and fn entity_center's definition were. They did not need to be shortened. - Indents -> spaces. (Imma let rustfmt handle that one 😅) - Ah, so *that's* how you split long if-lets up... - Do indent end-of-line comments to the same point. (In GDScript that's a no-no, so I'm used to avoiding that....)
|
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...). |
There was a problem hiding this comment.
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.
book/src/how-to-guides/migration-guides/migrate-from-0.8-to-0.9.md
Outdated
Show resolved
Hide resolved
book/src/how-to-guides/migration-guides/migrate-from-0.8-to-0.9.md
Outdated
Show resolved
Hide resolved
book/src/how-to-guides/migration-guides/migrate-from-0.8-to-0.9.md
Outdated
Show resolved
Hide resolved
Hate it when I miss this.
Co-authored-by: Trevor Lovell <trevorlovelldesign@gmail.com>
The anchor calculation depends on using the macro, as Trouv pointed out. And looking at my work later, I figured out how to make the center function clearer. Co-Authored-By: Trevor Lovell <trevorlovelldesign@gmail.com>
Because I can't imagine doing the conversions by hand every time would be at all easy on my sanity. Replace both existing instances of this code with calls to the new function, too, because code sharing is tight. Co-Authored-By: Trevor Lovell <trevorlovelldesign@gmail.com>
Doesn't do anyone any good unless they know it's there (or can guess, which, you never know...).
Co-Authored-By: Trevor Lovell <trevorlovelldesign@gmail.com>
Co-authored-by: Trevor Lovell <trevorlovelldesign@gmail.com>
…ecs_ldtk into fix/entity-pivot
For now, I'm putting the file in repo with the other image, knowing that it can be moved later. The image itself needs to be updated for clarity--I tested on my brother, and he was so distracted by the bright shiny object that he totally missed the tiny pivot dot.
Completely overlooked this function! Yes, these should probably also respect the pivot point. Understanding the macro it's used in took a bit, but thankfully the change wasn't too hard to apply once I did.
More indent->space fixes. I should probably change that in VSCode.
Asserts that 1. each axis is remapped from (0 - 1) to (-0.5 - 0.5) as is required for Bevy 2. Y is properly inverted.
uh...ok? Clearing out unneeded whitespace from a blank line....
Glad to find there actually was precedent for this, found in the book's tutorials folder.
Less contrasting entity graphic, bigger and more contrasting position reticule.
Missed 0.9, so it's gonna have to be in 0.10.
|
Alright, I've done what I can (including updating the migration guide to the new version). The rest is in your hands.... See you in 0.10 🖖 |
This PR modifies
calculate_transform_from_entity_instanceto use plainldtk_pixel_coords_to_translationrather than thepivotedversion. 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.)