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

Input cleanup 2 #9689

Merged
merged 4 commits into from May 19, 2021
Merged

Input cleanup 2 #9689

merged 4 commits into from May 19, 2021

Conversation

Filoppi
Copy link
Contributor

@Filoppi Filoppi commented May 4, 2021

Skimming of: #9489

A few fixes/improvements/cleanup for input related stuff, nicely described in commits.
Separated from #9688 because this one actually has behavioural changes and needs to be reviewed.

Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss left a comment

Choose a reason for hiding this comment

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

Looks fine otherwise, as far as I can tell anyway.

@AdmiralCurtiss
Copy link
Contributor

Oh, actually, two typos in commit messages that should probably be fixed!

Similar to the guitar, only control[0] was check, and that feels random. -> checked
make GetState update optinional -> optional

@Filoppi Filoppi force-pushed the input_cleanup_2 branch 2 times, most recently from af30044 to 32fad99 Compare May 18, 2021 09:14
@Filoppi
Copy link
Contributor Author

Filoppi commented May 18, 2021

Oh, actually, two typos in commit messages that should probably be fixed!

Similar to the guitar, only control[0] was check, and that feels random. -> checked
make GetState update optinional -> optional

done everything

@Filoppi Filoppi force-pushed the input_cleanup_2 branch 2 times, most recently from 7fc483e to 5004a2a Compare May 18, 2021 09:17
Copy link
Member

@leoetlino leoetlino left a comment

Choose a reason for hiding this comment

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

Changes look fine. Could you rebase this on master to fix the Windows build failures?

only control[0] was checked, and not one, which seems random.
Either both or none should be checked.
Similar to the guitar, only control[0] was checked, and that felt random.
casting a value to a u32 when it's originally an int, and it's exposed as int to users,
could end up in cases where a negative number would result as a positive one.
This doesn't really affect the value range of the attachment enum,
still I think the code was wrong.

Heavily tested.
…t, clean code

My future PRs will split the UI state from the Emulation State of some of these emulated
controller values and this readies the code for it.
@JMC47 JMC47 merged commit fbf7e93 into dolphin-emu:master May 19, 2021
10 of 11 checks passed
@Filoppi Filoppi mentioned this pull request Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants