FIX: Changed to frame based comparison for tracking UI click/navigation events #2112

ritamerkl · 2025-01-22T14:23:02Z

Description

During investigating ISXB-1313 it got clear that catching navigation and click events does not work properly once InputSystem is set to a custom / Fixed Update mode.

Analysis:
The main reason for this is that the InputSystemUIInputModule (Process -> ProcessNavigation -> WasPerformedThisFrame) is automatically called in dynamic Update, this calls in

WasPressedThisFrame()
WasReleasedThisFrame()
WasCompletedThisFrame()
WasPerformedThisFrame()

to check weather UI actions were triggered. Internally those methods use

pressedInUpdate
releasedInUpdate
lastPerformedInUpdate
lastCompletedInUpdate

which are properties to track at which update cycle of the InputSystem the events triggered. All above are just used for this checks.

Problem:
The update step is not comparable at the time of the dynamic Update since it is not clear how many times InputSystem.Update() (and with that the current update step was incremented by 1) was called in between two dynamic Updates. It can be incremented by 1 customly every millisecond or switched to be called in FixedUpdate.

Solution:
The solution is to have two different methods for the (WasPressedThisFrame/WasPerformedThisFrame ...)checks, where one checks for the current frame only and the other for the input update cycle.
They will be equal if the update mode is set to dynamic Update, if set to FixedUpdate or Manual update eg WasPressedThisRenderingFrame will return only true if the action was pressed this dynamic update cycle (no matter in which cycle the InputSystem is updated). This was missing since the old approach was assuming a frame based input updating and would not return valid values if updating the InputSystem would happen in any other time cycle.
InputSystemUIInputModule calls in the WasPerformedThisRenderingFrame() now, which returns valid results independent of the InputSetting during the dynamic Update.
Decoupling the rendering frame count checking from the input update count checking is one essential part of this PR. Those two values are only related if the input update mode is set to dynamic Update (in fact they are the same in this case). In any other case they don't have anything in common. Checking when an action state was changed can happen in the same time cycle like updating the InputSystem (which can now be decoupled from the dynamic Update).

See more details in this technical discussion document.

Testing status & QA

testing in InputSystem repo, bug project (attached to ISXB-1313) and against https://jira.unity3d.com/browse/ISXB-1160
https://jira.unity3d.com/browse/ISXB-1141.

Overall Product Risks

Complexity: Medium
Halo Effect: High

Comments to reviewers

For further explanation also check the comments in the ticket.

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".
Tests added/changed, if applicable.
- Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
- Performance tests.
- Integration tests.
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.

…d approach - track frame of interest in action

… checking

ritamerkl · 2025-01-24T14:12:22Z

WIP - Adding tests - coming soon

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

AlexTyrer

Changes look like a good approach to fixing the problems with dynamic/fixed and manual update.

lyndon-unity · 2025-01-24T17:20:40Z

I'm a little concerned about having two functions to check the state.
I'm also unsure if the naming is clear enough as WasPressed still checks part of the input update step.
actionStatePtr->pressedInUpdate == currentUpdateStep && currentUpdateStep != default

In summary as a user I find the naming confusing and hard to tell if I when to use WasPressed vs WasPressedThisFrame

Adding a another reviewer for input.

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

Packages/com.unity.inputsystem/InputSystem/Actions/InputAction.cs

ritamerkl · 2025-01-27T10:30:16Z

I'm a little concerned about having two functions to check the state. I'm also unsure if the naming is clear enough as WasPressed still checks part of the input update step. actionStatePtr->pressedInUpdate == currentUpdateStep && currentUpdateStep != default
I think separating those two is necessary because we have two cases:

Input updates happening between frames on custom times or fixed update (checking against input update step) and
Input updates happening between frames / custom times / fixed update and the need of knowing at the time of dynamic update (checking against frame)
The need to have the 2. option sets the requirement for the second API piece, this is made by two conditions: the desire to check frame based by components like UI (can also be handy for users who still want to check input in dynamic update despite updating input in another time cycle) and the fact that the input system can be updated manually.
The API surface could be reduced to only allow WasPerformed / WasPerformedThisFrame in the two different dynamic/input update modes. Since it is the only one now used internally to identify input events in dynamic updates. The reason that I applied it for all 4 API pieces is consistency at one hand and to allow users with custom input update cycles accessing input events between frames on the other hand (which would require another workaround otherwise).

Another option would be to just at a boolean parameter to each API piece wheather to check the dynamic Update cycle (which could default to true).

In summary as a user I find the naming confusing and hard to tell if I when to use WasPressed vs WasPressedThisFrame

I understand the concerns, I couldn't make up a better name for it since WasPressedThisUpdate() would be misleading since Update could be wrongly interpreted as dynamic Update again.

Assets/Tests/Samples/RebindingUITests.cs

lyndon-unity · 2025-01-27T11:34:43Z

Assets/Tests/InputSystem/CoreTests_Actions.cs

@@ -87,9 +87,9 @@ public void Actions_WhenShortcutsDisabled_AllConflictingActionsTrigger()
        Press(keyboard.wKey);

        // All Actions were triggered
-        Assert.That(action1.WasPerformedThisFrame());


There are a lot of tests update to make this pass?
Were all these failing without the changes?

Make me worry about how much customer code would be impacted by the change.

The test run without mimiking the dynamic update, which make this call wrong in it self (ThisFrame). Either they need to use Coroutines (see comment above) or rely on InputSystem.Update manual updates like here. In real settings (non test environments) there are no such requirements, so I don't think the users will be impacted. Especially because most users rely on update, which will work just like before.

ekcoh · 2025-01-27T14:39:27Z

What I meant with previous question/comment is that, considering e.g. this sequence, the results vary depending on what frame corresponds to right? Mainly adding this comment to support discussion.

Expected wasPressedThisFrame for different kinds of frames (Difference time perspectives)
--------------------------------------------------> time
0   0   1   1   1   0   1   0   1   0   0   0       Button State
|  Yes     |  No       | Yes      |  No       |     Dynamic Update
|  Yes         |  Yes         |  Yes         |      Fixed Update
|  No  | Yes   | No   | Yes      | No    | No   |   Manual Update

…r-update-non-dynamic # Conflicts: # Packages/com.unity.inputsystem/CHANGELOG.md

Pauliusd01

LGTM. Things checked (playmode, player, mac/win, 21.3; 22.3; 6.0; 6.2):

Changing Input Settings update modes
Different FPS settings (1-120)
Mouse clicks, keyboard clicks, gamepad clicks for the UI. Checked that both events shows up
Similar, older bug fixes are still fixed (ISXB-1141)
General input samples, main test project

lyndon-unity

With QA sign off on the changes I'm happy to approve. Thank you.

ekcoh

Changes generally makes sense, would have been great to see some test coverage of the FixedUpdate and ManualUpdate modes. Additionally I wonder whether the corresponding fixes for wasPressedThisFrame/wasReleasedThisFrame are missing (to provide consistency) for:

https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs#L278-L298

https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs#L323-L343

https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Plugins/UI/PointerModel.cs#L245-L248

Or would this make them behave differently compared to this if evaluated in tandem with each other? Which would be a great test to have for the different modes.

ekcoh · 2025-02-11T12:40:02Z

Assets/Tests/Samples/RebindingUITests.cs

    [Category("Samples")]
-    public void Samples_RebindingUI_SuppressingEventsDoesNotInterfereWithUIInput()
+    public IEnumerator Samples_RebindingUI_SuppressingEventsDoesNotInterfereWithUIInput()


@ritamerkl Do we have a test that verifies behavior when configured to run input processing on fixed update?

I will add tests now

added 8332195

especially tested the update and frame based comparing in co-dependency to another

Packages/com.unity.inputsystem/CHANGELOG.md

ekcoh · 2025-02-11T13:03:46Z

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs

@@ -3796,10 +3816,25 @@ public uint lastPerformedInUpdate
                set => m_LastPerformedInUpdate = value;
            }

-            internal int frame
+            internal int framePerformed


(Minor) If these are anyway internal and just plain getters/setters, why not skip the properties and access the fields directly here? If keeping it like this potentially mark with aggressive inlining hint

You are right, they were kinda decorational only - fixed here 7114e6f

ekcoh · 2025-02-11T13:09:35Z

Seems to be a conflict in InputSystemUIInputModule that needs to be resolved.

ritamerkl · 2025-02-12T11:09:28Z

Changes generally makes sense, would have been great to see some test coverage of the FixedUpdate and ManualUpdate modes. Additionally I wonder whether the corresponding fixes for wasPressedThisFrame/wasReleasedThisFrame are missing (to provide consistency) for:

https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs#L278-L298

https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs#L323-L343

Those two actually just check the Update step, like desired.

https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Plugins/UI/PointerModel.cs#L245-L248

This checks on the rendering frame, and is exclusively changed by the InputSystemUIInputModule, which looks right then, since this operates in the dynamic update cycle.

ekcoh · 2025-02-12T13:41:17Z

Changes generally makes sense, would have been great to see some test coverage of the FixedUpdate and ManualUpdate modes. Additionally I wonder whether the corresponding fixes for wasPressedThisFrame/wasReleasedThisFrame are missing (to provide consistency) for:
https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs#L278-L298
https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Controls/ButtonControl.cs#L323-L343

Those two actually just check the Update step, like desired.

https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Plugins/UI/PointerModel.cs#L245-L248

This checks on the rendering frame, and is exclusively changed by the InputSystemUIInputModule, which looks right then, since this operates in the dynamic update cycle.

Sounds good! Thanks for checking in detail

ekcoh · 2025-02-12T13:42:56Z

Seems the remaining CI issues you are facing Rita is due to this PR having version 1.13.1 when it needs to be 1.14.0 to not violate package verification due to 4 added APIs

ekcoh · 2025-02-12T13:43:51Z

Seems the remaining CI issues you are facing Rita is due to this PR having version 1.13.1 when it needs to be 1.14.0 to not violate package verification due to 4 added APIs

Will approve when this have been fixed

o/

…ub.com/Unity-Technologies/InputSystem into fix-ui-behavior-for-update-non-dynamic

ekcoh

Seems version is correct now given API additions which made CI green, great.
The only two remaining things I can spot or think of now is:

Why isn't this called out also under #Added in changelog since the PR is adding functionality not previously available?
This risk on this PR is quite high so curious whether UI or XR QA or devs have been involved at any point since it may likely also have halo effects into e.g. XRI and UI related solutions?

Will approve now but consider extending CHANGELOG and/or reach out to UI and/or XR maybe before landing it?

Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs

…r-update-non-dynamic # Conflicts: # Assets/Samples/InGameHints/InGameHintsActions.cs # Assets/Samples/SimpleDemo/SimpleControls.cs # Assets/Tests/InputSystem/InputActionCodeGeneratorActions.cs # Packages/com.unity.inputsystem/InputSystem/AssemblyInfo.cs # Packages/com.unity.inputsystem/InputSystem/Devices/Precompiled/FastKeyboard.cs # Packages/com.unity.inputsystem/InputSystem/Devices/Precompiled/FastMouse.cs # Packages/com.unity.inputsystem/InputSystem/Devices/Precompiled/FastTouchscreen.cs # Packages/com.unity.inputsystem/Tests/TestFixture/AssemblyInfo.cs # Packages/com.unity.inputsystem/package.json

reduce complexity for checking UI interactions in update - frame base…

063ddb9

…d approach - track frame of interest in action

ritamerkl requested a review from AlexTyrer January 22, 2025 14:23

ritamerkl added 9 commits January 22, 2025 15:29

added changelog

5067af8

changelog formatting

29c0eda

switch to additional InFrame and InputSystem update logic

7beeb2b

fixed wrong reference to update instead of frame

b6e48dc

temporary workaround for rebinding test

ea856c3

save frame count when status changed

a814fa2

fixed frame state saving

e581aad

switched to coroutine with rebinding test to fix UI submit framebased…

94b59a1

… checking

removed redundant frame from InputActionState and InputAction

65b20c9

ritamerkl requested review from Pauliusd01 and lyndon-unity January 24, 2025 14:08

AlexTyrer reviewed Jan 24, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs Outdated Show resolved Hide resolved

Removed m_Frame

18dedda

AlexTyrer approved these changes Jan 24, 2025

View reviewed changes

lyndon-unity requested review from ekcoh and removed request for lyndon-unity January 24, 2025 17:12

lyndon-unity reviewed Jan 24, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs Outdated Show resolved Hide resolved

ekcoh reviewed Jan 27, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Actions/InputAction.cs Outdated Show resolved Hide resolved

lyndon-unity reviewed Jan 27, 2025

View reviewed changes

ritamerkl added 4 commits February 3, 2025 15:49

Merge remote-tracking branch 'origin/develop' into fix-ui-behavior-fo…

6e01bb1

…r-update-non-dynamic # Conflicts: # Packages/com.unity.inputsystem/CHANGELOG.md

changed to proposal 2

b486ebd

fix comments in InputAction tests

f49ccf0

Fixed spelling

39e057c

ritamerkl requested a review from ekcoh February 4, 2025 14:21

Merge branch 'develop' into fix-ui-behavior-for-update-non-dynamic

e9367a4

This comment was marked as resolved.

Sign in to view

Pauliusd01 self-requested a review February 10, 2025 11:21

Pauliusd01 approved these changes Feb 10, 2025

View reviewed changes

lyndon-unity approved these changes Feb 10, 2025

View reviewed changes

ekcoh reviewed Feb 11, 2025

View reviewed changes

ritamerkl added 2 commits February 12, 2025 12:13

Merge branch 'develop' into fix-ui-behavior-for-update-non-dynamic

adeddc9

Update CHANGELOG.md

e3b1a38

ritamerkl and others added 3 commits February 12, 2025 14:58

skip properties

7114e6f

added tests

8332195

update version to 1.14.0

95e4bfc

o/

ritamerkl requested a review from ekcoh February 13, 2025 16:17

ritamerkl and others added 3 commits February 14, 2025 10:16

fixed doc

2b4d758

Merge branch 'fix-ui-behavior-for-update-non-dynamic' of https://gith…

b826dc2

…ub.com/Unity-Technologies/InputSystem into fix-ui-behavior-for-update-non-dynamic

Merge branch 'develop' into fix-ui-behavior-for-update-non-dynamic

49fe0dc

ekcoh approved these changes Feb 14, 2025

View reviewed changes

ritamerkl and others added 4 commits February 14, 2025 14:12

Update CHANGELOG.md

998fcda

fix formatting for changelog

8eee052

fix formatting

e6cc47a

Merge branch 'develop' into fix-ui-behavior-for-update-non-dynamic

c37b912

chris-massie reviewed Feb 18, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Plugins/UI/InputSystemUIInputModule.cs Outdated Show resolved Hide resolved

ritamerkl and others added 2 commits February 19, 2025 10:16

update comment

51b9643

Changed naming from RenderingFrame to DynamicUpdate

676ad0f

ritamerkl requested a review from chris-massie February 21, 2025 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Changed to frame based comparison for tracking UI click/navigation events #2112

FIX: Changed to frame based comparison for tracking UI click/navigation events #2112

ritamerkl commented Jan 22, 2025 •

edited

Loading

ritamerkl commented Jan 24, 2025

AlexTyrer left a comment

lyndon-unity commented Jan 24, 2025

ritamerkl commented Jan 27, 2025 •

edited

Loading

lyndon-unity Jan 27, 2025

ritamerkl Jan 27, 2025

ekcoh commented Jan 27, 2025

This comment was marked as resolved.

Pauliusd01 left a comment

lyndon-unity left a comment

ekcoh left a comment

ekcoh Feb 11, 2025

ritamerkl Feb 12, 2025

ritamerkl Feb 12, 2025

ritamerkl Feb 12, 2025

ekcoh Feb 11, 2025 •

edited

Loading

ritamerkl Feb 12, 2025

ekcoh commented Feb 11, 2025

ritamerkl commented Feb 12, 2025

ekcoh commented Feb 12, 2025

ekcoh commented Feb 12, 2025

ekcoh commented Feb 12, 2025

ekcoh left a comment

FIX: Changed to frame based comparison for tracking UI click/navigation events #2112

Are you sure you want to change the base?

FIX: Changed to frame based comparison for tracking UI click/navigation events #2112

Conversation

ritamerkl commented Jan 22, 2025 • edited Loading

Description

Testing status & QA

Overall Product Risks

Comments to reviewers

Checklist

ritamerkl commented Jan 24, 2025

AlexTyrer left a comment

Choose a reason for hiding this comment

lyndon-unity commented Jan 24, 2025

ritamerkl commented Jan 27, 2025 • edited Loading

lyndon-unity Jan 27, 2025

Choose a reason for hiding this comment

ritamerkl Jan 27, 2025

Choose a reason for hiding this comment

ekcoh commented Jan 27, 2025

This comment was marked as resolved.

Pauliusd01 left a comment

Choose a reason for hiding this comment

lyndon-unity left a comment

Choose a reason for hiding this comment

ekcoh left a comment

Choose a reason for hiding this comment

ekcoh Feb 11, 2025

Choose a reason for hiding this comment

ritamerkl Feb 12, 2025

Choose a reason for hiding this comment

ritamerkl Feb 12, 2025

Choose a reason for hiding this comment

ritamerkl Feb 12, 2025

Choose a reason for hiding this comment

ekcoh Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

ritamerkl Feb 12, 2025

Choose a reason for hiding this comment

ekcoh commented Feb 11, 2025

ritamerkl commented Feb 12, 2025

ekcoh commented Feb 12, 2025

ekcoh commented Feb 12, 2025

ekcoh commented Feb 12, 2025

ekcoh left a comment

Choose a reason for hiding this comment

ritamerkl commented Jan 22, 2025 •

edited

Loading

ritamerkl commented Jan 27, 2025 •

edited

Loading

ekcoh Feb 11, 2025 •

edited

Loading