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 #9688

Merged
merged 9 commits into from May 14, 2021
Merged

Input cleanup #9688

merged 9 commits into from May 14, 2021

Conversation

Filoppi
Copy link
Contributor

@Filoppi Filoppi commented May 4, 2021

Skimming of: #9489

Nothing here should really make any behavioural difference over the code. It's just:
-const correctness
-improved commenting
-added a new log category for the controller interface
-follow coding conventions and grammar errors
-expose mapping bool tooltips
-extremely tiny fixes

Most commits don't overlap the same files, but I suggest looking at commits individually when reviewing.

@Filoppi Filoppi mentioned this pull request May 4, 2021
where appropriate. SerialInterface was a leftover from the past,
and makes no sense to be used on actual/real controllers.
Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

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

Some changes make sense, others appear a bit random; so I'm not completely sure what you meant with some of the changes and whether they even should be here.
Or I'm just misunderstanding what you're trying to do...

Source/Core/Core/HW/GCPadEmu.cpp Show resolved Hide resolved
@@ -44,7 +44,7 @@ void ControlGroup::AddDeadzoneSetting(SettingValue<double>* value, double maximu
// i18n: The percent symbol.
_trans("%"),
// i18n: Refers to the dead-zone setting of gamepad inputs.
_trans("Input strength to ignore.")},
_trans("Input strength to ignore and remap.")},
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if "dead zone" is also used to remap anything. Care to elaborate?

Copy link
Contributor Author

@Filoppi Filoppi May 5, 2021

Choose a reason for hiding this comment

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

The deadzone remaps the values after the deadzone.
If deadzone is 0.1 and you pass in 0.5, the outcome won't be 0.5, it will be 0.55, so there is a remap on going (applied differently depending on the kind of deadzone), just saying "ignore" is not enough in my opinion, it's actually misleading.
Same for the other comment.

Copy link
Member

Choose a reason for hiding this comment

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

While the change is technically correct, I think it's going to be hard for users to understand what you mean. I think you at the very least should expand the i18n comment to explain what's meant by remap, so that the string doesn't get mistranslated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that. If anyone has got a better word than remap, I'm happy to change it (maybe scale?). "Ignore" alone is technically wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize this also happened here, thanks for clarifying!

Source/Core/Core/HW/GCPadEmu.cpp Outdated Show resolved Hide resolved
Source/Core/InputCommon/InputConfig.h Outdated Show resolved Hide resolved
This isn't entirely necessary, as they are interpreted as barewords expressions,
but it's still nicer to have by default. And my upcoming input changes will
always put `` around single letter inputs.
@Filoppi Filoppi force-pushed the input_cleanup branch 2 times, most recently from df58505 to 942a4cf Compare May 6, 2021 08:54
@Filoppi
Copy link
Contributor Author

Filoppi commented May 6, 2021

@BhaaLseN Thanks for the review. I think I corrected or improved everything you have mentioned.

@Filoppi Filoppi force-pushed the input_cleanup branch 2 times, most recently from bf59ff9 to f29083d Compare May 9, 2021 08:45
@AdmiralCurtiss
Copy link
Contributor

Two commits still have typos in their commit messages. ("NumericSettings support a mas" and "fix some related grammar erros")

fix some related grammar errors
only the ButtonManager required code changes
@Filoppi
Copy link
Contributor Author

Filoppi commented May 10, 2021

Two commits still have typos in their commit messages. ("NumericSettings support a mas" and "fix some related grammar erros")

Fixed both. thx

NumericSettings support a max, so let's use it.
It might not do much now, but the max and min values will be used to give visual feeback
in the UI in one of my upcoming input PRs
Works exactly as before by default.
It will be used by my upcoming input PRs.
Tooltip code is identical to MappingDouble and the tooltips (UI description)
are present in the underlying setting object.
Copy link
Contributor

@JMC47 JMC47 left a comment

Choose a reason for hiding this comment

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

My controllers all work. I love the new log type actually. It was a good decision.

@phire phire merged commit 9f91fb6 into dolphin-emu:master May 14, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
9 participants