FIX: PWA asset isn't properly restored after deleting Library folder (ISXB-853)

[timkeo]

Description

If the project's Library folder is deleted, the previously assigned Project-wide Actions asset isn't properly restored when launching Unity. The asset is still present, but it's not assigned as the current PWA asset. Re-launching Unity again seems to resolve the issue.

The problem occurs because the logic to load and set the PWA asset runs during InputSystem initialization, before the asset Library has been rebuilt, and the call to load the saved PWA asset from the EditorBuildSettings fails. To fix this, we need to run this operation again after the asset loading has finished.

Changes made

• Attempt to Load and assigns the PWA asset to Inputsystem.actions during OnPostprocessAllAssets()

Notes

I did a little bit of testing with multiple InputAction assets to verify PWA is changed when moving/copying another asset.
Still a little concerned about weird edge-cases, since this operation will run whenever any asset is changed.

Checklist

Before review:

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

[ekcoh]

Lets just consider what we are doing here....

1. var pwaAsset = ProjectWideActionsBuildProvider.actionsToIncludeInPlayerBuild;
   basically this equivalent to accessing EditorBuildPrefs and fetching the current Object reference stored within via. TryGetConfigObject.
2. The if (InputSystem.actions == null && pwaAsset != null) doesn't really do anything since this conditions for discarding a null-reference or missing reference upon assignment is handled within InputSystem.actions setter anyways. Hence this could be removed.
3. Assigning actions if not null AND not a missing reference.

I suspect the problem here is really that initialization in InputSystem.cs for PWA instance happens as you say before ADB is done loading assets. However, it seems also based on this fix that GUID reference stored within EditorBuildPrefs is valid, but basically Unity won't be able to resolve it to an existing object. Hence I suspect that we actually (or I would like us to verify) initialize InputSystem.actions (or rather InputSystem.s_Manager.actions) correctly, just that the object is a missing reference at initialization time? Or is it a true null value? This can be checked by using debugger to evaluate if its implicitly converted to null due to missing reference or if its actually null (which it shouldn't be)?
Hence I believe this code actually doesn't do much except retriggering the cached assignment in InputManager when asset loading is complete which is a fine fix I believe, but I wonder whether this replaces a similar reference with a true reference or an actual null value with a reference. Regardless I think fix is fine.

It would be good to check whether assignments (during init), here https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/InputSystem.cs#L3581 evaluates to true null or missing reference null (implicit) when opening the project.

[ekcoh]

An alternative approach would have been to always rely on TryGetConfig in edit mode and not use any cached value in InputManager. I assume this would avoid this problem in general because the getter would resolve to valid reference as soon as ADB is done withs initialization. However it could still be a problem that there is no onActionChange triggered when ADB is complete so might need something like this anyway.

[ekcoh]

The if statement doesn't hurt but I think it may be safely removed (based on setter checks), hence

if (!Application.isPlaying) InputSystem.actions = ProjectWideActionsBuildProvider.actionsToIncludeInPlayerBuild;

is likely sufficient.

[Pauliusd01]

LGTM, here's some things I checked:

• Whether other, custom assets assigned to PWA field are also restored if library is deleted
• Whether there's any unexpected issues when the field is set to none and library is deleted
• Made sure that PWA actually worked after they've been restored like this
• Whether there's any issues with it attempting to reset PWA field when that asset is actually deleted alongside the Library folder (outside an the Editor and while it is open)
• Checked what happens when I move the PWA asset, whether it attempts to reset anything

Still a little concerned about weird edge-cases, since this operation will run whenever any asset is changed.

Does that mean literally any asset or just input related ones? Is that not a performance concern, however small? Also, if you have any things you want me to check let me know. But I did not really notice it trying to reset anything when I made changes to assets, it stays assigned correctly as expected for me

[graham-huws]

Checked this over with the HDRP template where we were seeing the issue before and this seems to fix it there, nice!

[timkeo]

@Pauliusd01

Does that mean literally any asset or just input related ones?

The callback is fired at the end of every asset "import batch" and not for every asset. The callback can be noisy but I'm not too concerned about perf issues since the code to perform the asset check is fast.

My (minor) concern is around edge-cases for when the callback is fired, i.e. might try to assign the asset when we're not expecting too. Specifically, my original change cases a test failure because this assignment occurred during PlayMode (not allowed to set PWA during PlayMode). I added the if (!Application.isPlaying) to fix the failure, but it made me wonder if there could be other cases.

Worst case though, an exception is thrown and error logged to the console. Otherwise the change is safe.