Skip to content

Altered the move_sprite_to method to correctly move a sprite which ha…#59

Merged
WhyPenguins merged 1 commit intothoth-tech:t2-2024from
matt439:develop
Sep 26, 2024
Merged

Altered the move_sprite_to method to correctly move a sprite which ha…#59
WhyPenguins merged 1 commit intothoth-tech:t2-2024from
matt439:develop

Conversation

@matt439
Copy link

@matt439 matt439 commented Jul 30, 2024

The move_sprite_to method produces incorrect behavior when a sprite has the Boolean field position_at_anchor_point set to true.

For example, take a 10x10 sprite which is positioned at world coordinates {10,10}. Its anchor is then {5,5} in sprite coordinates and {15,15} in world coordinates. Now when move_sprite_to(sprite, 20.0, 20.0) is called the sprite’s updated position is {25,25} with an anchor position of {30,30}.

With my alteration to the method, the sprite position would be {15,15} with an anchor position of {20,20}. This seems more natural as the sprite’s position is now being set by its anchor position, as intended by the position_at_anchor_point Boolean being true.

Copy link

@Wills2022 Wills2022 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested change in code, does not generate any errors, does not break unit tests and test code (specifically i tested "cave escape", sprite test & shape drawing. This does not appear to break the way that sprite rotation works in existing games, at least as far as test cases are concerned. It might cause some unusual behavior in user code if users have noticed this behavior and tried to correct it themselves. I do not believe that this is something we can realistically test for however.

Going forwards please feel free to modify the test code for the relevant class(es) if you modify them. I believe you mentioned writing tests during the standup, so please feel free to integrate them into the pull. Otherwise please include a (brief) description of your testing methodology in the pull.

@hugglesfox
Copy link

I agree that this logic makes a lot more sense. If you set the anchor position for a sprite then it's reasonable to expect that when moving the sprite it would be relative to the anchor position.

As Will mentioned, this will probably break some existing code however I believe it's better to update the broken code rather then have an unintuitive api design. I'll bring up with the leadership team to make sure that they are aware of the potential for existing projects to break.

Thanks for the PR, LTGM!

Copy link

@WhyPenguins WhyPenguins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! This logic definitely makes more sense, looks good to merge😄

@WhyPenguins
Copy link

Also just as a note in case it's useful for anyone looking at this area in the future, _sprite_datas also have a draw_at_anchor_point member, which looks like it was intended to offset the position of where the sprite is drawn when using an anchor point, rather than its actual position (in which case I suppose position_at_anchor_point would be turned off?).

The function to enable/disable this (sprite_set_draw_at_anchor_point), isn't exposed in the header, and so doesn't end up exported as part of the API. This PR may affect how that option works if it ever gets exposed properly.

@WhyPenguins WhyPenguins changed the base branch from develop to t2-2024 September 26, 2024 17:19
@WhyPenguins WhyPenguins merged commit 6bea471 into thoth-tech:t2-2024 Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants