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 2 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
26 changes: 26 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,31 @@

yield return null;
}

[UnityTest]
public IEnumerator Migration_FromLegacyJson_ShouldConvertOrdinal_KeepInvertVector2_AndSeparators()
{
var legacyJson = m_Asset.ToJson().Replace("\"version\": 1", "\"version\": 0").Replace("Custom(SomeEnum=10)", "Custom(SomeEnum=1)");

// Add a trailing processor to verify the semicolon separator is preserved.
if (!legacyJson.Contains(";InvertVector2(invertX=true)"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The fact we have an conditional treatment on our test data before we act on it is not a good sign, ideally the data we use for testing is known ahead of time and we can just apply the function under test on it.

Whenever we need to bump the current version to 2 the replace above will fail, which is not what the test is trying to test.

I am wondering if a better approach here would be to just have a static json string here instead.

legacyJson = legacyJson.Replace("Custom(SomeEnum=1)\"", "Custom(SomeEnum=1);InvertVector2(invertX=true)\"");

var migratedAsset = InputActionAsset.FromJson(legacyJson);
Assume.That(migratedAsset, Is.Not.Null, "Failed to load legacy JSON into an InputActionAsset.");

var migratedJson = migratedAsset.ToJson();
Assume.That(migratedJson, Is.Not.Null.And.Not.Empty, "Migrated JSON was empty.");

Assert.Less(migratedJson.IndexOf("InvertVector2(invertX=true)", StringComparison.Ordinal), migratedJson.IndexOf(",Custom(SomeEnum=20)", StringComparison.Ordinal),
"Expected a comma between the first and second processors, with InvertVector2 first."
);

Assert.Greater(migratedJson.IndexOf(";InvertVector2(invertX=true)", StringComparison.Ordinal), migratedJson.IndexOf("Custom(SomeEnum=20)", StringComparison.Ordinal),

Check warning on line 131 in Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs#L131 

Added line #L131 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm also wondering if asserting on a substring is the most robust way here... The substring might be there but other things could make the json parsing fail... What we want to make sure is that after the migration the json can be parsed without issues and that the enum values are correct.

We even have the migratedAsset object, so let's just assert that the processors are set correctly.

"Expected a semicolon between the second and third processors, with the trailing InvertVector2 last."
);

yield return null;
}

Check warning on line 136 in Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs

View check run for this annotation

Codecov GitHub.com / codecov/patch

Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs#L135-L136 

Added lines #L135 - L136 were not covered by tests
}
#endif
129 changes: 97 additions & 32 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 @@ -3,7 +3,7 @@
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using UnityEngine.InputSystem.Editor;
using System.Globalization;
using UnityEngine.InputSystem.Utilities;

////TODO: make the FindAction logic available on any IEnumerable<InputAction> and IInputActionCollection via extension methods
Expand Down Expand Up @@ -1014,60 +1014,125 @@
{
if (parsedJson.version >= JsonVersion.Version1)
return;
if ((parsedJson.maps?.Length ?? 0) > 0 && (parsedJson.version) < JsonVersion.Version1)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to do this parsing manually ourselves here? Is there anything special that we need in this parsing that is no needed elsewhere?
I'm assuming this data is used in other parts of the system, and there would be logic we can reuse... now if there is an issue with the parsing of jsonmaps we have to fix in 2 places...

if (parsedJson.maps == null || parsedJson.maps.Length == 0)
{
parsedJson.version = JsonVersion.Version1;
return;
}

for (var mi = 0; mi < parsedJson.maps.Length; ++mi)
{
for (var mi = 0; mi < parsedJson.maps.Length; ++mi)
var mapJson = parsedJson.maps[mi];
if (mapJson.actions == null || mapJson.actions.Length == 0)
continue;

Check warning on line 1028 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#L1028 

Added line #L1028 was not covered by tests

for (var ai = 0; ai < mapJson.actions.Length; ++ai)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to add some inline comments to the different parts of this large for loop or split it up so that is clear what the intent is? It is quite difficult to review given that there is no motivation given for what we do here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like 'ai' isn't really used so why not e.g foreach instead?

{
var mapJson = parsedJson.maps[mi];
for (var ai = 0; ai < mapJson.actions.Length; ++ai)
var actionJson = mapJson.actions[ai];
var raw = actionJson.processors;
if (string.IsNullOrEmpty(raw))
continue;

var parts = System.Text.RegularExpressions.Regex.Split(raw, @"\s*([,;])\s*");
if (parts.Length == 0)
continue;

Check warning on line 1039 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#L1039 

Added line #L1039 was not covered by tests

var tokens = new List<string>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is inside a loop I would recommend moving tokens outside the loop and just do clear here to avoid GC pressure.

for (int i = 0; i < parts.Length; i += 2)
if (!string.IsNullOrEmpty(parts[i]))
tokens.Add(parts[i]);

if (tokens.Count == 0)
continue;

Check warning on line 1047 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#L1047 

Added line #L1047 was not covered by tests

var parsed = new List<NameAndParameters>(tokens.Count);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also this list may be moved to outer scope to avoid allocating on every iteration.

foreach (var t in tokens)
parsed.Add(NameAndParameters.Parse(t));

var rebuiltTokens = new List<string>(tokens.Count);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same with this list

var anyProcessorChanged = false;

for (int pi = 0; pi < parsed.Count; pi++)
{
var actionJson = mapJson.actions[ai];
var raw = actionJson.processors;
if (string.IsNullOrEmpty(raw))
var nap = parsed[pi];

var procType = InputSystem.TryGetProcessor(nap.name);
if (procType == null || nap.parameters.Count == 0)
{
rebuiltTokens.Add(tokens[pi]);

Check warning on line 1063 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#L1062-L1063 

Added lines #L1062 - L1063 were not covered by tests
continue;
}

var dict = new Dictionary<string, string>(nap.parameters.Count, System.StringComparer.OrdinalIgnoreCase);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This dict could also be outer scope?

foreach (var p in nap.parameters)
dict[p.name] = p.value.ToString();

var changedThisProcessor = false;

var list = NameAndParameters.ParseMultiple(raw).ToList();
var rebuilt = new List<string>(list.Count);
foreach (var nap in list)
foreach (var field in procType.GetFields(BindingFlags.Public | BindingFlags.Instance))
{
var procType = InputSystem.TryGetProcessor(nap.name);
if (nap.parameters.Count == 0 || procType == null)
{
rebuilt.Add(nap.ToString());
if (!field.FieldType.IsEnum)
continue;

if (!dict.TryGetValue(field.Name, out var rawVal))
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))
if (int.TryParse(rawVal, NumberStyles.Integer, CultureInfo.InvariantCulture, out var n))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand what the goal is here, could you add inline comments explaining?

{
if (dict.TryGetValue(field.Name, out var ordS) && int.TryParse(ordS, out var ord))
var values = System.Enum.GetValues(field.FieldType);
var looksLikeOrdinal = n >= 0 && n < values.Length && !System.Enum.IsDefined(field.FieldType, n);
if (looksLikeOrdinal)
{
var values = Enum.GetValues(field.FieldType).Cast<object>().ToArray();
if (ord >= 0 && ord < values.Length)
var underlying = Convert.ToInt32(values.GetValue(n));
if (underlying != n)
{
dict[field.Name] = Convert.ToInt32(values[ord]).ToString();
anyChanged = true;
dict[field.Name] = underlying.ToString(CultureInfo.InvariantCulture);
changedThisProcessor = true;
}
}
}
}

if (!changedThisProcessor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also code from here to end is fuzzy to me, it would be helpful with inline comments explaining what we are trying to achieve in order to review.

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 add comments in detail.

{
rebuiltTokens.Add(tokens[pi]);
}
else
{
var ordered = nap.parameters.Select(p =>
{
var v = dict.TryGetValue(p.name, out var nv) ? nv : p.value.ToString();
return $"{p.name}={v}";
});

var migrated = $"{nap.name}({string.Join(",", ordered)})";
rebuiltTokens.Add(migrated);
anyProcessorChanged = true;
}
}

if (!anyChanged)
if (anyProcessorChanged)
{
var sb = new System.Text.StringBuilder(raw.Length + 16);
int tokenIndex = 0;
for (int partIndex = 0; partIndex < parts.Length; ++partIndex)
{
if ((partIndex % 2) == 0)
{
rebuilt.Add(nap.ToString());
if (tokenIndex < rebuiltTokens.Count)
sb.Append(rebuiltTokens[tokenIndex++]);
}
else
{
var paramText = string.Join(",", dict.Select(kv => $"{kv.Key}={kv.Value}"));
rebuilt.Add($"{nap.name}({paramText})");
sb.Append(parts[partIndex]);
}
}

actionJson.processors = string.Join(";", rebuilt);
mapJson.actions[ai] = actionJson;
actionJson.processors = sb.ToString();
}
parsedJson.maps[mi] = mapJson;
mapJson.actions[ai] = actionJson;
}
parsedJson.maps[mi] = mapJson;
}
// Bump the version so we never re-migrate
parsedJson.version = JsonVersion.Version1;
Expand Down