Skip to content

FIX: Delayed input backend prompt to make sure asset import is not interrupted (ISXB-608) #2107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 17, 2025
Merged

FIX: Delayed input backend prompt to make sure asset import is not interrupted (ISXB-608) #2107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9f42a2b
delayed prompt to delay Call to make sure assets are imported
ritamerkl Jan 15, 2025
b97e334
Merge branch 'develop' into backend-prompt-interrupts-asset-import
ritamerkl Jan 16, 2025
4974238
added changelog entry
ritamerkl Jan 16, 2025
cd764b9
formatting fix
ritamerkl Jan 16, 2025
2d3243d
Merge branch 'develop' into backend-prompt-interrupts-asset-import
ritamerkl Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ however, it has to be formatted properly to pass verification tests.
## [Unreleased] - yyyy-mm-dd

### Fixed
- Fixed an issue where the prompt to enable the InputSystem backends would interrupt the import of large assets.
- Fixed Cut Mode for Action Maps and Actions to make renaming disabled. [ISXB-1155](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1155)


## [1.12.0] - 2025-01-15

### Fixed
Expand Down
30 changes: 18 additions & 12 deletions Packages/com.unity.inputsystem/InputSystem/InputSystem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3599,14 +3599,29 @@ internal static void InitializeInEditor(IInputRuntime runtime = null)
}

Debug.Assert(settings != null);
#if UNITY_EDITOR
Debug.Assert(EditorUtility.InstanceIDToObject(settings.GetInstanceID()) != null,
"InputSettings has lost its native object");
#endif

// If native backends for new input system aren't enabled, ask user whether we should
// enable them (requires restart). We only ask once per session and don't ask when
// running in batch mode.
// The warning is delayed to delay call (called a short while after the Asset are loaded, on Inspector update) to make sure it doesn't pop up while the editor is still loading or assets are not fully loaded -
// this would cancel the import of large assets that are dependent on the InputSystem package and import it as a dependency.
EditorApplication.delayCall += ShowRestartWarning;
Copy link
Collaborator

@ekcoh ekcoh Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this guarantee that it will execute after asset import? I would have expected this to be triggered by an AssetP since this one will simply but the callback on the editor event queue?
If it works its great, but felt I was not seeing how this is robust.

Copy link
Collaborator Author

@ritamerkl ritamerkl Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AssetPostProcessor was an other option I considered, but it requires another class to inherit from AssetPostprocessor, so I decided for delayedCall instead. delayCall is called after domain reloads (which include importing of assets at the end) when the inspector windows refresh, which I think is suitable for a prompt. I don't see why it's not robust, in which way do you think it couldn't be?

Copy link
Collaborator

@ekcoh ekcoh Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, yeah we have some classes doing that as part of InputActionImporter. If delayCall runs after domain reloads it's good, but to me it wasn't clear it runs after asset import (which might not trigger a domain reload if no code changed?). At least it's not explicit from https://docs.unity3d.com/6000.0/Documentation/ScriptReference/EditorApplication-delayCall.html, so it seems to relate to inspectors but not necessarily asset imports? Maybe you know something I do not in this area, but to me it wasn't clear that e.g. delayCall could be executed between two imports.

Copy link
Collaborator Author

@ritamerkl ritamerkl Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since InitializeInEditor is called from the static constructor it's only executed when the editor launches or the package gets added into the project (afaik - correct me if I forgot a case), in both cases there is a domain reload happening.
Here a short explanation why I think the call is reliable:
The callbacks in delayCall are invoked in the subsequent editor update.
Editor updates do not happen during a domain reload (which includes import of assets - no matter how many), Editor updates resume only after the domain reload is complete.
One reason for this is that Editor updates depend on stable managed objects and assemblies. Since these objects are being reloaded, updates during this period could result in crashes or undefined behavior.


#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
Copy link
Collaborator

@ekcoh ekcoh Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also be great to understand why this was moved out of its previous place?

Copy link
Collaborator Author

@ritamerkl ritamerkl Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it into a private function of the same class to make it more readable, I think having big chunks of code in single functions do no good for the readability some times. (technically still in the same place though)

Copy link
Collaborator

@ekcoh ekcoh Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I see the function you are referring to? What I meant was basically that previously EnableActions(), RunInitialUpdate(), was executed from within ShowRestartWarning() as part of the deferred ShowRestartWarning. Now it executes directly in InitializeInEditor (before) code in ShowRestartWarning(). Hence why I asked if it still provides the same behavior or what the reason was for relocating it / changing the execution order.

Copy link
Collaborator Author

@ritamerkl ritamerkl Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It all is still in the same order, it may look a bit confusing just looking at the diffs, but the only thing I changed is outsourcing the logic of the prompt to a new private function called ShowRestartWarning()

// Make sure project wide input actions are enabled.
// Note that this will always fail if entering play-mode within editor since not yet in play-mode.
EnableActions();
#endif

RunInitialUpdate();

k_InputInitializeInEditorMarker.End();
}

private static void ShowRestartWarning()
{
if (!s_SystemObject.newInputBackendsCheckedAsEnabled &&
!EditorPlayerSettingHelpers.newSystemBackendsEnabled &&
!s_Manager.m_Runtime.isInBatchMode)
Expand All @@ -3622,16 +3637,7 @@ internal static void InitializeInEditor(IInputRuntime runtime = null)
}
}
s_SystemObject.newInputBackendsCheckedAsEnabled = true;

#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
// Make sure project wide input actions are enabled.
// Note that this will always fail if entering play-mode within editor since not yet in play-mode.
EnableActions();
#endif

RunInitialUpdate();

k_InputInitializeInEditorMarker.End();
EditorApplication.delayCall -= ShowRestartWarning;
}

internal static void OnPlayModeChange(PlayModeStateChange change)
Expand Down
Loading