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

[ritamerkl]

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.

[ritamerkl]

WIP - Adding tests - coming soon

[AlexTyrer]

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

[lyndon-unity]

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.

[ritamerkl]

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:

1. Input updates happening between frames on custom times or fixed update (checking against input update step) and
2. 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.

[lyndon-unity]

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.

[ritamerkl]

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]

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

[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?

[chris-massie]

The documentation on WasPerformedThisDynamicUpdate says that

If the update mode is set to InputSettings.UpdateMode.ProcessEventsInDynamicUpdate, this method will behave exactly like WasPerformedThisFrame.

however that is not the behavior I'm seeing when I set up a test example.

I created a MonoBehaviour that uses an early execution order that ensures it runs before any of my other scripts:

[DefaultExecutionOrder(-31001)]
public class ManualInputTicker : MonoBehaviour
{
    protected void Update()
    {
        if (InputSystem.settings.updateMode != InputSettings.UpdateMode.ProcessEventsManually)
            return;

        InputSystem.Update();
    }
}

In my setup, the Input Action is just a very basic "Button" Action Type action with a basic <XRController>{RightHand}/{GripButton} binding without any other modifications.

When Input System settings are set to Process Events In Dynamic Update, the different ways of checking input is a match. However, when I set it to Process Events Manually, I get an unexpected frame delay when checking WasPerformedThisFrame/WasPerformedThisDynamicUpdate. In the screenshots, when I pressed the grip button, the WasPerformedThisFrame goes true on frame 223 in my example, but WasPerformedThisDynamicUpdate only goes true on frame 224. When I released the grip button, same frame delay outcome with WasCompletedThisFrame going true on frame 372 but WasCompletedThisDynamicUpdate going true on frame 373.

Automatic Dynamic Update:

Manual Update:

(I can go over my test setup in a Zoom call if needed, but in these logs pF: means WasPerformedThisFrame/WasPerformedThisDynamicUpdate and pC: means WasCompletedThisFrame/WasCompletedThisDynamicUpdate)

I would expect that if my code manually calls InputSystem.Update during Update when the project settings are set to Manually Update, it should apply within that same frame to match the other methods. But maybe my assumption is incorrect and this is actually working as designed?

[ritamerkl]

I added a help box now which indicates that the Manual update mode is not recommended: (it only shows up with this drop down option)