FIX: ISX-1954 UI action verification and null-reference access.

[ekcoh]

Description

This PR attempts to provide a fix for the following reported issues:

Steps to repro:

1. Make sure you have a Project-wide Input Actions asset created.
2. Create a new UIDocument and go to the UIBuilder to add any UI object to it.
3. Assign it to the UIDocument and go to the Project Settings > Input System Package tab and delete the UI map.
4. Enter play mode.

Actual behavior: Exception is being thrown (on every frame): NullReferenceException: Object reference not set to an instance of an object UnityEngine.InputSystem.Plugins.InputForUI.InputSystemProvider.ReadCurrentNavigationMoveVector () (at ./Library/PackageCache/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs:213) UnityEngine.InputSystem.Plugins.InputForUI.InputSystemProvider.DirectionNavigation (Unity.IntegerTime.DiscreteTime currentTime) (at ./Library/PackageCache/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs:162) UnityEngine.InputSystem.Plugins.InputForUI.InputSystemProvider.Update () (at ./Library/PackageCache/com.unity.inputsystem/InputSystem/Plugins/InputForUI/InputSystemProvider.cs:125) UnityEngine.InputForUI.EventProvider.NotifyUpdate () (at D:/Gitrepo/unity/Modules/InputForUI/Provider/EventProvider.cs:164)

Expected behavior: Warning mentions that the UI Map is necessary for the UITK UI functionality OR it automatically defaults to a default working copy.

The reason for the reported problem is that InputSystemProvider (Input.ForUI integration) utilise actions but there is no guarantee the Project-wide Input Action Asset guarantee that the UI map exists, that the actions within it map to implicit integration requirements nor that they have the expected properties. The exception is thrown when one or multiple actions subject for (unchecked) polling behaviour in the implementation while an action is missing.

Changes made

This PR introduces the following changes:

• Previous verification/checks of UI compliance in ProjectWideActionsAsset.cs have been refactored to allow for use from both editor and from Input.ForUI plugin. Additionally, verification code has been moved to the plugin (since the core wouldn't now about potential existing integrations) and additionally registers a verifier at static initialization time to later be evaluated when requested.
• Extended capabilities of verification code to not only check for map and action existence by name, but also check action type and associated expected control type. Any divergence from requirements would be reported.
• Extended warnings to be generated both when saving in editor and now also when entering play mode (on configuration being instantiated by integration code).
• Added test cases for all possible combinations of failure cases for verification to InputForUITests.
• Added test cases for generic verifier functionality to ProjectWideInputActionsEditorTests.
• Removed previous verification tests from ProjectWideInputActionsEditorTests since obsolete.

Open issues:

• Will enter-play mode warnings be annoying to users? Should we add a setting to disable them via the Importer inspector and only show the setting for PWA? E.g. a checkbox in the asset inspector. I struggle to see another good fit for such a setting without creating any asset.
• Clarity on actual UI Toolkit requirements on integration actions. Are the current 'Action Types', 'expected control type' and 'initial state check' configuration and coverage in verifier and tests mapping to actual assumptions? @benoit-alain? Are there any degrees of freedom or what could cause issues on consumer side?

Notes

N/A

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.
  • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
  • FogBugz is marked as "Resolved" with next release version correctly set.
• [x] Tests added/changed, if applicable.
  • Functional tests: 34 new functional tests added to InputForUITests. Additional tests added for generic verification of verifier code in ProjectWideActionsEditorTests.
  • 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.

[ekcoh]

Found that there are some issues remaining on this PR so temporarily putting "work in progress" label on it until resolved.

[Pauliusd01]

Is the asset path supposed to be clickable and point to that asset? As it does not do anything right now, I suggest it either should not be highlighted or should correctly select that asset (but the second option is going above than normal, usually asset paths in console logs are just text)

[ekcoh]

Is the asset path supposed to be clickable and point to that asset? As it does not do anything right now, I suggest it either should not be highlighted or should correctly select that asset (but the second option is going above than normal, usually asset paths in console logs are just text)

The console URL isn't but the full preview below is (this is how Unity seem to handle hyperlinks). However I realize a drawback is that it opens in free-floating editor. I do not see a way around that, should I remove it?

https://github.com/Unity-Technologies/InputSystem/assets/8974064/08b40c88-ddfe-454f-b294-18c551c3c530

[ekcoh]

Another question for reviewers, should we stop at just reporting the missing map when the map is missing (and skip reporting actions to limit console flooding) or should we always report all (current behavior)?

[Pauliusd01]

Is the asset path supposed to be clickable and point to that asset? As it does not do anything right now, I suggest it either should not be highlighted or should correctly select that asset (but the second option is going above than normal, usually asset paths in console logs are just text)

The console URL isn't but the full preview below is (this is how Unity seem to handle hyperlinks). However I realize a drawback is that it opens in free-floating editor. I do not see a way around that, should I remove it? Clicking.Link.mov

Yep I'm aware but it does not work on Windows in either field. The doc links work fine but that specific asset path does not

[Pauliusd01]

Another question for reviewers, should we stop at just reporting the missing map when the map is missing (and skip reporting actions to limit console flooding) or should we always report all (current behavior)?

That sounds nice, my vote goes to - whole map missing = 1 warning asking you to restore it and pass on each individual action warnings, actions from that map missing = 1 warning for each instance.

[graham-huws]

Another question for reviewers, should we stop at just reporting the missing map when the map is missing (and skip reporting actions to limit console flooding) or should we always report all (current behavior)?

That sounds nice, my vote goes to - whole map missing = 1 warning asking you to restore it and pass on each individual action warnings, actions from that map missing = 1 warning for each instance.

This sounds good to me. I guess this also goes back to the question of whether there's ever a valid reason for a user to remove the default UI actions? Maybe if they'd interfere with their own custom solution? Perhaps, if there could be a reason, adding a tickbox for "don't warn about this" would be good. If there's no good reason for it, better to focus on stopping users from being able to delete them, and/or automatically adding them to any fresh input assets?

Both probably out of scope for this PR, though 😃

[ekcoh]

Another question for reviewers, should we stop at just reporting the missing map when the map is missing (and skip reporting actions to limit console flooding) or should we always report all (current behavior)?

That sounds nice, my vote goes to - whole map missing = 1 warning asking you to restore it and pass on each individual action warnings, actions from that map missing = 1 warning for each instance.

I will make an update changing this unless someone adds an objection around it to avoid flooding the console when missing.

[ekcoh]

This sounds good to me. I guess this also goes back to the question of whether there's ever a valid reason for a user to remove the default UI actions? Maybe if they'd interfere with their own custom solution? Perhaps, if there could be a reason, adding a tickbox for "don't warn about this" would be good. If there's no good reason for it, better to focus on stopping users from being able to delete them, and/or automatically adding them to any fresh input assets?

Both probably out of scope for this PR, though 😃

I also feel like that would be good, wonder where to fit it though, likely importer setting or editorbuildpref (but weird fit). Probably best to leave to another PR if we change that.

[ekcoh]

When you delete the Click action from the Project Settings asset, you get a warning in the console that reads The UI action 'Click' name has been modified. This will break the UI input at runtime. Please make sure the action name with 'Click' exists. UnityEngine.UnitySynchronizationContext:ExecuteTasks () (at C:/Code/unity3/Runtime/Export/Scripting/UnitySynchronizationContext.cs:109)

Did you really test this branch then @benoitalain ? That message and logic is no longer present on this branch, sounds like you tested something else?

[ekcoh]

@benoitalain all points sounds like you tested something else, could you double check you tested this branch and behavior and feedback on that and I will follow-up?

[benoitalain]

@benoitalain all points sounds like you tested something else, could you double check you tested this branch and behavior and feedback on that and I will follow-up?

Oh god, you're right! My checkout command somehow didn't pan out and I assumed it had, proceeded to test, and since I knew nothing of the situation I didn't know I was testing the problem not the fix! :-P

Will test again tomorrow, very sorry! On the bright side, at least now I know what we're fixing!

[benoitalain]

@benoit-alain? Are there any degrees of freedom or what could cause issues on consumer side?

We will very likely want to add new UI actions in future versions of trunk. PageUp/PageDown is an example of an action that we might want to extend to the Gamepad, for instance, and as such we will probably introduce a new UI action for that eventually. If that's the case,

1. Can we automatically upgrade the user's project-wide asset to include default mappings for the new UI action?
2. What happens if the user voluntarily deletes their new default mappings, we should probably not add them back against their will?
3. Unit tests for those two scenarios

[ekcoh]

Regarding @Pauliusd01

Yep I'm aware but it does not work on Windows in either field. The doc links work fine but that specific asset path does not I tested it on Windows and it works for me (also with UNIX path separator), are you sure it didn't work for you in the console log preview part of the window?

[ekcoh]

@benoitalain Related to your multiple feedback comments I am reconsidering some aspects of this fix. Additionally I found non reliable behavior tied to InitializeOnLoad vs RuntimeInitializeOnLoad and for scenarios called out. I think it would be desirable to have the warnings directly in the editor when editing that way but have the console warnings when just running. There is a dependency between InputSystemProvider and its registration tied to scene load and presence of UIDocuments which needs to be decoupled from requirements on the PWA if present. There is also lacking detecting and fix support for existing PWAs. I will attempt updating this PR with remedies for the above but am exploring a derivative from this PR at the moment. Will update when conclusions are final.

[ekcoh]

Thanks for the clarification on the manual @duckets, very welcome, however I am still waiting for confirmation that the configuration is matching expectations from UI consumer side? @benoitalain E.g. what implicit type or callback constraints are of relevance to the consuming side here. E.g. having Value or Passthrough or Button make a behavioural difference and expected control type implies a type expectation.

[benoitalain]

Thanks for the clarification on the manual @duckets, very welcome, however I am still waiting for confirmation that the configuration is matching expectations from UI consumer side? @benoitalain E.g. what implicit type or callback constraints are of relevance to the consuming side here. E.g. having Value or Passthrough or Button make a behavioural difference and expected control type implies a type expectation.

I'll rewrite my channel answer here for visibility.

As far as action type, state check and control type, if this is the same as it was in 1.7.0, but in a visible asset, then that should be good. From what I've seen so far, everything works for us as it is right now, and you, Dmytro, Lyndon and myself had gone through a long testing and validating process when we worked on the first iteration of InputForUI.

Here are our requirements for the future

1. The ability to expose Tab/Shift+Tab actions in the asset and make them reconfigurable (there's a bug where Shift+Tab and Tab actions both get fired at the same time if they're in an action map IIRC)
2. The ability to add new actions later and upgrade the user's project-wide asset with a smooth enough process
3. A path to fixing bugs in the PWA if we find out we didn't do it perfectly on the first try after all

Point (3) I just added in reaction to your question about the behavioural differences. It could be having a popup that tells the user "the defaults settings for the UI/Click action have changed since the last version of Unity, do you want to (a) upgrade to the new default settings, (b) keep your current settings untouched? If you're unsure, here's a link to the documentation." The differences between Value, Passthrough, Button aren't very familiar to me yet, I mainly tested the result and saw that it seemed normal. We definitely need a way to readjust if we find later that this isn't perfect.

Also, our unit tests don't current validate the "quality" of the input. They merely check that when we receive a PointerEvent of type ButtonPressed, it starts clicking on our UI, when we receive a NavigationEvent in some direction, it moves our focus, etc. If any, it's the InputSystem tests that currently validate that pushing an analog stick just a little bit to the right should or shouldn't generate a NavigationEvent to the right. Do you think that this testing and quality assurance should belong to the UI team ultimately?

[Pauliusd01]

Regarding @Pauliusd01

Yep I'm aware but it does not work on Windows in either field. The doc links work fine but that specific asset path does not I tested it on Windows and it works for me (also with UNIX path separator), are you sure it didn't work for you in the console log preview part of the window?

Yes, here's what it looks like to me: https://github.com/Unity-Technologies/InputSystem/assets/54306142/64c46d58-a09f-4561-a76b-7daafe418042 (in the video I'm in Play mode the entire time but that does not have any change by the way, does not work with it running or not)

[ekcoh]

We will very likely want to add new UI actions in future versions of trunk. PageUp/PageDown is an example of an action that we might want to extend to the Gamepad, for instance, and as such we will probably introduce a new UI action for that eventually. If that's the case,

Can we automatically upgrade the user's project-wide asset to include default mappings for the new UI action?

Not with this PR but I have captured this into continuation work in ISX-1969. This desired behaviour suggests that what you would like to see is not that we plainly generate warning and ignore in case of missing action, you would rather see a default binding being generated for the action to provide support anyway (possible with a warning?). I would recommend we continue this discussion for a future update. As a short answer I believe there could be benefit of integrating requirement and warning feedback into the editor instead and for this scenario it would make more sense to generate default bindings where missing but show visually what requirements exist and provide convenience tools to add missing customisations to the asset. I believe its out of scope of this PR.

What happens if the user voluntarily deletes their new default mappings, we should probably not add them back against their will?

At the moment we think this could create more friction than help.

Unit tests for those two scenarios

Absolutely when behaviour is strictly defined. (Continuation work)

[ekcoh]

I'll rewrite my channel answer here for visibility.

As far as action type, state check and control type, if this is the same as it was in 1.7.0, but in a visible asset, then that should be good. From what I've seen so far, everything works for us as it is right now, and you, Dmytro, Lyndon and myself had gone through a long testing and validating process when we worked on the first iteration of InputForUI.

Here are our requirements for the future

The ability to expose Tab/Shift+Tab actions in the asset and make them reconfigurable (there's a bug where Shift+Tab and Tab actions both get fired at the same time if they're in an action map IIRC) The ability to add new actions later and upgrade the user's project-wide asset with a smooth enough process A path to fixing bugs in the PWA if we find out we didn't do it perfectly on the first try after all Point (3) I just added in reaction to your question about the behavioural differences. It could be having a popup that tells the user "the defaults settings for the UI/Click action have changed since the last version of Unity, do you want to (a) upgrade to the new default settings, (b) keep your current settings untouched? If you're unsure, here's a link to the documentation." The differences between Value, Passthrough, Button aren't very familiar to me yet, I mainly tested the result and saw that it seemed normal. We definitely need a way to readjust if we find later that this isn't perfect.

Also, our unit tests don't current validate the "quality" of the input. They merely check that when we receive a PointerEvent of type ButtonPressed, it starts clicking on our UI, when we receive a NavigationEvent in some direction, it moves our focus, etc. If any, it's the InputSystem tests that currently validate that pushing an analog stick just a little bit to the right should or shouldn't generate a NavigationEvent to the right. Do you think that this testing and quality assurance should belong to the UI team ultimately?

Lets follow this up outside this PR. I will file initial ideas to the ticket mentioned previously in this post.

[ekcoh]

I have done some updates to this PR:

https://github.com/Unity-Technologies/InputSystem/pull/1903/commits/98de28874d41e7e3e35119ef2f0e1dd34f749081 - To avoid a warning about unreachable code in tests generated by compiler.

https://github.com/Unity-Technologies/InputSystem/pull/1903/commits/2394e9cd32375cbd063b5e00bf37e808e52f3c28 - Fixes hyperlink quote called out by @benoitalain in review.

https://github.com/Unity-Technologies/InputSystem/pull/1903/commits/64270ecabb8b0875b17c8b7fa8140baba3f1507f - Based on previous discussion allow defaults to be used if the UI map is completely missing. Otherwise any user with custom PWA asset might not be able to use UITK.

https://github.com/Unity-Technologies/InputSystem/pull/1903/commits/199c341e306ab5931252c5f382e5ba5f5037be72 - Avoid verifier registration to be tied to scene load / run-time load. Instead load on domain reload in editor. If not, the verifier might not be registered after a domain reload.

Additionally I also removed the hyperlinked asset path due to inconsistent behavior being reported by @Pauliusd01 .

I think it makes sense to retest this one @Pauliusd01 and then it could land. I decided any further work on this is better handled outside of this PR and this planned release since it will be a large set of work.

[Pauliusd01]

Did a quick re-check, seems good to me. The UITK UI works correctly even when UI map is deleted or renamed