Skip to content
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

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

Merged
merged 10 commits into from
Jun 26, 2024
28 changes: 28 additions & 0 deletions Assets/Tests/InputSystem/CoreTests_Actions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
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 @@ -27,6 +27,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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
)
Expand All @@ -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))
Expand Down
42 changes: 34 additions & 8 deletions Packages/com.unity.inputsystem/InputSystem/InputManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -2645,12 +2673,10 @@ internal void ApplySettings()
runPlayerUpdatesInEditMode = m_Settings.IsFeatureEnabled(InputFeatureNames.kRunPlayerUpdatesInEditMode);
#endif

if (m_Settings.IsFeatureEnabled(InputFeatureNames.kUseWindowsGamingInputBackend))
{
var command = UseWindowsGamingInputCommand.Create(true);
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.
Expand Down Expand Up @@ -3549,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)
Expand Down Expand Up @@ -3739,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
Expand Down
33 changes: 7 additions & 26 deletions Packages/com.unity.inputsystem/InputSystem/InputSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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)
Expand Down