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 7 commits into
base: develop
Choose a base branch
from
+87 −26
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 all commits
Commits
Show all changes
7 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
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
53 changes: 53 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 @@ -108,5 +108,58 @@ public IEnumerator ProcessorEnum_ShouldSerializeByValue_WhenSerializedToAsset()

yield return null;
}

[Test]
public void Migration_ShouldProduceValidActionAsset_WithEnumProcessorConverted()
{
var legacyJson = @"
{
""name"": ""InputSystem_Actions"",
""maps"": [
{
""name"": ""Player"",
""id"": ""df70fa95-8a34-4494-b137-73ab6b9c7d37"",
""actions"": [
{
""name"": ""Move"",
""type"": ""Value"",
""id"": ""351f2ccd-1f9f-44bf-9bec-d62ac5c5f408"",
""expectedControlType"": ""Vector2"",
""processors"": ""StickDeadzone,InvertVector2(invertX=false),Custom(SomeEnum=1)"",
""interactions"": """",
""initialStateCheck"": true
}
]
}
],
""controlSchemes"": [],
""version"": 0
}";

// Parse and migrate the legacy JSON
var asset = InputActionAsset.FromJson(legacyJson);

// Object is valid after migration
Assert.That(asset, Is.Not.Null, "Migration failed to produce a valid InputActionAsset.");

var map = asset.FindActionMap("Player");
Assert.That(map, Is.Not.Null, "Expected Player map to exist.");

var action = map.FindAction("Move");
Assert.That(action, Is.Not.Null, "Expected Move action to exist.");

var processors = action.processors;

// Verify processor order and that enum was converted properly
Assert.That(processors, Does.Contain("StickDeadzone"), "StickDeadzone processor missing.");
Assert.That(processors, Does.Contain("InvertVector2(invertX=false)"), "InvertVector2 missing.");
Assert.That(processors, Does.Contain("Custom(SomeEnum=20)"), "Custom(SomeEnum=1) should migrate to SomeEnum=20 (OptionB).");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is not verifying that the processor string can be parsed properly, which is what we want... we could still have a ";" separating them and those asserts would most likely pass.

@ekcoh do you know how we could get access to the parsed list of processors so we could assert on that instead? I could only find InputControl.processors or InputActionState.processors, but I don't know how to go from an InputActionAsset to InputControl or InputActionState

It seems InputAction.ResolveBindings end up calling InputBindingResolver.AddActionMap which then calls InstantiateWithParameters and that leads to the parsing and creation of processors, but I am not sure how we could call that from a test and then retrieve the list of processors to assert on that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch, since the API design of Input Actions is based on string you can put whatever into Processors. So this test needs to complemented with code resolving the actions and the only way to actually prove the processors is correctly parsed and applied I believe would be to enable the actions and do ReadValue on the action after being resolved. To drive that action you also likely need a fake device to drive it with e.g. 1.0 and then see that e.g. scale processor multiples by 10 or similar.

In CoreTests_Actions.cs there are various examples on how to achieve this, e.g. search for tests containing ReadValue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would have been trivial if the string based API design was not a thing and instead actions had a list of actual processors on them.

Copy link
Collaborator Author

@AswinRajGopal AswinRajGopal Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @LeoUnity @ekcoh, our NameAndParameters.ParseMultiple() explicitly checks for both seperators , and ; between processors entries. Either way we get the same result however I agree with semantic issues from string only assertions.
Should we go for
var parsed = NameAndParameters.ParseMultiple(action.processors).ToList(); Assert.That(parsed.Count, Is.EqualTo(expectedCount)); ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it just work to do this? Its simple and very little code

var legacyJson = ....
var modernJson = ....

var ported = InputActionAsset.FromJson(legacyJson).ToJson();
Assert.That(modernJson, Is.EqualTo(ported));

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally you would have compared InputActionAsset objects but unfortunately it's not supported :/ Hence full cycle allows you to avoid the details. If porting is successful I would expect them both to be equal. Or even

var legacyJson = ....
var modernJson = ....

var ported = InputActionAsset.FromJson(legacyJson).ToJson();
Assert.That(InputActionAsset.FromJson(modernJson).ToJson(), Is.EqualTo(ported));

Since that wouldn't break either if formatting is updated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it should work, earlier the cause of regression was not considering ; separator in the migration logic it only splits , and keeps the ; unchanged caused the serialization to break. Since now as @LeoUnity suggested the robust approach to use the NameAndParameters the above FromJson and ported json assertion should do good I believe.


// Verify To JSON
var toJson = asset.ToJson();
var reloaded = InputActionAsset.FromJson(toJson);
Assert.That(reloaded, Is.Not.Null, "Reloaded asset after migration is null.");
Assert.That(reloaded.FindAction("Player/Move"), Is.Not.Null, "Reloaded asset did not contain expected Move action.");
}
}
#endif
36 changes: 15 additions & 21 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 @@ -1027,43 +1027,37 @@ internal void MigrateJson(ref ReadFileJson parsedJson)
continue;

var list = NameAndParameters.ParseMultiple(raw).ToList();
var rebuilt = new List<string>(list.Count);
var converted = new List<NameAndParameters>(list.Count);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still do not see why this isn't moved to outer scope and reused over iterations instead of being allocated on every iteration? Am I missing something?

foreach (var nap in list)
{
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))
var updatedParameters = new List<NamedValue>(nap.parameters.Count);
foreach (var param in nap.parameters)
{
if (dict.TryGetValue(field.Name, out var ordS) && int.TryParse(ordS, out var ord))
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();
var values = Enum.GetValues(fieldInfo.FieldType);
if(index >= 0 && index < values.Length)
{
dict[field.Name] = Convert.ToInt32(values[ord]).ToString();
anyChanged = true;
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.SerializeMultiple(converted);
mapJson.actions[ai] = actionJson;
}
parsedJson.maps[mi] = mapJson;
Expand Down
6 changes: 1 addition & 5 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 @@ -83,11 +83,7 @@ private void OnParametersChanged(ParameterListView listView, int index)

private static string ToSerializableString(IEnumerable<NameAndParameters> parametersForEachListItem)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this function existing if it only forwards to NameAndParameters.SerializeMultiple? Seems redundant

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe keep the old name instead?

{
if (parametersForEachListItem == null)
return string.Empty;

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

public override void RedrawUI(InputActionsEditorState state)
Expand Down
18 changes: 18 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,24 @@
return $"{name}({parameterString})";
}

internal static string SerializeMultiple(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)
{
var result = new NameAndParameters
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: Why create a result variable here instead of just

return new NameAndParameters { .... };

{
name = name,
parameters = new ReadOnlyArray<NamedValue>(parameters.ToArray())
};
return result;
}

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