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 relative input for the Wiimote IR #4010

Merged
merged 4 commits into from Oct 3, 2016

Conversation

leoetlino
Copy link
Member

@leoetlino leoetlino commented Jul 14, 2016

This adds an option to enable relative input for the Wiimote IR as described in issue 9014.

Enabling it will result in the pointer not going back to the centre and the inputs will control the direction, not the absolute position.

Also adds a Dead Zone setting which is really needed when relative input is enabled to prevent the cursor from slowly drifting on most controllers. (Note: the Deadzone setting has no effect when relative input is disabled, so the field is disabled when relative input is disabled)


This change is Reviewable

auto* const pad_setting = static_cast<PadSettingCheckBox*>(control->GetClientData());
pad_setting->UpdateValue();

// TODO: find a cleaner way to have actions depending on the setting

This comment was marked as off-topic.

This comment was marked as off-topic.

@lioncash
Copy link
Member

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.


Source/Core/DolphinWX/InputConfigDiag.cpp, line 114 [r1] (raw file):

  ((wxCheckBox*)wxcontrol)->SetValue(setting->GetValue());
  // Force WX to trigger an event after updating the value
  auto* const event = new wxCommandEvent(wxEVT_CHECKBOX);

This doesn't need to be allocated with new, you can just declare it as a value type variable.

What is this trying to accomplish, by the way?


Source/Core/InputCommon/ControllerEmu.h, line 378 [r1] (raw file):

    // This is used to reduce the cursor speed for relative input
    // to something that makes sense with the default range.
    static constexpr double SPEED_MULTIPLIER = 0.02;

This probably shouldn't be publicly exposed, since it's only used within this class.


Comments from Reviewable

@lioncash
Copy link
Member

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.


Source/Core/DolphinWX/InputConfigDiag.h, line 227 [r1] (raw file):

  void AdjustControlOption(wxCommandEvent& event);
  void EnableSettingControl(const std::string& group_name, const std::string& name,
                            const bool enabled);

const for bool (or value types in general) isn't necessary in the declaration of a function.


Comments from Reviewable

@lioncash
Copy link
Member

Review status: 0 of 4 files reviewed at latest revision, 5 unresolved discussions.


Source/Core/DolphinWX/InputConfigDiag.cpp, line 488 [r1] (raw file):

  // TODO: find a cleaner way to have actions depending on the setting
  if (control->GetLabelText() == "Iterative Input")

This will likely break for different language settings. We really shouldn't have code that depends on user-facing strings.


Comments from Reviewable

@leoetlino
Copy link
Member Author

Review status: 0 of 4 files reviewed at latest revision, 5 unresolved discussions.


Source/Core/DolphinWX/InputConfigDiag.cpp, line 114 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

This doesn't need to be allocated with new, you can just declare it as a value type variable.

What is this trying to accomplish, by the way?

Thanks, didn't know. This forces WX to call our AdjustBooleanSetting event handler after updating the value, so we can disable/enable the deadzone setting when relative input gets turned on or off. I'm sure there is probably a better way to do this, but I don't know enough about wxWidgets…

Source/Core/DolphinWX/InputConfigDiag.cpp, line 488 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

This will likely break for different language settings. We really shouldn't have code that depends on user-facing strings.

This is actually the internal name, i.e. the one we store in the configuration files. So it shouldn't depend on the locale at all. But imo this is still a bit fragile; but I couldn't find another easy way to have custom, setting-specific actions. (see above discussion)

Source/Core/DolphinWX/InputConfigDiag.h, line 227 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

const for bool (or value types in general) isn't necessary in the declaration of a function.

Done.

Source/Core/InputCommon/ControllerEmu.h, line 378 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

This probably shouldn't be publicly exposed, since it's only used within this class.

Oops. I have made this private.

Comments from Reviewable

@lioncash
Copy link
Member

Source/Core/DolphinWX/InputConfigDiag.cpp, line 114 [r1] (raw file):

Previously, leoetlino (Léo Lam) wrote…

Thanks, didn't know.
This forces WX to call our AdjustBooleanSetting event handler after updating the value, so we can disable/enable the deadzone setting when relative input gets turned on or off. I'm sure there is probably a better way to do this, but I don't know enough about wxWidgets…

You can likely use [wxPostEvent](http://docs.wxwidgets.org/trunk/group__group__funcmacro__events.html#ga0cf60a1ad3a5f1e659f7ae591570f58d) for that.

Comments from Reviewable

@lioncash
Copy link
Member

Source/Core/DolphinWX/InputConfigDiag.h, line 227 [r1] (raw file):

Previously, leoetlino (Léo Lam) wrote…

Done.

fwiw, You only needed to remove const from this specific declaration. You can leave the const in the definition where the actual code is if you want to enforce immutability.

Comments from Reviewable

@leoetlino
Copy link
Member Author

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.


Source/Core/DolphinWX/InputConfigDiag.cpp, line 114 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

You can likely use wxPostEvent for that.

Done. This doesn't remove the need for event.SetEventObject (as I'd have hoped), but I think using wxPostEvent makes the purpose clearer.

Source/Core/DolphinWX/InputConfigDiag.h, line 227 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

fwiw, You only needed to remove const from this specific declaration. You can leave the const in the definition where the actual code is if you want to enforce immutability.

I see. Thanks, I have left the const in the code.

Comments from Reviewable

@mbc07
Copy link
Contributor

mbc07 commented Jul 15, 2016

It would be nice to have the speed of movement adjustable when using relative IR (and maybe a "reset pointer to center" binding or hotkey?).

@leoetlino
Copy link
Member Author

You can actually already adjust the speed with the existing Range feature. Just right-click on each axis and increase the range to make it faster; decrease it to make it slower.
As for resetting it to the centre, hmm, I'm not too sure if/how this could be done. A binding would be yet another control to add to the dialog; a hotkey would perhaps be confusing if relative input isn't enabled? For now, you can reset it by disabling then re-enabling relative input.

@mbc07
Copy link
Contributor

mbc07 commented Jul 15, 2016

Whoops, I forgot we had a range option in advanced settings. About recentering, I think a new binding which remains grayed out unless Relative Input is enabled should do the trick...

private:
// This is used to reduce the cursor speed for relative input
// to something that makes sense with the default range.
static constexpr double SPEED_MULTIPLIER = 0.02;

This comment was marked as off-topic.

This comment was marked as off-topic.

@mbc07
Copy link
Contributor

mbc07 commented Jul 17, 2016

Sweet! This PR now looks perfect on user-experience side!

@mbc07
Copy link
Contributor

mbc07 commented Sep 5, 2016

Err... ping?

This adds an option to enable relative input for the Wiimote IR
as described in issue 9014.

Enabling it will result in the pointer not going back to the centre
and the inputs will control the direction, not the absolute position.

Also adds a Dead Zone setting which is really needed when relative
input is enabled to prevent the cursor from slowly drifting on
most controllers. (Note: the Deadzone setting has no effect when
relative input is disabled)
This changes InputConfigDiag to disable the Dead Zone field in the IR
group when relative input is disabled.
This makes the GUI show the settings that are loaded when the config
gets reloaded, instead of showing potentially outdated settings that
are not applied.
This adds a recenter control binding which allows recentering the
cursor when relative input is enabled.

(EnableSettingControl is renamed to avoid confusions.)
@shuffle2 shuffle2 merged commit 2db2e88 into dolphin-emu:master Oct 3, 2016
@leoetlino leoetlino deleted the relative-input branch October 7, 2016 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants