FIX: Composite touchscreen controls not firing action after enabling (ISXB-98)#1540
FIX: Composite touchscreen controls not firing action after enabling (ISXB-98)#1540jamesmcgill merged 9 commits intodevelopfrom
Conversation
|
|
ekcoh
left a comment
ekcoh
left a comment
There was a problem hiding this comment.
@jamesmcgill Great description of the issue and fix in this PR and walking the reader through what was causing the problem. Impressed you were able to decipher this issue. Is there something regarding this behavior we should add or are missing in docs in your opinion?
|
Thanks for the review @ekcoh ! I found one place that the sorting was mentioned in the docs and I edited that part. But generally I think the sorting is an internal detail so I don't think we will need to add anything (especially as this PR just partially restores the previous behaviour rather than add something new). |
| // i.e. don't reverse the order of controls which have the same complexity | ||
| var firstComplexity = InputActionState.GetComplexityFromMonitorIndex(listeners[j - 1].monitorIndex); | ||
| var secondComplexity = InputActionState.GetComplexityFromMonitorIndex(listeners[j].monitorIndex); | ||
| if (firstComplexity >= secondComplexity) | ||
| break; |
There was a problem hiding this comment.
If we remove sorting by monitorIndex, which includes the binding index in the lowest bits, we'll end up processing bindings in potentially a different order than they are specified in the input asset. I can't decide if that's ok or not. Wasn't there some discussion recently in devs-input about a team using the explicit order of bindings to fall through to less specific bindings if more specific ones weren't available? Can this break that?
There was a problem hiding this comment.
The sorting was merged on the 24th March and it effectively was reversing the order they would have been in before. So bindings with the sorting enabled were now in descending order e.g. 4,3,2,1. This PR change would keep them in whatever order they were before 24th March (which I think would be ascending).
I'm not sure about the order in the asset or about the binding fall through discussion, but I'm thinking unless these things were post 24th March, then they might have also been broken by adding the sort, and this PR should probably help restore those too?
There was a problem hiding this comment.
XR team is using it for this use-case and might have a project to test with, contact @chris-massie may provide insight
5 participants
Description
After the first enabling of an action map that is bound to composite touchscreen controls, the action would not fire if the touchscreen was not in the default state at this point. Which would happen if the screen was ever touched prior to enabling the action. (This is because Touchscreens are never in the default state if they were ever used. E.g. the position never returns to default and is essentially always actuated forever).
The issue was due to sorting behaviour introduced in #1405
This sorting would reverse the order of the Touchscreen controls which just happened to work before because the Press Control was first in the list (after sorting everything was reversed; device order was Touch 4, Touch3, Touch 2 etc, and controls within are reversed, with Pressed now last).
This fragile behaviour of the Touchscreen controls always being actuated and the order mattering is not addressed in this PR. What this PR does, is restore the previous ordering but still doing the sorting. As sorting by composite complexity is required for the composite PR #1405 to work.
Changes made
Checklist
Before review:
Changed,Fixed,Addedsections.([case %number%](https://issuetracker.unity3d.com/issues/...)).Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.