From 62ceae40cdb2cb1f3ece4de557428eccd934fb61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ha=CC=8Akan=20Sidenvall?= Date: Fri, 21 Jun 2024 08:58:49 +0200 Subject: [PATCH 1/6] FIX: ISXB-925 Fixed an issue where changing InputSettings instance for the Input System wouldn't affect feature flags. --- Packages/com.unity.inputsystem/CHANGELOG.md | 1 + .../InputSystem/Controls/InputControl.cs | 12 +++---- .../InputSystem/InputManager.cs | 15 +++++++-- .../InputSystem/InputSettings.cs | 33 ++++--------------- 4 files changed, 27 insertions(+), 34 deletions(-) diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md index bf5b05c77b..0c97bc0198 100644 --- a/Packages/com.unity.inputsystem/CHANGELOG.md +++ b/Packages/com.unity.inputsystem/CHANGELOG.md @@ -25,6 +25,7 @@ however, it has to be formatted properly to pass verification tests. - Fixed an issue where adding a `OnScreenButton` or `OnScreenStick` to a regular GameObject would lead to exception in editor. - Fixed an issue where adding a `OnScreenStick` to a regular GameObject and entering play-mode would lead to exceptions being generated. - Fixed InputActionReference issues when domain reloads are disabled [ISXB-601](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-601), [ISXB-718](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-718), [ISXB-900](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-900) +- Fixed an issue where changing `InputSettings` instance would not affect associated feature flags. ### Added - Added additional device information when logging the error due to exceeding the maximum number of events processed diff --git a/Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs b/Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs index a9500197b6..c5693b716e 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs @@ -929,7 +929,7 @@ public void ApplyParameterChanges() private void SetOptimizedControlDataType() { // setting check need to be inline so we clear optimizations if setting is disabled after the fact - m_OptimizedControlDataType = InputSettings.optimizedControlsFeatureEnabled + m_OptimizedControlDataType = InputSystem.s_Manager.m_OptimizedControlsFeatureEnabled ? CalculateOptimizedControlDataType() : (FourCC)InputStateBlock.kFormatInvalid; } @@ -957,7 +957,7 @@ internal void SetOptimizedControlDataTypeRecursively() [Conditional("UNITY_EDITOR")] internal void EnsureOptimizationTypeHasNotChanged() { - if (!InputSettings.optimizedControlsFeatureEnabled) + if (!InputSystem.s_Manager.m_OptimizedControlsFeatureEnabled) return; var currentOptimizedControlDataType = CalculateOptimizedControlDataType(); @@ -1172,7 +1172,7 @@ public ref readonly TValue value if ( // if feature is disabled we re-evaluate every call - !InputSettings.readValueCachingFeatureEnabled + !InputSystem.s_Manager.m_ReadValueCachingFeatureEnabled // if cached value is stale we re-evaluate and clear the flag || m_CachedValueIsStale // if a processor in stack needs to be re-evaluated, but unprocessedValue is still can be cached @@ -1183,7 +1183,7 @@ public ref readonly TValue value m_CachedValueIsStale = false; } #if DEBUG - else if (InputSettings.paranoidReadValueCachingChecksEnabled) + else if (InputSystem.s_Manager.m_ParanoidReadValueCachingChecksEnabled) { var oldUnprocessedValue = m_UnprocessedCachedValue; var newUnprocessedValue = unprocessedValue; @@ -1225,7 +1225,7 @@ internal unsafe ref readonly TValue unprocessedValue if ( // if feature is disabled we re-evaluate every call - !InputSettings.readValueCachingFeatureEnabled + !InputSystem.s_Manager.m_ReadValueCachingFeatureEnabled // if cached value is stale we re-evaluate and clear the flag || m_UnprocessedCachedValueIsStale ) @@ -1234,7 +1234,7 @@ internal unsafe ref readonly TValue unprocessedValue m_UnprocessedCachedValueIsStale = false; } #if DEBUG - else if (InputSettings.paranoidReadValueCachingChecksEnabled) + else if (InputSystem.s_Manager.m_ParanoidReadValueCachingChecksEnabled) { var currentUnprocessedValue = ReadUnprocessedValueFromState(currentStatePtr); if (CompareValue(ref currentUnprocessedValue, ref m_UnprocessedCachedValue)) diff --git a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs index 5bee4ac31a..709f07ca08 100644 --- a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs +++ b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs @@ -2116,6 +2116,12 @@ internal struct AvailableDevice internal IInputRuntime m_Runtime; internal InputMetrics m_Metrics; internal InputSettings m_Settings; + + // Extract as booleans (from m_Settings) because feature check is in the hot path + internal bool m_OptimizedControlsFeatureEnabled; + internal bool m_ReadValueCachingFeatureEnabled; + internal bool m_ParanoidReadValueCachingChecksEnabled; + #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS private InputActionAsset m_Actions; #endif @@ -2651,6 +2657,11 @@ internal void ApplySettings() if (ExecuteGlobalCommand(ref command) < 0) Debug.LogError($"Could not enable Windows.Gaming.Input"); } + + // Extract feature flags into fields since used in hot-path + m_ReadValueCachingFeatureEnabled = m_Settings.IsFeatureEnabled((InputFeatureNames.kUseReadValueCaching)); + m_OptimizedControlsFeatureEnabled = m_Settings.IsFeatureEnabled((InputFeatureNames.kUseOptimizedControls)); + m_ParanoidReadValueCachingChecksEnabled = m_Settings.IsFeatureEnabled((InputFeatureNames.kParanoidReadValueCachingChecks)); } // Cache some values. @@ -3549,7 +3560,7 @@ private void ResetCurrentProcessedEventBytesForDevices() [Conditional("UNITY_EDITOR")] void CheckAllDevicesOptimizedControlsHaveValidState() { - if (!InputSettings.optimizedControlsFeatureEnabled) + if (!InputSystem.s_Manager.m_OptimizedControlsFeatureEnabled) return; foreach (var device in devices) @@ -3739,7 +3750,7 @@ private unsafe void WriteStateChange(InputStateBuffers.DoubleBuffers buffers, in deviceStateSize); } - if (InputSettings.readValueCachingFeatureEnabled) + if (InputSystem.s_Manager.m_ReadValueCachingFeatureEnabled) { // if the buffers have just been flipped, and we're doing a full state update, then the state from the // previous update is now in the back buffer, and we should be comparing to that when checking what diff --git a/Packages/com.unity.inputsystem/InputSystem/InputSettings.cs b/Packages/com.unity.inputsystem/InputSystem/InputSettings.cs index c9cdafa369..e79bd68402 100644 --- a/Packages/com.unity.inputsystem/InputSystem/InputSettings.cs +++ b/Packages/com.unity.inputsystem/InputSystem/InputSettings.cs @@ -717,27 +717,13 @@ public void SetInternalFeatureFlag(string featureName, bool enabled) if (string.IsNullOrEmpty(featureName)) throw new ArgumentNullException(nameof(featureName)); - switch (featureName) - { - case InputFeatureNames.kUseOptimizedControls: - optimizedControlsFeatureEnabled = enabled; - break; - case InputFeatureNames.kUseReadValueCaching: - readValueCachingFeatureEnabled = enabled; - break; - case InputFeatureNames.kParanoidReadValueCachingChecks: - paranoidReadValueCachingChecksEnabled = enabled; - break; - default: - if (m_FeatureFlags == null) - m_FeatureFlags = new HashSet(); - - if (enabled) - m_FeatureFlags.Add(featureName.ToUpperInvariant()); - else - m_FeatureFlags.Remove(featureName.ToUpperInvariant()); - break; - } + if (m_FeatureFlags == null) + m_FeatureFlags = new HashSet(); + + if (enabled) + m_FeatureFlags.Add(featureName.ToUpperInvariant()); + else + m_FeatureFlags.Remove(featureName.ToUpperInvariant()); OnChange(); } @@ -778,11 +764,6 @@ internal bool IsFeatureEnabled(string featureName) return m_FeatureFlags != null && m_FeatureFlags.Contains(featureName.ToUpperInvariant()); } - // Needs a static field because feature check is in the hot path - internal static bool optimizedControlsFeatureEnabled = false; - internal static bool readValueCachingFeatureEnabled; - internal static bool paranoidReadValueCachingChecksEnabled; - internal void OnChange() { if (InputSystem.settings == this) From 4d9cf2be3ecf44c5ad45b3662b510cacb4779a9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ha=CC=8Akan=20Sidenvall?= Date: Sun, 23 Jun 2024 21:35:19 +0200 Subject: [PATCH 2/6] FIX: ISXB-927 Fix for ignoring attempting to set Windows Gaming Input runtime IOCTL command flag on non-Windows platforms leading to debug error log entries. --- Packages/com.unity.inputsystem/InputSystem/InputManager.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs index 709f07ca08..4550de1a9f 100644 --- a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs +++ b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs @@ -2651,12 +2651,14 @@ internal void ApplySettings() runPlayerUpdatesInEditMode = m_Settings.IsFeatureEnabled(InputFeatureNames.kRunPlayerUpdatesInEditMode); #endif + #if UNITY_EDITOR_WIN || UNITY_STANDALONE_WIN if (m_Settings.IsFeatureEnabled(InputFeatureNames.kUseWindowsGamingInputBackend)) { var command = UseWindowsGamingInputCommand.Create(true); if (ExecuteGlobalCommand(ref command) < 0) Debug.LogError($"Could not enable Windows.Gaming.Input"); } + #endif // Extract feature flags into fields since used in hot-path m_ReadValueCachingFeatureEnabled = m_Settings.IsFeatureEnabled((InputFeatureNames.kUseReadValueCaching)); From ab5ff848514471a787724ac86bda3d70f24e68a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ha=CC=8Akan=20Sidenvall?= Date: Sun, 23 Jun 2024 21:36:22 +0200 Subject: [PATCH 3/6] Added test case for problematic scenario. --- Assets/Tests/InputSystem/CoreTests_Actions.cs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/Assets/Tests/InputSystem/CoreTests_Actions.cs b/Assets/Tests/InputSystem/CoreTests_Actions.cs index c4ae3b7c5d..23fbff8cc0 100644 --- a/Assets/Tests/InputSystem/CoreTests_Actions.cs +++ b/Assets/Tests/InputSystem/CoreTests_Actions.cs @@ -30,6 +30,33 @@ // in terms of complexity. partial class CoreTests { + // ISXB-925: Feature flag values should live with containing settings instance. + [TestCase(InputFeatureNames.kUseReadValueCaching)] + [TestCase(InputFeatureNames.kUseOptimizedControls)] + [TestCase(InputFeatureNames.kParanoidReadValueCachingChecks)] + [TestCase(InputFeatureNames.kUseWindowsGamingInputBackend)] + [TestCase(InputFeatureNames.kDisableUnityRemoteSupport)] + [TestCase(InputFeatureNames.kRunPlayerUpdatesInEditMode)] + [TestCase(InputFeatureNames.kUseIMGUIEditorForAssets)] + public void Settings_ShouldStoreSettingsAndFeatureFlags(string featureName) + { + using (var settings = Scoped.Asset(InputSettings.CreateInstance())) + { + InputSystem.settings = settings.value; + + Assert.That(InputSystem.settings.IsFeatureEnabled(featureName), Is.False); + settings.value.SetInternalFeatureFlag(featureName, true); + Assert.That(InputSystem.settings.IsFeatureEnabled(featureName), Is.True); + + using (var other = Scoped.Asset(InputSettings.CreateInstance())) + { + InputSystem.settings = other.value; + + Assert.That(InputSystem.settings.IsFeatureEnabled(featureName), Is.False); + } + } + } + [Test] [Category("Actions")] public void Actions_WhenShortcutsDisabled_AllConflictingActionsTrigger() From d29453955f8bbe5ce67449e2c115d286767a6d17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ha=CC=8Akan=20Sidenvall?= Date: Mon, 24 Jun 2024 14:21:21 +0200 Subject: [PATCH 4/6] Added conditional compilation check --- Assets/Tests/InputSystem/CoreTests_Actions.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Assets/Tests/InputSystem/CoreTests_Actions.cs b/Assets/Tests/InputSystem/CoreTests_Actions.cs index 23fbff8cc0..0fc7083fe1 100644 --- a/Assets/Tests/InputSystem/CoreTests_Actions.cs +++ b/Assets/Tests/InputSystem/CoreTests_Actions.cs @@ -37,7 +37,9 @@ partial class CoreTests [TestCase(InputFeatureNames.kUseWindowsGamingInputBackend)] [TestCase(InputFeatureNames.kDisableUnityRemoteSupport)] [TestCase(InputFeatureNames.kRunPlayerUpdatesInEditMode)] + #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS [TestCase(InputFeatureNames.kUseIMGUIEditorForAssets)] + #endif public void Settings_ShouldStoreSettingsAndFeatureFlags(string featureName) { using (var settings = Scoped.Asset(InputSettings.CreateInstance())) From db67d57d24d16e51414a08c4fe8399522c8a2752 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ha=CC=8Akan=20Sidenvall?= Date: Mon, 24 Jun 2024 14:51:48 +0200 Subject: [PATCH 5/6] Corrected incorrect use of Scoped.Asset instead of Scoped.Object --- Assets/Tests/InputSystem/CoreTests_Actions.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Assets/Tests/InputSystem/CoreTests_Actions.cs b/Assets/Tests/InputSystem/CoreTests_Actions.cs index 0fc7083fe1..65614d45d5 100644 --- a/Assets/Tests/InputSystem/CoreTests_Actions.cs +++ b/Assets/Tests/InputSystem/CoreTests_Actions.cs @@ -42,7 +42,7 @@ partial class CoreTests #endif public void Settings_ShouldStoreSettingsAndFeatureFlags(string featureName) { - using (var settings = Scoped.Asset(InputSettings.CreateInstance())) + using (var settings = Scoped.Object(InputSettings.CreateInstance())) { InputSystem.settings = settings.value; @@ -50,7 +50,7 @@ public void Settings_ShouldStoreSettingsAndFeatureFlags(string featureName) settings.value.SetInternalFeatureFlag(featureName, true); Assert.That(InputSystem.settings.IsFeatureEnabled(featureName), Is.True); - using (var other = Scoped.Asset(InputSettings.CreateInstance())) + using (var other = Scoped.Object(InputSettings.CreateInstance())) { InputSystem.settings = other.value; From c93e565d0b9260c0911f2af882871dd8551fe80a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ha=CC=8Akan=20Sidenvall?= Date: Tue, 25 Jun 2024 11:16:14 +0200 Subject: [PATCH 6/6] Changes scope of optimization flags to private scope and added internal getters instead. Removed WindowsGamingInputBackend feature flag from test since generating error. This is handled in a separate PR as well. --- Assets/Tests/InputSystem/CoreTests_Actions.cs | 1 - .../InputSystem/Controls/InputControl.cs | 12 +++--- .../InputSystem/InputManager.cs | 37 +++++++++++++------ 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/Assets/Tests/InputSystem/CoreTests_Actions.cs b/Assets/Tests/InputSystem/CoreTests_Actions.cs index 65614d45d5..00e2f5747b 100644 --- a/Assets/Tests/InputSystem/CoreTests_Actions.cs +++ b/Assets/Tests/InputSystem/CoreTests_Actions.cs @@ -34,7 +34,6 @@ partial class CoreTests [TestCase(InputFeatureNames.kUseReadValueCaching)] [TestCase(InputFeatureNames.kUseOptimizedControls)] [TestCase(InputFeatureNames.kParanoidReadValueCachingChecks)] - [TestCase(InputFeatureNames.kUseWindowsGamingInputBackend)] [TestCase(InputFeatureNames.kDisableUnityRemoteSupport)] [TestCase(InputFeatureNames.kRunPlayerUpdatesInEditMode)] #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS diff --git a/Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs b/Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs index c5693b716e..38a6f16b0b 100644 --- a/Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs +++ b/Packages/com.unity.inputsystem/InputSystem/Controls/InputControl.cs @@ -929,7 +929,7 @@ public void ApplyParameterChanges() private void SetOptimizedControlDataType() { // setting check need to be inline so we clear optimizations if setting is disabled after the fact - m_OptimizedControlDataType = InputSystem.s_Manager.m_OptimizedControlsFeatureEnabled + m_OptimizedControlDataType = InputSystem.s_Manager.optimizedControlsFeatureEnabled ? CalculateOptimizedControlDataType() : (FourCC)InputStateBlock.kFormatInvalid; } @@ -957,7 +957,7 @@ internal void SetOptimizedControlDataTypeRecursively() [Conditional("UNITY_EDITOR")] internal void EnsureOptimizationTypeHasNotChanged() { - if (!InputSystem.s_Manager.m_OptimizedControlsFeatureEnabled) + if (!InputSystem.s_Manager.optimizedControlsFeatureEnabled) return; var currentOptimizedControlDataType = CalculateOptimizedControlDataType(); @@ -1172,7 +1172,7 @@ public ref readonly TValue value if ( // if feature is disabled we re-evaluate every call - !InputSystem.s_Manager.m_ReadValueCachingFeatureEnabled + !InputSystem.s_Manager.readValueCachingFeatureEnabled // if cached value is stale we re-evaluate and clear the flag || m_CachedValueIsStale // if a processor in stack needs to be re-evaluated, but unprocessedValue is still can be cached @@ -1183,7 +1183,7 @@ public ref readonly TValue value m_CachedValueIsStale = false; } #if DEBUG - else if (InputSystem.s_Manager.m_ParanoidReadValueCachingChecksEnabled) + else if (InputSystem.s_Manager.paranoidReadValueCachingChecksEnabled) { var oldUnprocessedValue = m_UnprocessedCachedValue; var newUnprocessedValue = unprocessedValue; @@ -1225,7 +1225,7 @@ internal unsafe ref readonly TValue unprocessedValue if ( // if feature is disabled we re-evaluate every call - !InputSystem.s_Manager.m_ReadValueCachingFeatureEnabled + !InputSystem.s_Manager.readValueCachingFeatureEnabled // if cached value is stale we re-evaluate and clear the flag || m_UnprocessedCachedValueIsStale ) @@ -1234,7 +1234,7 @@ internal unsafe ref readonly TValue unprocessedValue m_UnprocessedCachedValueIsStale = false; } #if DEBUG - else if (InputSystem.s_Manager.m_ParanoidReadValueCachingChecksEnabled) + else if (InputSystem.s_Manager.paranoidReadValueCachingChecksEnabled) { var currentUnprocessedValue = ReadUnprocessedValueFromState(currentStatePtr); if (CompareValue(ref currentUnprocessedValue, ref m_UnprocessedCachedValue)) diff --git a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs index 4550de1a9f..02dc53bd18 100644 --- a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs +++ b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using System.Runtime.CompilerServices; using System.Text; using Unity.Collections; using UnityEngine.InputSystem.Composites; @@ -2118,9 +2119,30 @@ internal struct AvailableDevice internal InputSettings m_Settings; // Extract as booleans (from m_Settings) because feature check is in the hot path - internal bool m_OptimizedControlsFeatureEnabled; - internal bool m_ReadValueCachingFeatureEnabled; - internal bool m_ParanoidReadValueCachingChecksEnabled; + + private bool m_OptimizedControlsFeatureEnabled; + internal bool optimizedControlsFeatureEnabled + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => m_OptimizedControlsFeatureEnabled; + set => m_OptimizedControlsFeatureEnabled = value; + } + + private bool m_ReadValueCachingFeatureEnabled; + internal bool readValueCachingFeatureEnabled + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => m_ReadValueCachingFeatureEnabled; + set => m_ReadValueCachingFeatureEnabled = value; + } + + private bool m_ParanoidReadValueCachingChecksEnabled; + internal bool paranoidReadValueCachingChecksEnabled + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => m_ParanoidReadValueCachingChecksEnabled; + set => m_ParanoidReadValueCachingChecksEnabled = value; + } #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS private InputActionAsset m_Actions; @@ -2651,15 +2673,6 @@ internal void ApplySettings() runPlayerUpdatesInEditMode = m_Settings.IsFeatureEnabled(InputFeatureNames.kRunPlayerUpdatesInEditMode); #endif - #if UNITY_EDITOR_WIN || UNITY_STANDALONE_WIN - if (m_Settings.IsFeatureEnabled(InputFeatureNames.kUseWindowsGamingInputBackend)) - { - var command = UseWindowsGamingInputCommand.Create(true); - if (ExecuteGlobalCommand(ref command) < 0) - Debug.LogError($"Could not enable Windows.Gaming.Input"); - } - #endif - // Extract feature flags into fields since used in hot-path m_ReadValueCachingFeatureEnabled = m_Settings.IsFeatureEnabled((InputFeatureNames.kUseReadValueCaching)); m_OptimizedControlsFeatureEnabled = m_Settings.IsFeatureEnabled((InputFeatureNames.kUseOptimizedControls));