Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Anchor Copy #9327

Merged
merged 2 commits into from
Aug 11, 2023
Merged

Make Anchor Copy #9327

merged 2 commits into from
Aug 11, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Aug 1, 2023

Objective

In bevy_sprite, the Anchor type is not Copy. It makes interacting with it more difficult than necessary.

Solution

Derive Copy on it. The rust API guidelines are that you should derive Copy when possible. https://rust-lang.github.io/api-guidelines/interoperability.html#types-eagerly-implement-common-traits-c-common-traits
Regardless, Anchor is a very small enum which warrants Copy.


Changelog

  • In bevy_sprite Anchor is now Copy.

@nicopap nicopap added C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 1, 2023
@alice-i-cecile
Copy link
Member

Definitely agree on Anchor, not totally sold on Sprite 🤔

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen and removed C-Feature A new feature, making something new possible labels Aug 1, 2023
@nicopap
Copy link
Contributor Author

nicopap commented Aug 1, 2023

The one objection I can see for Sprite is that it might in the future hold a Handle<Image>. I find it unlikely.

Otherwise it's a 68 bytes struct where each field themselves are Copy. GlobalTransform is 64 bytes for comparison.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Definitely agree on Anchor, not totally sold on Sprite 🤔

I'm going to mirror this concern. Adding Copy changes the semantics of the type, and any changes to revert this is going to be a very painful breaking change. @cart has mentioned multiple times that types should be Copy within the codebase if and only if you can treat the type as values (i.e. Vec2, Transform), and I don't think Sprites logically meet this criteria.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Aug 3, 2023
@cart cart changed the title Make Sprite and Anchor Copy Make Anchor Copy Aug 11, 2023
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Yeah I agree that Sprite should not be copy. I removed that derive.

@cart cart removed the X-Controversial There is active debate or serious implications around merging this PR label Aug 11, 2023
@cart cart enabled auto-merge August 11, 2023 20:46
@cart cart added this pull request to the merge queue Aug 11, 2023
Merged via the queue into bevyengine:main with commit b702811 Aug 11, 2023
20 checks passed
@nicopap nicopap deleted the text-anchor-copy branch August 30, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants