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
29 changes: 29 additions & 0 deletions Assets/Tests/InputSystem/CoreTests_Actions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,35 @@
// 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)]
#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 @@ -26,6 +26,7 @@ however, it has to be formatted properly to pass verification tests.
- 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 a performance issue with many objects using multiple action maps [ISXB-573](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-573).
- 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.m_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.m_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.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
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.m_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.m_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.m_ParanoidReadValueCachingChecksEnabled)
{
var currentUnprocessedValue = ReadUnprocessedValueFromState(currentStatePtr);
if (CompareValue(ref currentUnprocessedValue, ref m_UnprocessedCachedValue))
Expand Down
17 changes: 15 additions & 2 deletions Packages/com.unity.inputsystem/InputSystem/InputManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

internal bool m_OptimizedControlsFeatureEnabled;
internal bool m_ReadValueCachingFeatureEnabled;
internal bool m_ParanoidReadValueCachingChecksEnabled;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please change these to private fields and add an internal read-only accessor property?

When refactoring for CoreCLR support, internal fields were very frustrating because I had to check the entire code base for any usages.

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 have been adressed now in c93e565

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regarding FWPM refactor I think this is straightforward since ApplySettings seem intact.


#if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS
private InputActionAsset m_Actions;
#endif
Expand Down Expand Up @@ -2645,12 +2651,19 @@ 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));
m_ParanoidReadValueCachingChecksEnabled = m_Settings.IsFeatureEnabled((InputFeatureNames.kParanoidReadValueCachingChecks));
}

// Cache some values.
Expand Down Expand Up @@ -3549,7 +3562,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 +3752,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