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

Add derives to mouse structs #270

Merged
merged 3 commits into from
Sep 6, 2020
Merged

Add derives to mouse structs #270

merged 3 commits into from
Sep 6, 2020

Conversation

aevyrie
Copy link
Member

@aevyrie aevyrie commented Aug 21, 2020

This is a proposed fix for issue #269
This adds Copy, as well as any other derives where possible.

@aevyrie aevyrie changed the title Add derives to mouse struct Add derives to mouse structs Aug 21, 2020
@karroffel karroffel added C-Code-Quality A section of code that is hard to understand or change A-Input Player input via keyboard, mouse, gamepad, and more labels Aug 21, 2020
@cart
Copy link
Member

cart commented Aug 21, 2020

This is a stylistic choice, but I think anything that is a "value type" should impl Copy. Ex: Vec2, MouseScrollUnit, and f32 all meet my definition of "value type".

I think by default, most structs don't meet my bar for Copy and should instead require explicit clones. MouseWheel and MouseMotion both have the potential to grow in the future (ex: with a device id field). Anything that has the potential to grow should be clone() unless theres a really good reason.

I'm happy to discuss it, but I think my preference here is to just impl Copy for MouseScrollUnit

@aevyrie
Copy link
Member Author

aevyrie commented Aug 23, 2020

I think by default, most structs don't meet my bar for Copy and should instead require explicit clones.

That's interesting! I thought that best practice was to implement Copy where possible because it can be much more performant than clone() in many cases. (Edit: the compiler can often determine when a clone should be a bytewise copy, but it is not a guarantee)

The major downside to implementing Copy, as far as I can see, is that if a non-Copy field is added to the struct in the future, it will cause a breaking change by forcing the removal of Copy and invalidating any code that relies on it instead of clone(). However, it sounds like breaking changes are a non-issue for Bevy right now.

For something that is probably being used in many places every frame, does the potential performance price of clone() matter to you? If not, I'll make the changes you requested!

@cart
Copy link
Member

cart commented Sep 2, 2020

Yeah you might be right about it being a performance optimization, but i would still prefer it if we made the change. Explicit clones make you think about type ownership, which I think is worth the potential perf cost (but its probably all in the noise anyway).

@aevyrie
Copy link
Member Author

aevyrie commented Sep 2, 2020

Sounds good! This is probably a case of premature optimization anyway. 😄

@cart
Copy link
Member

cart commented Sep 2, 2020

Build failures appear to be azure's mirrors of the ubuntu repos. We probably just need to wait a bit.

@cart cart merged commit 6e460a6 into bevyengine:master Sep 6, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants