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. 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.

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 WasPressed will return only true if the action was pressed this InputSystem update cycle (eg in fixed update). This was missing since the old approach was assuming a frame based input updating.

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.

[lyndon-unity]

Am wondering how big an impact this increase in size is for states.
(How many do we typically have active)

[ekcoh]

It would be linear to the number of actions if I am not mistaken?

[ritamerkl]

yes

[ekcoh]

I am a little confused of the intent here. wasXXXXThisFrame was intended to be evaluation whether an event occurrent at least once in the associated frame (which is a time window related to the driving update setting if I am not wrong on this). When the system was configured with Dynamic Update "ThisFrame" corresponds to a dynamic update frame. When set to e.g. FixedUpdate "ThisFrame" corresponds to a fixed update frame. Regardless of setting both frames coexist as overlapping time windows in parallel. Hence, wouldn't it make sense to attempt keeping frame comparison and tracking but in parallel for each time context? This would in turn require the same "tracking data" to coexist per update perspective but this cannot be avoided I am afraid if the requirement is to be able to evaluate multiple timelines. Based on such an approach there could e.g. be

public unsafe bool WasReleasedThisFrame(InputUpdateType updateType = InputUpdateType.UseCurrentSetting) and let it default to current configuration unless specified. If specified update type could be used to offset into array of parallel timeline information. This way API contract could remain intact, but one could be explicit, e.g. UI could always pass InputUpdateType.Dynamic?

With the proposed e.g. WasReleased() it's not clear to me what frame this maps to? The comment says Update cycle which would imply the frame given by setting right? Could you help me understand what time window this maps to?

[ritamerkl]

I am a little suprised that ThisFrame should map to FixedUpdate, FixedUpdate can happen multiple times between frames.

I will schedule a call to explain this in more detail.
But the basic idea maps down to the solution to the fact that InputSystem.Update() can be called to any time (every millisecond if desired) which increases the update step.

So there is the desire to check for input events on input update steps (which are not necesarilly related to dynamic update at all) and the desire to check against the current frame (eg for UI, maybe users need some events only once per frame).
The previous logic was solely relying on a frame based approach and would fail for FixedUpdate and any other manual input update setup.

[ritamerkl]

In short:
WasReleased()- input update step (eg in fixed update, fixed update & dynamic update or at any millisecond in the game)
WasReleasedThisFrame() - dynamic update cycle

[ekcoh]

Yes a frame is a time window, as you say it gets problematic if the consumer of the input do not get the answer in the time window (frame) of interest the consuming context is assuming. For UI update its rendering frames as you say. Let's discuss in the call. Also added an ASCII image comment to this PR to aid the discussion.

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

Why is this test update to step a frame?
Was it failing without this change?

[ritamerkl]

yes it was failing before, since this is relying on a frame based (dynamic update) comparison and InvokeUpdate() does not set the frameCount like in a real playmode (Update) scenario.

[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