Skip to content

FIX: ISXB-925 Fixed an issue where changing InputSettings instance fo… #1954

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.

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 10 commits into from
Jun 26, 2024
Merged
28 changes: 28 additions & 0 deletions Assets/Tests/InputSystem/CoreTests_Actions.cs
Original file line number Diff line number Diff line change
@@ -30,6 +30,34 @@
// 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.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.Object(InputSettings.CreateInstance<InputSettings>()))
{
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.Object(InputSettings.CreateInstance<InputSettings>()))
{
InputSystem.settings = other.value;

Assert.That(InputSystem.settings.IsFeatureEnabled(featureName), Is.False);
}
}
}

[Test]
[Category("Actions")]
public void Actions_WhenShortcutsDisabled_AllConflictingActionsTrigger()
1 change: 1 addition & 0 deletions Packages/com.unity.inputsystem/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -28,6 +28,7 @@ however, it has to be formatted properly to pass verification tests.
- 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 a performance issue with many objects using multiple action maps [ISXB-573](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-573).
- Fixed an variable scope shadowing issue causing compilation to fail on Unity 2019 LTS.
- 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
Original file line number Diff line number Diff line change
@@ -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.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.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.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.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.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.paranoidReadValueCachingChecksEnabled)
{
var currentUnprocessedValue = ReadUnprocessedValueFromState(currentStatePtr);
if (CompareValue(ref currentUnprocessedValue, ref m_UnprocessedCachedValue))
37 changes: 35 additions & 2 deletions Packages/com.unity.inputsystem/InputSystem/InputManager.cs
Original file line number Diff line number Diff line change
@@ -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;
@@ -2116,6 +2117,33 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!


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;
#endif
@@ -2644,6 +2672,11 @@ internal void ApplySettings()
#if UNITY_EDITOR
runPlayerUpdatesInEditMode = m_Settings.IsFeatureEnabled(InputFeatureNames.kRunPlayerUpdatesInEditMode);
#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));
m_ParanoidReadValueCachingChecksEnabled = m_Settings.IsFeatureEnabled((InputFeatureNames.kParanoidReadValueCachingChecks));
}

// Cache some values.
@@ -3542,7 +3575,7 @@ private void ResetCurrentProcessedEventBytesForDevices()
[Conditional("UNITY_EDITOR")]
void CheckAllDevicesOptimizedControlsHaveValidState()
{
if (!InputSettings.optimizedControlsFeatureEnabled)
if (!InputSystem.s_Manager.m_OptimizedControlsFeatureEnabled)
return;

foreach (var device in devices)
@@ -3732,7 +3765,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
33 changes: 7 additions & 26 deletions Packages/com.unity.inputsystem/InputSystem/InputSettings.cs
Original file line number Diff line number Diff line change
@@ -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<string>();

if (enabled)
m_FeatureFlags.Add(featureName.ToUpperInvariant());
else
m_FeatureFlags.Remove(featureName.ToUpperInvariant());
break;
}
if (m_FeatureFlags == null)
m_FeatureFlags = new HashSet<string>();

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)