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 "Enable Controller Input" Checkbox on TAS dialogs #7203

Merged
merged 1 commit into from Jul 8, 2018

Conversation

rukai
Copy link
Contributor

@rukai rukai commented Jul 2, 2018

wx used to display physical controller inputs in the TAS dialogs, this was cool but it broke TAS inputs when you had a controller plugged in that sent inputs in the background e.g. GC -> Wii U adapter or a regular controller with background input enabled.

This PR restores that functionality without background inputs causing problems by using a checkbox.
The same checkbox also allows the user to swap between input by TAS and input by physical controller.

Thanks to @spycrab for the initial attempt. You can close your existing PR.

use_tas_checkbox2

@JosJuice
Copy link
Member

JosJuice commented Jul 2, 2018

I think the wording "Use TAS" makes it too hard to understand what the option is supposed to be doing. "Use TAS Input" would be much better.

@rukai
Copy link
Contributor Author

rukai commented Jul 2, 2018

Fair enough. It was in an effort to conserve space on the GC TAS Dialog. Users could potentially have 4 of these TAS dialogs on screen at once.

If you want me to rename to "Use TAS Input", then I will move it to a separate line on the GC TAS dialog?

@JosJuice
Copy link
Member

JosJuice commented Jul 2, 2018

Having it on a separate line sounds fine. Alternatively, if you would prefer to have something as short as "Use TAS", I think "Enable" would work.

@JosJuice
Copy link
Member

JosJuice commented Jul 2, 2018

No, I mean "Enable", not "Enable TAS". The important thing is that it doesn't say "TAS" without "Input" afterwards.

@rukai
Copy link
Contributor Author

rukai commented Jul 2, 2018

Oh, my bad.
I need the "T" or at least some unique letter for the shortcut.
So I'll just stick it on a newline and call it "Enable TAS Input".

@rukai rukai changed the title Add "Use TAS" Checkbox on TAS dialogs Add "Enable TAS Input" Checkbox on TAS dialogs Jul 2, 2018
Copy link
Contributor

@spycrab spycrab left a comment

Choose a reason for hiding this comment

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

LGTM

@spycrab
Copy link
Contributor

spycrab commented Jul 3, 2018

Please take a look at this. Might need to be addressed before merging.

Edit: Some further thoughts

Maybe turning the checkbox into a combobox is feasible? So you have "Controller only", "TAS Input only", "Auto" (with Auto being the default)

@Techjar
Copy link
Contributor

Techjar commented Jul 3, 2018

To amend to what spycrab linked, I think the best solution to this would be to drop the checkbox idea, and instead do it based on previous controller state. So as long as no buttons or axes on the controller are touched, use the TAS window, otherwise override whatever value(s) changed on the controller.

That seems like the most intuitive solution to me, and solves the issue of axes always being overridden when using the Wii U adapter.

Edit: Referring to spycrab's edit, this behavior would be "auto".

@rukai
Copy link
Contributor Author

rukai commented Jul 4, 2018

I'm looking into making it a QComboBox with an auto option.

@rukai
Copy link
Contributor Author

rukai commented Jul 4, 2018

I added a QComboBox with 3 options, but then I realized that the Controller Input only option was pointless as the controller will always overwrite the TAS input in the auto option. So we can just use the auto case as the Controller Input case

So I went back to a QCheckBox but named it "Enable Controller Input".

  • When enabled: input is from TAS but this is overwritten when presses/movement is detected from controller input (analog values will always be overridden for controllers that give background input)
  • When disabled: input is from TAS

@Techjar
Copy link
Contributor

Techjar commented Jul 4, 2018

The issue with background input is why I suggested having the TAS input store the last controller state, and then have the TAS input override the controller values as long as the state of the real controller doesn't change.

@rukai
Copy link
Contributor Author

rukai commented Jul 4, 2018

Sorry, I misread your comment.
My intuition is that I cant rely on controllers to maintain the same analog value.
However now that I think about it that's probably wrong, so I'll investigate, tonight/tomorrow.

@Techjar
Copy link
Contributor

Techjar commented Jul 4, 2018

I did some quick testing already and, at least with my GC controllers, the axes will stay at one value as long as I don't touch it. The only problem is those values aren't zero, which is what my suggestion is a solution for.

Small amounts of wiggle shouldn't be a problem for anything but the Wii U adapter mode, as all other types have a configurable deadzone.

@rukai
Copy link
Contributor Author

rukai commented Jul 5, 2018

My gamecube controllers with GC -> Wii U adapter sometimes jitter between 2 values when left idle.

@Techjar
Copy link
Contributor

Techjar commented Jul 5, 2018

Well, it might not work on everyone's hardware, but it will on more than it does now (which is none).

@rukai rukai force-pushed the use_tas_checkbox branch 2 times, most recently from a2c172a to 2a1ee53 Compare July 7, 2018 12:24
@rukai rukai changed the title Add "Enable TAS Input" Checkbox on TAS dialogs Add "Enable Controller Input" Checkbox on TAS dialogs Jul 7, 2018
@rukai
Copy link
Contributor Author

rukai commented Jul 7, 2018

qt_tas_use_thingy_finished

All feedback should now be addressed.
The code should be re-reviewed as there has been significant changes since the last review.

This PR now adds an 'Enable Controller Input' checkbox to TAS dialogs.

  • When disabled: only inputs from TAS dialog are used.
  • When enabled: inputs from TAS dialog are used, except when a change in input is detected from a real controller, in this case the TAS value is replaced with the real controller value.

When the checkbox is ticked analog values may reset to controller values at random, due to jitter on control sticks/triggers.
To avoid users getting confused by this behavior:

  • The checkbox tooltip says: "Warning: Analog inputs may reset to controller values at random. In some cases this can be fixed by adding a deadzone."
  • The checkbox is disabled by default.

When disabled only inputs from TAS dialog are used.
When enabled inputs from TAS dialog are used, except when a change in
input is detected from a real controller, in this case the TAS value is
replaced with the real controller value.
@spycrab spycrab merged commit 8c97fb7 into dolphin-emu:master Jul 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants