Skip to content

Conversation

bmalrat
Copy link
Collaborator

@bmalrat bmalrat commented Oct 2, 2024

Description

When using an OnScreenControl with default Control scheme with a Move Action it will trigger a Cancel event on every frame since if will autoswitch back to pointer device when the user made an input and the OnScreenControl will switch back to the emulated one.

ISXB-656 The "Cancel" event is registered every frame when the Move action is used

Changes made

Added an instance counter to OnScreenControl to know if there is an active one.
Added a check in player autoswitch filter to prevent switching to pointer scheme when using an OnScreenControl
Changed OnScreenControl to automatically switch to a scheme that use the emulated device to prevent some remaining interaction when is was previously using a pointer.

Testing

added test and tested on customer project

Risk

I added the auto switch to target device in the OnScreenControl but I'm not sure if it's the right condition.
For now it's only on the first control if it's not part of the current player device.
But it may be better to just move aways from pointer devices in all case 🤔.
(or remove that part of the change)

Checklist

Before review:

  • [X ] 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".
  • [X ] Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • [X ] Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

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.

var devices = firstPlayer.devices;
bool deviceFound = false;
// skip is the device is already part of the current scheme
foreach (var device in devices)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm less confident with that part I choose to switch to target device in all case but maybe I just only try to move away from a pointer device. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I started to think the same, that maybe the design error here is, first of all auto-switch with on-screen controls, but also that on screen controls rely on pointer instead of touch. Of course pointer is convenient when working in the editor but generally it feels like touch should be the underlying input here... In editor its possible to simulate touch from mouse so problem can be solved anyway....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

without isolation it's hard to reliably filter touch event with ugui event.

@bmalrat bmalrat requested review from ekcoh and Pauliusd01 October 2, 2024 14:40
@Pauliusd01
Copy link
Collaborator

Will check today

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.

Only commenting for now since I need to let this sink in a bit. I wonder if there is another solution where OnScreen controls would consume touch but not pointer? I am not sure you considered this. In general, this is one more example of auto-switch causing problems.

Move(mouse.position, new Vector2(100f, 100f));
InputSystem.Update();

// The controlScheme shouldn't have changed
Copy link
Collaborator

Choose a reason for hiding this comment

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

I remain confused regarding why this would be expected... I guess the current problem is that OnScreenStick and OnScreenButton is using pointer instead of touch so this leads to ping pong in editor setting. Still given how things currently work don't we define auto switching as being triggered by input on related control schemes? If this scenario is about on screen sticks it feels like what would make sense would be for the on screen controls to turn of auto switching since it makes little sense to have onscreen controls and auto-switching combined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know it's a valid use case to keep auto-switching with onscreen control.
If we are afraid of side effect we could add an opt out parameter to allow autoswitch when there is an on screen control.

var devices = firstPlayer.devices;
bool deviceFound = false;
// skip is the device is already part of the current scheme
foreach (var device in devices)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I started to think the same, that maybe the design error here is, first of all auto-switch with on-screen controls, but also that on screen controls rely on pointer instead of touch. Of course pointer is convenient when working in the editor but generally it feels like touch should be the underlying input here... In editor its possible to simulate touch from mouse so problem can be solved anyway....

@bmalrat bmalrat force-pushed the isxb-656-fix-for-auto-schemes-switching-with-onscreen-components branch 3 times, most recently from b11acea to bd64ebd Compare October 31, 2024 18:36
@bmalrat bmalrat force-pushed the isxb-656-fix-for-auto-schemes-switching-with-onscreen-components branch from bd64ebd to f1ff06b Compare October 31, 2024 18:41
@bmalrat bmalrat force-pushed the isxb-656-fix-for-auto-schemes-switching-with-onscreen-components branch from f1ff06b to cc25550 Compare October 31, 2024 19:23
@bmalrat bmalrat requested a review from ekcoh October 31, 2024 19:25
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

@bmalrat bmalrat merged commit 08349cc into develop Nov 4, 2024
77 checks passed
@bmalrat bmalrat deleted the isxb-656-fix-for-auto-schemes-switching-with-onscreen-components branch November 4, 2024 22:35
smnwttbr pushed a commit that referenced this pull request Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants