Skip to content

FIX: ISXB-1674 - Input actions asset not converted correctly when upgrading from 1.14.1 (Regression) #2244

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.

Sign up for GitHub

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

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

FIX: ISXB-1674 - Input actions asset not converted correctly when upgrading from 1.14.1 (Regression) #2244

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a260324
Update the parser for migration
AswinRajGopal Sep 25, 2025
00b37a5
Added a test to cover the package migration.
AswinRajGopal Oct 2, 2025
d637adf
Revert "Added a test to cover the package migration."
AswinRajGopal Oct 3, 2025
4e76813
Revert "Update the parser for migration"
AswinRajGopal Oct 3, 2025
a8ba533
The the processors are split by , instead of ; it was typo
AswinRajGopal Oct 3, 2025
ff05b1d
Robust handling of migration logic
AswinRajGopal Oct 6, 2025
829c4aa
Added test to cover the migration
AswinRajGopal Oct 6, 2025
9467adf
Update test
AswinRajGopal Oct 7, 2025
3e490f1
Address PR feeback.
AswinRajGopal Oct 7, 2025
a31d971
Address PR comments.
AswinRajGopal Oct 7, 2025
817ff0e
Update the test to include more than one processors in actionmap.
AswinRajGopal Oct 7, 2025
2914bf7
Removed redundant call
AswinRajGopal Oct 7, 2025
6daabc9
Revert changes and moved the function changes to NameAndParameters as…
AswinRajGopal Oct 8, 2025
6aca9db
Bring back the test which verifies the migration along with processed…
AswinRajGopal Oct 10, 2025
f3f7ea5
Update the test as per review comments.
AswinRajGopal Oct 16, 2025
f5b06da
Simply the code removing redundancy - as these checks are handled in …
AswinRajGopal Oct 16, 2025
6b4c75a
Add check incase if raw string is null.
AswinRajGopal Oct 16, 2025
64076cb
Reformatting files.
AswinRajGopal Oct 16, 2025
f042f1e
Merge branch 'develop' into isxb-1678/fix-customprocessor-serialise-r…
ekcoh Oct 21, 2025
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
65 changes: 65 additions & 0 deletions Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using UnityEngine;
using UnityEngine.InputSystem;
using UnityEngine.InputSystem.Editor;
using UnityEngine.InputSystem.Processors;
using UnityEngine.TestTools;
using UnityEngine.UIElements;

Expand Down Expand Up @@ -108,5 +109,69 @@ public IEnumerator ProcessorEnum_ShouldSerializeByValue_WhenSerializedToAsset()

yield return null;
}

[Test]
[Description("Regression test for case ISXB-1674")]
public void Migration_ShouldProduceValidActionAsset_WithEnumProcessorConverted()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you document what the intent of this test case is inline in code?

My interpretation is that you want to verify that processors defined in a certain way in .inputaction json are parsed/preserved correctly when imported (with migration)? If this is the case I suggest you convert this into a functional test instead of doing a lot of string comparisons. Here is a rough example (coded directly into web interface so it may contain errors but hope you get the idea):

const string legacyJson = "<as before>"; // Consider including binding in JSON to avoid writing code for it
var asset = InputActionAsset.FromJson(legacyJson);
var gamepad = InputSystem.AddDevice<Gamepad>();
var action = asset.FindAction("Player/Move");
action.Enable();

// Make sure deadzone processor is applied
Set(gamepad.leftStick, new Vector2(InputSystem.settings.defaultDeadzoneMin * 0.9f, 0.0f));
Assert.That(action.ReadValue<Vector2>(), Is.Equal(Vecto2.zero)); // Filtered by deadzone

// Make sure invert processor is applied
Set(gamepad.leftStick, new Vector2(1.0f, 0.0f));
Assert.That(action.ReadValue<Vector2>(), Is.Equal(-Vector2.right)); // Inverted

// Make sure custom processor is applied
Set(gamepad.leftStick, new Vector2(0.5f, 0.0f));
Assert.That(action.ReadValue<Vector2>(), Is.Equal(something)); // Not sure how custom should change value, so change this to what is appropriate

Ideally order dependency can be proven through the same test case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure I'll change it into a functional test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you see any pros/cons with changing approach? Would such a functional test miss some aspect of this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My view about authoring a regression test is to cover only the migration case where we actually verify that a JSON with a custom processor and some neighboring processors like stickdeadzone or invertVector are parsed properly after migration.

The functional tests already exists which covers most of the functionals in the input system generic tests. However it will be robust to club both of these for this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, that is also fine so keep this if you want. Not sure @LeoUnity has additional perspective?
Not sure we already test this specific processor order somewhere else?

{
const string k_Json = @"
{
""name"": ""InputSystem_Actions"",
""maps"": [
{
""name"": ""Player"",
""id"": ""df70fa95-8a34-4494-b137-73ab6b9c7d37"",
""actions"": [
{
""name"": ""Move"",
""type"": ""Value"",
""id"": ""938a78a0-f8c6-4b2e-b8a7-d3c26d83a4e9"",
""expectedControlType"": ""Vector2"",
""processors"": ""StickDeadzone,InvertVector2(invertX=false),Custom(SomeEnum=1)"",
""interactions"": """",
""initialStateCheck"": true
}
],
""bindings"": [
{
""name"": """",
""id"": ""c9a175a0-a5ed-4e2c-b3a9-1d4d3d3a7a9a"",
""path"": ""<Gamepad>/leftStick"",
""interactions"": """",
""processors"": """",
""groups"": """",
""action"": ""Move"",
""isComposite"": false,
""isPartOfComposite"": false
}
]
}
],
""controlSchemes"": []
}";

var asset = InputActionAsset.FromJson(k_Json);

// Enable the action to call rebinding
asset.FindAction("Move").Enable();

var map = asset.FindActionMap("Player");

// Directly tests the outcome of the migration and parsing logic, which is the core goal.
var instantiatedProcessors = map.m_State.processors;
Assert.That(map.m_State.totalProcessorCount, Is.EqualTo(3), "Should create exactly three processors.");

// Verify the order and type of each processor.
Assert.That(instantiatedProcessors[0], Is.TypeOf<StickDeadzoneProcessor>(), "First processor should be StickDeadzone.");
Assert.That(instantiatedProcessors[1], Is.TypeOf<InvertVector2Processor>(), "Second processor should be InvertVector2.");
Assert.That(instantiatedProcessors[2], Is.TypeOf<CustomProcessor>(), "Third processor should be the custom type.");

// Verify the specific data for processors with parameters.
var invertProcessor = (InvertVector2Processor)instantiatedProcessors[1];
Assert.That(invertProcessor.invertX, Is.False, "invertX parameter should be false.");

var customProcessor = (CustomProcessor)instantiatedProcessors[2];
Assert.That(customProcessor.SomeEnum, Is.EqualTo(SomeEnum.OptionB), "Enum parameter should be parsed correctly to OptionB.");
}
}
#endif
60 changes: 35 additions & 25 deletions Packages/com.unity.inputsystem/InputSystem/Actions/InputActionAsset.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,11 @@
return;
if ((parsedJson.maps?.Length ?? 0) > 0 && (parsedJson.version) < JsonVersion.Version1)
{
List<NameAndParameters> parsedList = null;
var converted = new List<NameAndParameters>(8);
var updatedParameters = new List<NamedValue>(4);
var enumValuesCache = new Dictionary<Type, Array>(8);

for (var mi = 0; mi < parsedJson.maps.Length; ++mi)
{
var mapJson = parsedJson.maps[mi];
Expand All @@ -1026,44 +1031,49 @@
if (string.IsNullOrEmpty(raw))
continue;

var list = NameAndParameters.ParseMultiple(raw).ToList();
var rebuilt = new List<string>(list.Count);
foreach (var nap in list)
if (!NameAndParameters.ParseMultiple(raw, ref parsedList))
continue;

Check warning on line 1035 in Packages/com.unity.inputsystem/InputSystem/Actions/InputActionAsset.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionAsset.cs#L1035 

Added line #L1035 was not covered by tests

converted.Clear();

for (int i = 0; i < parsedList.Count; ++i)
{
var nap = parsedList[i];
var procType = InputSystem.TryGetProcessor(nap.name);
if (nap.parameters.Count == 0 || procType == null)
{
rebuilt.Add(nap.ToString());
converted.Add(nap);
continue;
}

var dict = nap.parameters.ToDictionary(p => p.name, p => p.value.ToString());
var anyChanged = false;
foreach (var field in procType.GetFields(BindingFlags.Public | BindingFlags.Instance).Where(f => f.FieldType.IsEnum))
updatedParameters.Clear();
for (int k = 0; k < nap.parameters.Count; ++k)
{
if (dict.TryGetValue(field.Name, out var ordS) && int.TryParse(ordS, out var ord))
var param = nap.parameters[k];
var updatedPar = param;

var fieldInfo = procType.GetField(param.name, BindingFlags.Public | BindingFlags.Instance);
if (fieldInfo != null && fieldInfo.FieldType.IsEnum)
{
var values = Enum.GetValues(field.FieldType).Cast<object>().ToArray();
if (ord >= 0 && ord < values.Length)
var index = param.value.ToInt32();
if (index >= 0)
{
dict[field.Name] = Convert.ToInt32(values[ord]).ToString();
anyChanged = true;
if (!enumValuesCache.TryGetValue(fieldInfo.FieldType, out var values))
{
values = Enum.GetValues(fieldInfo.FieldType);
enumValuesCache[fieldInfo.FieldType] = values;
}
if (index < values.Length)
{
var convertedValue = Convert.ToInt32(values.GetValue(index));
updatedPar = NamedValue.From(param.name, convertedValue);
}
}
}
updatedParameters.Add(updatedPar);
}

if (!anyChanged)
{
rebuilt.Add(nap.ToString());
}
else
{
var paramText = string.Join(",", dict.Select(kv => $"{kv.Key}={kv.Value}"));
rebuilt.Add($"{nap.name}({paramText})");
}
converted.Add(NameAndParameters.Create(nap.name, updatedParameters));
}

actionJson.processors = string.Join(";", rebuilt);
actionJson.processors = NameAndParameters.ToSerializableString(converted);
mapJson.actions[ai] = actionJson;
}
parsedJson.maps[mi] = mapJson;
Expand Down
17 changes: 4 additions & 13 deletions ...m.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/NameAndParametersListView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
var newElement = new NameAndParameters() { name = name};
interactionsOrProcessorsList.Add(newElement);

m_ListProperty.stringValue = ToSerializableString(interactionsOrProcessorsList);
m_ListProperty.stringValue = NameAndParameters.ToSerializableString(interactionsOrProcessorsList);

Check warning on line 40 in Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/NameAndParametersListView.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/NameAndParametersListView.cs#L40 

Added line #L40 was not covered by tests
m_ListProperty.serializedObject.ApplyModifiedProperties();
}

Expand All @@ -61,35 +61,26 @@
if (interactionsOrProcessors.Length == 0 || !newIndexIsValid || !oldIndexIsValid)
return;
MemoryHelpers.Swap(ref interactionsOrProcessors[oldIndex], ref interactionsOrProcessors[newIndex]);
m_ListProperty.stringValue = ToSerializableString(interactionsOrProcessors);
m_ListProperty.stringValue = NameAndParameters.ToSerializableString(interactionsOrProcessors);

Check warning on line 64 in Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/NameAndParametersListView.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/NameAndParametersListView.cs#L64 

Added line #L64 was not covered by tests
m_ListProperty.serializedObject.ApplyModifiedProperties();
}

private void DeleteElement(int index)
{
var interactionsOrProcessorsList = NameAndParameters.ParseMultiple(m_ListProperty.stringValue).ToList();
interactionsOrProcessorsList.RemoveAt(index);
m_ListProperty.stringValue = ToSerializableString(interactionsOrProcessorsList);
m_ListProperty.stringValue = NameAndParameters.ToSerializableString(interactionsOrProcessorsList);

Check warning on line 72 in Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/NameAndParametersListView.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/NameAndParametersListView.cs#L72 

Added line #L72 was not covered by tests
m_ListProperty.serializedObject.ApplyModifiedProperties();
}

private void OnParametersChanged(ParameterListView listView, int index)
{
var interactionsOrProcessorsList = NameAndParameters.ParseMultiple(m_ListProperty.stringValue).ToList();
interactionsOrProcessorsList[index] = new NameAndParameters { name = interactionsOrProcessorsList[index].name, parameters = listView.GetParameters() };
m_ListProperty.stringValue = ToSerializableString(interactionsOrProcessorsList);
m_ListProperty.stringValue = NameAndParameters.ToSerializableString(interactionsOrProcessorsList);
m_ListProperty.serializedObject.ApplyModifiedProperties();
}

private static string ToSerializableString(IEnumerable<NameAndParameters> parametersForEachListItem)
{
if (parametersForEachListItem == null)
return string.Empty;

return string.Join(NamedValue.Separator,
parametersForEachListItem.Select(x => x.ToString()).ToArray());
}

public override void RedrawUI(InputActionsEditorState state)
{
if (m_ContentContainer != null)
Expand Down
17 changes: 17 additions & 0 deletions Packages/com.unity.inputsystem/InputSystem/Utilities/NameAndParameters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,23 @@
return $"{name}({parameterString})";
}

internal static string ToSerializableString(IEnumerable<NameAndParameters> list)
{
if(list == null)
return string.Empty;

Check warning on line 33 in Packages/com.unity.inputsystem/InputSystem/Utilities/NameAndParameters.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Packages/com.unity.inputsystem/InputSystem/Utilities/NameAndParameters.cs#L33 

Added line #L33 was not covered by tests

return string.Join(NamedValue.Separator, list.Select(x => x.ToString()).ToArray());
}

internal static NameAndParameters Create(string name, IList<NamedValue> parameters)
{
return new NameAndParameters
{
name = name,
parameters = new ReadOnlyArray<NamedValue>(parameters.ToArray())
};
}

public static IEnumerable<NameAndParameters> ParseMultiple(string text)
{
List<NameAndParameters> list = null;
Expand Down