Skip to content

FIX: The button is not clickable when using Windows touch simulation inputs (ISXB-483) #2012

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Oct 4, 2024
Merged

FIX: The button is not clickable when using Windows touch simulation inputs (ISXB-483) #2012

merged 12 commits into from
Oct 4, 2024

Conversation

jfreire-unity
Copy link
Collaborator

@jfreire-unity jfreire-unity commented Sep 24, 2024 •
edited
Loading

Description

Fixes ISXB-483

Simulated touches were not processed using Touch simulation (Input Debug > Options > Simulate Touch Input) with the PlayerInput component. This PR addresses that.

image

Changes made

Made sure the simulated touchscreen is paired with the PlayerInput users' devices.
TouchSimulation queues input events instead of changing state. This triggers device pairing and control scheme switching of PlayerInput.
And also, makes the events show up in Input Debugger.

Kapture.2024-09-26.at.15.52.38.mp4

Testing

Tested:

  • Toggling the Simulate Touch Input option in PlayMode
  • Removing the simulated touchscreen device.
  • Toggling the PlayerInput component in the project scene

Careful ⚠️ : For the specific project of the bug report, the actions that have touch bindings are not set to the Touch control scheme. So the simulated touchscreen/real touchscreen will not work out of the box. The InputActionAsset actions for touchscreen devices need to be changed to included "Touch" control scheme:
image

Risk

Minimal regression risk. I validated that queuing these simulated events still occurs in the same frame where they were generated from Mouse/Pen.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

ekcoh
ekcoh reviewed Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

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

Code changes look good, I think I need to check this again to figure out how this is intended to work. I think you need to run formatter on the code.

Packages/com.unity.inputsystem/InputSystem/Plugins/EnhancedTouch/TouchSimulation.cs Outdated
InputUser.onChange += PairSimulatedTouchscreenOnChange;
}
}
private void PairSimulatedTouchscreenOnChange(InputUser user, InputUserChange change, InputDevice device)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the reported issue also an issue when not using control schemes or does it only surface when the control schemes are present? Basically what would the intended behavior be in case a user was using a real mouse and a real touch screen at the same time? I would expect ping ponging effects on pointer. So maybe this is different since one want to avoid that and the current system do not support reasoning about sources of synthesised events...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is the reported issue also an issue when not using control schemes or does it only surface when the control schemes are present?

I didn't test this without control schemes, as they essentially control the PlayerInput use device pairing mechanism (which uses input events). And the problem is pairing mechanism was not taking into account the simulated touchscreen, because it doesn't generate input events.

Basically what would the intended behavior be in case a user was using a real mouse and a real touch screen at the same time? I would expect ping ponging effects on pointer

Yes that would be the behavior. But for this case, mouse (or pen) doesn't exist as its driving the simulated touchscreen. Also the ping pong only ocurrs if "Auto-switch" in PlayerInput is selected. Unless I'm misunderstanding something.

I'm confused with what you mean with this. Can you clarify?

So maybe this is different since one want to avoid that and the current system do not support reasoning about sources of synthesised events...

@ritamerkl ritamerkl removed their request for review September 24, 2024 15:34
@jfreire-unity jfreire-unity requested a review from ekcoh September 25, 2024 15:44
@jfreire-unity
Merge branch 'develop' into isxb-483-touchsimulation-playerinput-fix
3ed2fda
@jfreire-unity
Fix formatting
bbe2365
@jfreire-unity
Merge branch 'develop' into isxb-483-touchsimulation-playerinput-fix
531f05a
@jfreire-unity
Refactored initial fix to queue state events instead
4203ed1 
Based on review and code comments, this seems like a better solution that fixes the main problem and also better simulates a touchscreen when used together with PlayerInput.
Automatic pairing and control scheme occurs now since we're using input events a real device would also use.
ekcoh
ekcoh approved these changes Sep 27, 2024
View reviewed changes
Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

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

LGTM. Side-note: Its extremely odd that this primaryTouch is not part of the array, but that is unrelated to your PR

@Pauliusd01
Copy link
Collaborator

@jfreire-unity is this ready to look at? There's a lot of CI failures

@jfreire-unity
Copy link
Collaborator Author

@jfreire-unity is this ready to look at? There's a lot of CI failures

Not yet. I'll make sure to ping you when it's done. Dealing with CI failures first

@jfreire-unity
Change logic to be based on state events
3e5849a 
The previous logic was based on changing state and didn't apply if we queued events.

Also, this commit avoids processing the queued events for the simulated Touchscreen.
@jfreire-unity
Merge branch 'develop' into isxb-483-touchsimulation-playerinput-fix
b419900
@Pauliusd01
Copy link
Collaborator

Will check today

@jfreire-unity jfreire-unity force-pushed the isxb-483-touchsimulation-playerinput-fix branch from 93d31be to 42a43dd Compare October 1, 2024 12:03
Pauliusd01
Pauliusd01 requested changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

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

Updating status, had an issue with simulated multi touch events staying active. More details in DM

Pauliusd01
Pauliusd01 approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

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

LGTM, checked real and simulated touches on Windows and Android in a couple of touch samples as well as the user's project (after fixing their input actions) Did not test pen support though as I do not have the device for it

@jfreire-unity jfreire-unity merged commit bb685c5 into develop Oct 4, 2024
77 checks passed
@jfreire-unity jfreire-unity deleted the isxb-483-touchsimulation-playerinput-fix branch October 4, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ekcoh ekcoh ekcoh approved these changes

@Pauliusd01 Pauliusd01 Pauliusd01 approved these changes

@stefanunity stefanunity Awaiting requested review from stefanunity

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jfreire-unity @Pauliusd01 @ekcoh