Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ public void Report(string message)
}
}

[Test(Description = "Verifies that the default asset do not generate any verification errors (Regardless of existing requirements)")]
[Test(Description = "Verifies that the default asset does not generate any verification errors (Regardless of existing requirements)")]
[Category(kTestCategory)]
public void ProjectWideActions_ShouldSupportAssetVerification_AndHaveNoVerificationErrorsForDefaultAsset()
{
Expand Down
17 changes: 17 additions & 0 deletions Assets/Tests/InputSystem/Plugins/UITests.InputModuleTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,23 @@ public IEnumerator PointerExitChildShouldFullyExit()
Assert.IsTrue(callbackCheck.pointerData.fullyExited == true);
}

[UnityTest]
[Description("Regression test for https://jira.unity3d.com/browse/ISXB-1493")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great you added a specific test for the regression

public IEnumerator DisablingDoesNotResetUserActions()
{
var actions = new DefaultInputActions();
m_InputModule.actionsAsset = actions.asset;
m_InputModule.cancel = InputActionReference.Create(actions.UI.Cancel);

m_InputModule.enabled = false;

yield return null;

Assert.IsNotNull(m_InputModule.cancel, "Disabling component shouldn't lose its data.");

actions.Dispose();
}

public class PointerExitCallbackCheck : MonoBehaviour, IPointerExitHandler
{
public PointerEventData pointerData { get; private set; }
Expand Down
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ however, it has to be formatted properly to pass verification tests.
- Fixed Gamepad stick up/down inputs that were not recognized in WebGL. [ISXB-1090](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1090)
- Fixed reenabling the VirtualMouseInput component may sometimes lead to NullReferenceException. [ISXB-1096](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1096)
- Fixed the default button press point not being respected in Editor (as well as some other Touchscreen properties). [ISXB-1152](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1152)
- Fixed actions being reset when disabling the InputSystemUIInputModule component [ISXB-1493](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1493)
- Fixed PlayerInput component automatically switching away from the default ActionMap set to 'None'.
- Fixed a console error being shown when targeting visionOS builds in 2022.3.
- Fixed a Tap Interaction issue with analog controls. The Tap interaction would keep re-starting after timeout. [ISXB-627](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-627)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1528,6 +1528,17 @@ public InputActionReference trackedDevicePosition
set => SwapAction(ref m_TrackedDevicePositionAction, value, m_ActionsHooked, m_OnTrackedDevicePositionDelegate);
}

// We should dispose the static default actions thing because otherwise it will survive domain reloads
[RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.SubsystemRegistration)]
static void ResetDefaultActions()
{
if (defaultActions != null)
{
defaultActions.Dispose();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess, calling Dispose is just to stay on the safe side since the only data default actions stores is a ref to ScriptableObject that won't survive even with domain reload off. It will be in this intermediate state though, where its native data is gone but the instance is not null until the next gc run (however you'll see it as "null" in debugger, and ==null will be true as well, thanks to Unity's Equals overload for all Object descendants).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this comment imply that it should be Object.ReferenceEquals(null, defaultActions) in if statement above? If its disposable, yes it should be disposed, even if the implementation would do nothing IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I wasn't clear, pardon. DefaultActions is fine to check for null like above since it's not destroyed natively. What is in the intermediate state happens to be aggregated in defaultActions (DefaultActions has a ref to ScriptableObject, so it's the ScriptableObject would have to be checked with ReferenceEquals if we needed to, but we don't).

defaultActions = null;
}
}

/// <summary>
/// Assigns default input actions asset and input actions, similar to how defaults are assigned when creating UI module in editor.
/// Useful for creating <see cref="InputSystemUIInputModule"/> at runtime.
Expand Down Expand Up @@ -1665,7 +1676,12 @@ protected override void OnDisable()
InputActionState.s_GlobalState.onActionControlsChanged.RemoveCallback(m_OnControlsChangedDelegate);
DisableAllActions();
UnhookActions();
UnassignActions();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

^ This line is very wrong, it results in user data being lost on component deactivation.


// In the case we've been initialized with default actions, we want to release them
if (defaultActions != null && defaultActions.asset == actionsAsset)
{
UnassignActions();
}

base.OnDisable();
}
Expand Down