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

Motion Input enhancements #8440

Open
wants to merge 3 commits into
base: master
from

Conversation

@rlnilsen
Copy link
Contributor

rlnilsen commented Oct 29, 2019

Small enhancements to PR #8352.

Motion Input tab

  • Apply default accelerometer and gyroscope bindings (as shown) when loading an old input configuration.
    Reason: If there exists a WiimoteNew.ini from an older Dolphin version, those bindings will be blank, and motion input won't work. You would have to either manually set each of the 12 controls (which is not hard, just time consuming) or press the Default button and lose all other bindings.
    Details: This was accomplished by adding versioning to input config files, including input profiles.
  • Add checkbox for enabling/disabling the motion controlled pointer. This is useful if you want to control the pointer by other means, like a mouse or the DualShock 4's touch pad.

2019-11-01 03_38_00-vs2017 (1) - Remote Viewer - Copy

DSU Client config

  • Enable Server IP Address and Server Port fields only when Enable is checked.

2019-10-29 21_39_37-vs2017 (1) - Remote Viewer

@rlnilsen rlnilsen force-pushed the rlnilsen:motion-input-tweaks branch from 8dfe679 to bc0fbaf Oct 30, 2019
@rlnilsen rlnilsen changed the title [WIP] Motion Input enhancements Motion Input enhancements Oct 30, 2019
@jordan-woyak

This comment has been minimized.

Copy link
Member

jordan-woyak commented Oct 30, 2019

I think I liked the "Enable" checkbox you had better than the floating mystery checkbox in the title.

@rlnilsen

This comment has been minimized.

Copy link
Contributor Author

rlnilsen commented Oct 30, 2019

This PR is now ready for review.

Since there are a few UI changes, perhaps @MayImilae and/or @JMC47 could take a look?

Also @mbc07 as the originator of most of these additions.

@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Oct 30, 2019

The UI additions look fine to me.

My one worry is when we add Nunchuck support to this, how it'll fit in, but that was discussed in the previous PR.

@rlnilsen

This comment has been minimized.

Copy link
Contributor Author

rlnilsen commented Oct 30, 2019

I think I liked the "Enable" checkbox you had better than the floating mystery checkbox in the title.

@jordan-woyak is referencing a WIP version of this PR that looked like this:
2019-10-29 16_50_06-vs2017 (1) - Remote Viewer - Copy - Copy

@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Oct 30, 2019

I have to disagree, personally, but to each their own. I don't care enough about the small details to really argue one way or the other.

@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Oct 30, 2019

Add Reset to Default buttons for Accelerometer and Gyroscope controls. If there exists a WiimoteNew.ini from an older Dolphin version, those controls will be blank, and motion input won't work. Without these new Reset to Default buttons, you would have to either manually set each of the 12 controls (which is not hard, just time consuming) or press the Default button and lose all other bindings.

I know it was already talked about but I didn't really voice my thoughts before. I think this is a bit weird. The reset buttons are not on any of our other sections and it's odd to me to include them here.

I do understand the reasoning, just wonder if there's a way to avoid it. Here's just a stab at one potential alternative:

Instead of the default buttons, if you click the "Enable" button under DSU Client, it will notice if the input controller(s) has any empty assignments. If so, it will ask the user if they wish to update them with the defaults.

More generally, this is a problem we've seen before and I feel like we need to figure out a way to "upgrade" user configuration without requiring UI or other solutions (like the above). Probably a good discussion for IRC

@mbc07

This comment has been minimized.

Copy link
Contributor

mbc07 commented Oct 30, 2019

Ok, let's review:

  • Changes on Alternate Input Sources window LGTM
  • Regarding the IMU Pointer, I think we're almost there:
    • I agree with @jordan-woyak that a "floating" checkbox on the header of the section is odd. Also it would be inconsistent as nowhere else on our UI we have a section with a checkbox on its header. Better having an individual checkbox inside the section like in the previous WIP.
    • However, for consistency, I think we could follow the same design pattern of the DSU Client window: The "Enable" (instead of "Enabled") checkbox should be the first item, followed by Recenter and Total Yaw.
    • To better communicate their relation to the user, Recenter and Total Yaw items should remain in disabled state until the "Enable" checkbox is selected.
    • While we're at it, perhaps consider renaming this group from "Point" to "IMU Pointer", "Emulated Pointer" or anything along those lines, to make clear it's not the same "Point" present on the "Motion Simulation" tab.
  • Regarding the "Reset to Default" buttons added to "Accelerometer" and "Gyroscope" sections:
    • Yes, it was me who asked for them, but seeing how they look now that it's implemented, indeed they seem odd and out of place.
    • @iwubcode suggestion to this seems more reasonable, but perhaps it doesn't need to be that verbal. Unless I missed something, the only situation where these bindings will be blank is when an user is updating from an older version of Dolphin or when loading a saved profile made with old Dolphin versions.
    • So, based on that, make Dolphin check if the Accel/Gyro bindings are blank on the moment the user enable DSU Client; if yes, just silently set them to the default values and be done. If any of the bindings isn't blank, assume the user has previously manually configured them (or that the default bindings already are defined) and do nothing.
    • Finally, get rid of the new "Reset to Default" buttons.
@rlnilsen

This comment has been minimized.

Copy link
Contributor Author

rlnilsen commented Oct 30, 2019

After looking through the code, it seems straightforward to add versioning to input config ini files (including profiles). This would allow detecting a "pre Motion Input" input configuration, allowing automatically resetting the Accel/Gyro bindings to default values. Similar situations in the future would also be covered.

Are there any reasons not to take this route?

@rlnilsen rlnilsen force-pushed the rlnilsen:motion-input-tweaks branch from bc0fbaf to a6598be Nov 1, 2019
… fields only when "server enable" is checked.
@rlnilsen rlnilsen force-pushed the rlnilsen:motion-input-tweaks branch from a6598be to 9e36094 Nov 1, 2019
@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Nov 1, 2019

Personally I think versioning should not be specific to inputs, it should be available for all configuration and there should be a central version value. Not sure where it would be best to go. Maybe IniFile?

A couple of days ago we talked about having a config validator/fixer that happens during startup and that could do the changes to whatever config files we need to make modifications to. We also did talk about a rollback/roll-forward feature but probably fine to start simple..

EDIT:

Thinking more, it's probably fine to start here. We can move the metadata concept to all ini files in the future and take our time coming up with a solid configuration upgrade solution.

@rlnilsen

This comment has been minimized.

Copy link
Contributor Author

rlnilsen commented Nov 1, 2019

I have now implemented most of the things discussed, but with a different solution for the accel/gyro bindings issue. The Point group enable checkbox was moved from the header into the group body. Please see the updated PR description for details and screenshots.

@mbc07 suggested some new names for the Point group on the Motion Input tab to separate it from the group of same name on the Motion Simulation tab. I wasn't really able to find a name that I liked, so I left it alone.

Personally I think versioning should not be specific to inputs, it should be available for all configuration and there should be a central version value.

There's some layering stuff going on with non input config files that I didn't want to deal with. I hope this implementation is good enough for the current needs, and that a more complete solution can be devised in the future if necessary.

@mbc07

This comment has been minimized.

Copy link
Contributor

mbc07 commented Nov 1, 2019

I can't really comment about the code as I'm very unfamiliar with Dolphin's config system at this level, but all UI changes looks good to me...

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Nov 1, 2019

I noticed that the IR emulation added in the previous PR doesn't take the Sideways/Upright Wii Remote settings into account. In terms of what this would represent on real Wii Remote, it's as if the IR camera moves from one side of the Wii Remote to another when you enable Sideways Wii Remote. Should this be changed? There are some games that reply on the IR camera not pointing at the sensor bar when holding the Wii Remote sideways, like Metroid: Other M.

@rlnilsen

This comment has been minimized.

Copy link
Contributor Author

rlnilsen commented Nov 1, 2019

I noticed that the IR emulation added in the previous PR doesn't take the Sideways/Upright Wii Remote settings into account.

It was done like this on purpose, after this recommendation from @jordan-woyak:

I'd vote to not touch the IR data as it's just going to confuse people if they can't point normally

I personally don't know enough to have an opinion.

@jordan-woyak

This comment has been minimized.

Copy link
Member

jordan-woyak commented Nov 1, 2019

@JosJuice Dolphin has never made sideways/upright affect IR data. We pretend the camera is always sticking out towards the TV in any orientation.

re: Metroid: Other M: The "Hide" mapping accomplishes the pointing vs. not pointing. I think it would confuse users to require they disable "Sideways Wii Remote" to do pointing. Consider any sideways remote game that has a pointer. e.g. Mario Kart Wii menus would be harder to navigate when playing without an extension.

Potentially we could make the "Upright" setting affect IR data. I think most games will not present a pointer with conflicting accelerometer data, anyways. But for consistency we've made neither orientation affect IR.

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Nov 1, 2019

Fair enough. I'm fine with leaving it the way it is unless we get comments about it from users.

@rlnilsen rlnilsen force-pushed the rlnilsen:motion-input-tweaks branch 2 times, most recently from 8527370 to a5a47bf Nov 2, 2019
@rlnilsen

This comment has been minimized.

Copy link
Contributor Author

rlnilsen commented Nov 2, 2019

@BhaaLseN @iwubcode Is the code review satisfactory?

If so, I think this PR should be merge ready.

@rlnilsen rlnilsen force-pushed the rlnilsen:motion-input-tweaks branch from a5a47bf to 35796a2 Nov 2, 2019
@rlnilsen

This comment has been minimized.

Copy link
Contributor Author

rlnilsen commented Nov 2, 2019

After starting work on motion input support for the Nunchuk, I realized that my proposed solution for the accel/gyro bindings issue needs more thought. I therefore withdraw the related commits from this PR, to not delay the other issues fixed.

If there are no more code review issues, this PR should be merge ready.

@rlnilsen rlnilsen force-pushed the rlnilsen:motion-input-tweaks branch from 35796a2 to 72eeaec Nov 2, 2019
@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Nov 2, 2019

Just tested this. Controller profiles don't seem to save the "enabled" state for point movement. This is mostly an inconvienance but also might be surprising to some users (if the user is like me, it'd be when using game-specific input profiles)

@rlnilsen

This comment was marked as outdated.

Copy link
Contributor Author

rlnilsen commented Nov 2, 2019

Controller profiles don't seem to save the "enabled" state for point movement.

It seems to work fine here. Like other settings, it is not written to file when it has the default value, which is true. Perhaps this is what you experienced?

Nevermind, I see it now.

@rlnilsen rlnilsen force-pushed the rlnilsen:motion-input-tweaks branch from 72eeaec to f14e609 Nov 3, 2019
@rlnilsen

This comment has been minimized.

Copy link
Contributor Author

rlnilsen commented Nov 3, 2019

Controller profiles don't seem to save the "enabled" state for point movement.

Fixed, the UI was not updated after loading a profile. I should not have missed that, sorry.

(Crossing my fingers hoping this PR is ready now.)

@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Nov 3, 2019

Thanks, the controller profiles now work well. One minor thing, when I disable the point controls and close Dolphin, if I open Dolphin back up and go to the configuration, the controls are enabled despite the "enabled" checkbox not being checked.

rlnilsen added 2 commits Oct 30, 2019
The setting is exposed as a check box in the QGroupBox instance that visualises the ControlGroup instance.
The setting is saved under "[control group name]/Enabled", but only when it is "false". The default value is "true".
@rlnilsen rlnilsen force-pushed the rlnilsen:motion-input-tweaks branch from f14e609 to f7a5054 Nov 3, 2019
@rlnilsen

This comment has been minimized.

Copy link
Contributor Author

rlnilsen commented Nov 3, 2019

One minor thing, when I disable the point controls and close Dolphin, if I open Dolphin back up and go to the configuration, the controls are enabled despite the "enabled" checkbox not being checked.

Fixed.

Copy link
Contributor

iwubcode left a comment

The code seems fine and everything tested works great. I say this LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.