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: allow mapping buttons to touchscreen #5163

Merged
merged 4 commits into from May 31, 2020

Conversation

z87
Copy link
Contributor

@z87 z87 commented Apr 1, 2020

This change creates an additional TouchDevice, TouchFromButtonDevice, which uses a list of mappings between ButtonDevices and screen coordinates. If enabled, this device is polled in the HID module along with the regular one to allow both free and preset inputs.

Addresses #3494, #4261.

Mapping lists are maintained separately from input profiles to allow per-game setup. Device is configured by assigning one of the mappings to the input profile. Relevant qt-config.ini entries:

profiles\1\use_touch_from_button=true
profiles\1\touch_from_button_map=0
touch_from_button_maps\1\name=default
touch_from_button_maps\1\entries\1\bind="(ButtonDevice params),x:(x),y:(y)"

GUI screenshots:

ui1

ui2-win-light

ui2-lxqt-dark


This change is Reviewable

Copy link
Member

@zhaowenlan1779 zhaowenlan1779 left a comment

Hello and thanks for the contribution!

Regarding the UI, I would suggest something like...

Profile: [My mapping           v]    [New] [Delete] [Rename]
------------------    horizontal rule  ----------------------
Double click to change a field.               [Add] [Delete]

And you can just get rid of the [Edit] button as it is pretty much redundant.

I think this will work better as the Add button is closer to the table, but I'm not exactly good at UI designing either...

src/citra_qt/configuration/config.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/config.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_motion_touch.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_motion_touch.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_motion_touch.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_from_button.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_from_button.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_from_button.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_from_button.ui Outdated Show resolved Hide resolved
@z87
Copy link
Contributor Author

z87 commented Apr 2, 2020

Thanks for the review and suggestions! Fixed most of the issues.

@z87 z87 requested a review from zhaowenlan1779 Apr 2, 2020
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 left a comment

Generally looks good!

@zhaowenlan1779
Copy link
Member

zhaowenlan1779 commented Apr 3, 2020

I played around with it a bit and found some UI bugs:

  1. If you add a binding, but do not set its X and/or Y values, after you click on OK and click Configure again it crashes/freezes
  2. If there wasn't a mapping and you clicked on Configure and created one, the checkbox will become enabled. If you check the checkbox, but leave the combobox empty, once you start a game it will crash.
  3. The UI isn't very friendly to newcomers - the checkbox is disabled and there's no indication that the user should click on Configure to create a mapping. IMO, we should have a default mapping with no bindings by default so that the UI looks less confusing. (And I think we should make the last mapping undeleteable as it doesn't really make much sense and causes problems with the UI)
  4. Not too big a problem, but I think the Delete (binding) button should only be enabled when a binding is selected

@z87
Copy link
Contributor Author

z87 commented Apr 4, 2020

Implemented the suggestions and made ok/cancel work predictably through both dialogs by keeping current state in the motion/touch dialog and passing it to the mapping dialog.

@B3n30
Copy link
Contributor

B3n30 commented Apr 27, 2020

@zhaowenlan1779 Can you have a second look, and eventually approve this PR, please

B3n30
B3n30 approved these changes May 1, 2020
@z87 z87 force-pushed the input-touch-mapping branch from 44426bd to 0993b91 Compare May 5, 2020
z87 added 2 commits May 8, 2020
Property "overflow" isn't documented, and it makes Qt complain about an
unknown property in the terminal.
@z87 z87 force-pushed the input-touch-mapping branch from 2ef4d16 to e792c3d Compare May 8, 2020
@z87 z87 requested a review from zhaowenlan1779 May 8, 2020
@B3n30
Copy link
Contributor

B3n30 commented May 9, 2020

@zhaowenlan1779 can you give this another review, so we can merge this soon?

Copy link
Member

@zhaowenlan1779 zhaowenlan1779 left a comment

Thanks a lot for your hard work! Comments are mostly just nitpicking. UI looks really great, I'll try this out tomorrow!

src/citra_qt/configuration/config.cpp Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_from_button.h Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_from_button.h Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_from_button.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_from_button.cpp Outdated Show resolved Hide resolved
}
}

void TouchScreenPreview::HighlightDot(const int id, const bool active) const {
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 May 9, 2020

Choose a reason for hiding this comment

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

Would it make more sense to simplify this to HighlightDot(id) which highlights that dot and de-highlight other dots?

Copy link
Contributor Author

@z87 z87 May 11, 2020

Choose a reason for hiding this comment

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

It would require either additionally keeping selection state or unnecessarily applying stylesheets, which could be slow according to some reports. I think the current form fits better: since the model keeps track of selection, this function is compatible with the model's behavior of adding to and removing from selection independently.

src/citra_qt/configuration/configure_touch_from_button.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_from_button.cpp Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_widget.h Outdated Show resolved Hide resolved
src/citra_qt/configuration/configure_touch_from_button.ui Outdated Show resolved Hide resolved
@RianCaio
Copy link

RianCaio commented May 10, 2020

There will be a option to add mutiple buttons to do one action

@z87
Copy link
Contributor Author

z87 commented May 11, 2020

@zhaowenlan1779 Thanks! Fixed most of the issues.

@RianCaio Yes, the configuration doesn't prevent several entries with equal coordinates. You can simply drag points on top of each other.

@RianCaio
Copy link

RianCaio commented May 11, 2020

No I a saying Two button to one action example ZR+Y To map button 2 in oot3d

@z87
Copy link
Contributor Author

z87 commented May 12, 2020

Good idea, but I think it's better to leave that for another PR.

@z87 z87 requested a review from zhaowenlan1779 May 12, 2020
@RianCaio
Copy link

RianCaio commented May 12, 2020

i dont think it would be a dificult change

@B3n30
Copy link
Contributor

B3n30 commented May 12, 2020

i dont think it would be a dificult change

I still think doing one feature at a time and trying to keep PRs smaller is better. So this could be a nice follow up PR

@RianCaio
Copy link

RianCaio commented May 12, 2020

yeah seeing that way make prs smaler and easy to see if any error happens

@zhaowenlan1779 zhaowenlan1779 merged commit 81a1e56 into citra-emu:master May 31, 2020
2 of 3 checks passed
@RianCaio
Copy link

RianCaio commented Jun 25, 2020

Please Add Multiple Buttons at the same mapping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants