Skip to content

Input uses references when possible #14182

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Input uses references when possible #14182

wants to merge 3 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Jul 6, 2024

Objective

Solution

  • Use references when possible for input
  • Also update touches the same way for symmetry

Not using references is mostly not a problem in Bevy because most build in inputs are small types. It feels more correct to take references and it will be better for larger input types

@mockersf mockersf added A-Input Player input via keyboard, mouse, gamepad, and more C-Code-Quality A section of code that is hard to understand or change labels Jul 6, 2024
@mockersf mockersf force-pushed the input-uses-references-when-possible branch 2 times, most recently from 41b19fd to acf804a Compare July 6, 2024 14:09
@mockersf mockersf force-pushed the input-uses-references-when-possible branch from acf804a to a781fe6 Compare July 6, 2024 14:18
mnmaita
mnmaita reviewed Jul 6, 2024
View reviewed changes
Copy link
Member

@mnmaita mnmaita left a comment

Choose a reason for hiding this comment

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

Wondering if this needs a Migration note since some public signatures changed.

@mockersf
Copy link
Member Author

mockersf commented Jul 6, 2024

Wondering if this needs a Migration note since some public signatures changed.

yes it should...

But I'm not sure I like this PR. It's the right thing to do, but there soo many additional &...

@mockersf mockersf force-pushed the input-uses-references-when-possible branch from cdfe4f6 to 57c3dd9 Compare July 6, 2024 22:57
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through S-Needs-SME Decision or review from an SME is required M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jul 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 9, 2024

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@BenjaminBrienen BenjaminBrienen added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label May 17, 2025
@BenjaminBrienen
Copy link
Contributor

Has merge conflicts, waiting on migration guide

@BenjaminBrienen BenjaminBrienen added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label May 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@mnmaita mnmaita mnmaita left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-SME Decision or review from an SME is required S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mockersf @BenjaminBrienen @mnmaita @alice-i-cecile