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

TAS dialog: Toggle analog inputs between TAS and regular input #3609

Closed
wants to merge 1 commit into from

Conversation

rukai
Copy link
Contributor

@rukai rukai commented Feb 8, 2016

Solves https://bugs.dolphin-emu.org/issues/9325

As suggested by @Sonicadvance1 I have added a toggle button for each analog input allowing the user to have fine grained control over whether each analog input should be taken from the TAS input or its regular input. Digital inputs remain as they were, deciding which input to take automatically
input = TASInput && regularInput

Previously the code assumed a keyboard input which is not compatible with analog inputs.

@MaJoRoesch Can you please make a proper icon for this?

Review on Reviewable

tasanaloglockprdemo

@BhaaLseN
Copy link
Member

BhaaLseN commented Feb 8, 2016

Got a screenshot?


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Source/Core/DolphinWX/TASInputDlg.h, line 147 [r1] (raw file):
Alignment looks off here; Tabs for indent., then spaces for alignment if necessary - this line seems to be all spaces.


Comments from the review on Reviewable.io

@rukai rukai changed the title TAS dialog: analog inputs have buttons that toggle between tas control and regular input TAS dialog: Toggle analog inputs between TAS and regular input Feb 8, 2016
@BhaaLseN
Copy link
Member

BhaaLseN commented Feb 8, 2016

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@rukai
Copy link
Contributor Author

rukai commented Feb 8, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


Source/Core/DolphinWX/TASInputDlg.h, line 147 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@JosJuice
Copy link
Member

JosJuice commented Feb 8, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Source/Core/DolphinWX/TASInputDlg.cpp, line 44 [r2] (raw file):
This is wrong. The original alignment is the correct one.


Source/Core/DolphinWX/TASInputDlg.h, line 30 [r2] (raw file):
Same here.


Comments from the review on Reviewable.io

@lioncash
Copy link
Member

lioncash commented Feb 8, 2016

Review status: all files reviewed at latest revision, 8 unresolved discussions.


a discussion (no related file):
There should be a tooltip set for the lock buttons that explain what they do. Also the lock buttons should be considerably smaller to fit the UI. It's almost overlapping the depression that indicates the shoulder buttons. It would likely be a better idea to place the button under the slider value indicators for them as well.


Source/Core/DolphinWX/TASInputDlg.cpp, line 1137 [r2] (raw file):
This event table can be replaced with a Bind call in the constructor .

Bind(wxEVT_TOGGLEBUTTON, &TASInputDlg::OnLockToggle, this);

Source/Core/DolphinWX/TASInputDlg.h, line 11 [r2] (raw file):
this can be replaced with a forward declaration. The include can then be moved to the cpp file to avoid cluttering anything that includes the TAS header.


Source/Core/DolphinWX/TASInputDlg.h, line 66 [r2] (raw file):
lowercase tas


Source/Core/DolphinWX/TASInputDlg.h, line 148 [r2] (raw file):
m_locked_graphic
m_unlocked_graphic


Source/Core/DolphinWX/TASInputDlg.h, line 149 [r2] (raw file):
This should be placed with all the other functions.


Comments from the review on Reviewable.io

@RisingFog
Copy link
Member

@rukai anything left with this PR?

@rukai
Copy link
Contributor Author

rukai commented Jul 4, 2016

@RisingFog Yeah, still need a proper icon for it. @MaJoRoesch seems really busy these days, is there anyone else who could make/find one? Or alternatively is there a more suitable UI element we could use here instead of a lock icon?

@ghost
Copy link

ghost commented Jul 4, 2016

Not sure if this is really related to the PR but is there a reason why the circle is being rendered (1, 1) off-centre or is that intended? I know that happens in master too.

@ghost
Copy link

ghost commented Jul 24, 2016

The issue I noted in my previous post turned out to be a wxWidgets problem and is fixed now as of 5.0-211.

@RisingFog
Copy link
Member

This PR will need to be rebased.

If I don't see any response within two weeks, I'll rebase and re-submit a PR for this.

@shuffle2
Copy link
Contributor

shuffle2 commented Oct 3, 2016

Moving to #4265

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants