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

AswinRajGopal · 2025-09-25T11:03:56Z

Description

Bug: https://jira.unity3d.com/browse/ISXB-1674
Port: 1.14.X

Preserve original tokens and only touch the enum values. That means processor names, order, legacy processors, and formatting survive unchanged so the editor no longer collapses or marks them “Obsolete”.
Avoid whole string rewrites, which previously caused mismatches between what the editor expected and what was written back and only assign a new processor string if something actually changed.

Testing status & QA

Verified manually with the attached repro project.

Overall Product Risks

Complexity: 0
Halo Effect: 0

Comments to reviewers

Checklist

Before review:

Changelog entry added.
- Explains the change in Changed, Fixed, Added sections.
- For API change contains an example snippet and/or migration example.
- JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
- Jira port for the next release set as "Resolved".
Tests added/changed, if applicable.
- Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
- Performance tests.
- Integration tests.
Docs for new/changed API's.
- Xmldoc cross references are set correctly.
- Added explanation how the API works.
- Usage code examples added.
- The manual is updated, if needed.

During merge:

Commit message for squash-merge is prefixed with one of the list:
- NEW: ___.
- FIX: ___.
- DOCS: ___.
- CHANGE: ___.
- RELEASE: 1.1.0-preview.3.

After merge:

Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

codecov-github-com · 2025-09-25T11:30:29Z

Codecov Report

Attention: Patch coverage is 41.37931% with 34 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...nputsystem/InputSystem/Actions/InputActionAsset.cs	12.90%	27 Missing ⚠️
...tsystem/InputSystem/Utilities/NameAndParameters.cs	50.00%	4 Missing ⚠️
...UITKAssetEditor/Views/NameAndParametersListView.cs	25.00%	3 Missing ⚠️

@@             Coverage Diff             @@
##           develop    #2244      +/-   ##
===========================================
+ Coverage    68.14%   76.76%   +8.61%     
===========================================
  Files          367      465      +98     
  Lines        53685    87944   +34259     
===========================================
+ Hits         36584    67508   +30924     
- Misses       17101    20436    +3335

Flag	Coverage Δ
inputsystem_MacOS_2021.3	`5.91% <0.00%> (?)`
inputsystem_MacOS_2021.3_project	`78.04% <10.25%> (?)`
inputsystem_MacOS_2022.3	`5.37% <0.00%> (?)`
inputsystem_MacOS_2022.3_project	`74.58% <9.30%> (?)`
inputsystem_MacOS_6000.0	`5.18% <0.00%> (?)`
inputsystem_MacOS_6000.0_project	`76.56% <41.37%> (?)`
inputsystem_MacOS_6000.2	`5.18% <0.00%> (?)`
inputsystem_MacOS_6000.2_project	`76.55% <41.37%> (?)`
inputsystem_MacOS_6000.3	`5.18% <0.00%> (?)`
inputsystem_MacOS_6000.3_project	`76.55% <41.37%> (?)`
inputsystem_MacOS_6000.4	`5.18% <0.00%> (?)`
inputsystem_MacOS_6000.4_project	`76.55% <41.37%> (?)`
inputsystem_Ubuntu_2021.3	`5.91% <0.00%> (?)`
inputsystem_Ubuntu_2021.3_project	`77.95% <10.25%> (?)`
inputsystem_Ubuntu_2022.3	`5.37% <0.00%> (?)`
inputsystem_Ubuntu_2022.3_project	`74.38% <9.30%> (?)`
inputsystem_Ubuntu_6000.0	`5.19% <0.00%> (?)`
inputsystem_Ubuntu_6000.0_project	`76.37% <41.37%> (?)`
inputsystem_Ubuntu_6000.2	`5.19% <0.00%> (?)`
inputsystem_Ubuntu_6000.2_project	`76.37% <41.37%> (?)`
inputsystem_Ubuntu_6000.3	`5.19% <0.00%> (?)`
inputsystem_Ubuntu_6000.3_project	`76.38% <41.37%> (?)`
inputsystem_Ubuntu_6000.4	`5.19% <0.00%> (?)`
inputsystem_Ubuntu_6000.4_project	`76.37% <41.37%> (?)`
inputsystem_Windows_2021.3	`5.91% <0.00%> (?)`
inputsystem_Windows_2021.3_project	`78.18% <10.25%> (?)`
inputsystem_Windows_2022.3	`5.37% <0.00%> (?)`
inputsystem_Windows_2022.3_project	`74.72% <9.30%> (?)`
inputsystem_Windows_6000.0	`5.18% <0.00%> (?)`
inputsystem_Windows_6000.0_project	`76.70% <41.37%> (?)`
inputsystem_Windows_6000.2	`5.18% <0.00%> (?)`
inputsystem_Windows_6000.2_project	`76.69% <41.37%> (?)`
inputsystem_Windows_6000.3	`5.18% <0.00%> (?)`
inputsystem_Windows_6000.3_project	`76.69% <41.37%> (?)`
inputsystem_Windows_6000.4	`5.18% <0.00%> (?)`
inputsystem_Windows_6000.4_project	`0.00% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ests/InputSystem.Editor/CustomProcessorEnumTest.cs	`84.21% <100.00%> (ø)`
...UITKAssetEditor/Views/NameAndParametersListView.cs	`65.38% <25.00%> (-0.98%)`	⬇️
...tsystem/InputSystem/Utilities/NameAndParameters.cs	`90.12% <50.00%> (-3.03%)`	⬇️
...nputsystem/InputSystem/Actions/InputActionAsset.cs	`73.07% <12.90%> (-1.01%)`	⬇️

... and 105 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ekcoh · 2025-10-02T08:34:38Z

I noticed this PR title was incorrect so I changed it to remove that failure.

ekcoh

Thanks for looking into this, my main comment at this review round is to add inline comments explaining why we need to do the different parts since it is not clear to me from the code. I think this is important to make sure its maintainable.

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

ekcoh · 2025-10-03T08:32:50Z

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

+                if (mapJson.actions == null || mapJson.actions.Length == 0)
+                    continue;
+
+                for (var ai = 0; ai < mapJson.actions.Length; ++ai)


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

the iterator actionJson is immutable as the ReadActionJson is a struct so we use for loop instead of foreach.

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

ekcoh · 2025-10-03T08:35:33Z

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

+                    if (tokens.Count == 0)
+                        continue;
+
+                    var parsed = new List<NameAndParameters>(tokens.Count);


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

ekcoh · 2025-10-03T08:35:45Z

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

+                    foreach (var t in tokens)
+                        parsed.Add(NameAndParameters.Parse(t));
+
+                    var rebuiltTokens = new List<string>(tokens.Count);


same with this list

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

LeoUnity

There are a few things you mention in the PR description that I don't understand why they are needed:

Preserve original tokens and only touch the enum values

Formatting survive unchanged
I don't understand why this would be a requirement, I believe that if the user uses our editor to edit the asset that was written manually and has custom formatting that will overwrite the users formatting. Trying to parse json with string replaces, regex is a losing battle, the only way this won't be brittle is by parsing it, converting the data and serializing it back to json.

Avoid whole string rewrites, which previously caused mismatches between what the editor expected
As above, I don't understand why do we want to avoid string rewrites, my current thinking is that we didnt catch this for 2 reasons, we are not reusing the parsing and serializing funcitons that is used elsewhere, and we didnt test this case.

I believe the proper way of moving forward here is that we use the exact same code to parse and serialize json for this type that is used elsewhere in the editor, if we try to do anything different we are creating an opportunity for a bug to be hidden here.

Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs

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

Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs

This reverts commit 00b37a5.

This reverts commit a260324.

Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs

ekcoh

Same comment as @LeoUnity, let's complement the test by making sure it parses correctly (logically) and then we can conclude this I believe.

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

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

ekcoh · 2025-10-07T06:47:12Z

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

+
+        internal static NameAndParameters Create(string name, IList<NamedValue> parameters)
+        {
+            var result = new NameAndParameters


Nitpick: Why create a result variable here instead of just

return new NameAndParameters { .... };

ekcoh · 2025-10-07T06:48:54Z

.../com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/NameAndParametersListView.cs

            m_ListProperty.serializedObject.ApplyModifiedProperties();
        }

        private static string ToSerializableString(IEnumerable<NameAndParameters> parametersForEachListItem)


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

Maybe keep the old name instead?

LeoUnity

Missing an automated test case that reproduces the issue we are trying to fix here.

LeoUnity · 2025-10-07T12:54:28Z

Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs

        yield return null;
    }
+
+    [Test]


We are missing a test case here that tests that an action map containing 2 processors are working. It's the whole reason we are doing this fix. We know that with a single processor everything was working.

… ListView is editor only.

LeoUnity · 2025-10-08T12:29:08Z

Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs

+    [Test]
+    public void Migration_ShouldProduceValidActionAsset_WithAllProcessors_AndEnumConverted()
+    {
+        m_ActionMap.ResolveBindings();


This test passes without the fix, this means this is not testing what we want. The asset in the editor is created with version 1 already, the migration code is not being run at all as part of this test.

There are 2 problems with the way we create the asset:

The asset is created once and shared across the different tests, which means the order in which the tests run could impact the results of the test, this is bad.

The migration code requires the version to be 0, creating the asset via the editor will create an asset with version 1.

Also the start data already has the CustomProcessor with the value being 10, the initial value should be 1, and then the conversion should run and upgrade it to 10.

This test is not a UI test, but its part of the UIToolkitBaseTestWindow class, I would recommend splitting this test into its own class that has no dependency on the UI.

Update the parser for migration

a260324

ekcoh changed the title ~~Regression Fix: ISXB-1674 - Input actions asset not converted correctly when upgrading from 1.14.1~~ FIX: ISXB-1674 - Input actions asset not converted correctly when upgrading from 1.14.1 (Regression) Oct 2, 2025

Added a test to cover the package migration.

00b37a5

AswinRajGopal requested review from ekcoh and LeoUnity October 2, 2025 12:24

ekcoh requested changes Oct 3, 2025

View reviewed changes

LeoUnity reviewed Oct 3, 2025

View reviewed changes

AswinRajGopal added 5 commits October 3, 2025 21:44

Revert "Added a test to cover the package migration."

d637adf

This reverts commit 00b37a5.

Revert "Update the parser for migration"

4e76813

This reverts commit a260324.

The the processors are split by , instead of ; it was typo

a8ba533

Robust handling of migration logic

ff05b1d

Added test to cover the migration

829c4aa

AswinRajGopal requested review from LeoUnity and ekcoh October 6, 2025 09:09

LeoUnity reviewed Oct 6, 2025

View reviewed changes

Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs Outdated Show resolved Hide resolved

ekcoh requested changes Oct 6, 2025

View reviewed changes

ekcoh reviewed Oct 6, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Utilities/NameAndParameters.cs Show resolved Hide resolved

ekcoh reviewed Oct 6, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionAsset.cs Outdated Show resolved Hide resolved

ekcoh reviewed Oct 7, 2025

View reviewed changes

AswinRajGopal added 3 commits October 7, 2025 17:12

Update test

9467adf

Address PR feeback.

3e490f1

Address PR comments.

a31d971

LeoUnity requested changes Oct 7, 2025

View reviewed changes

AswinRajGopal added 2 commits October 8, 2025 00:24

Update the test to include more than one processors in actionmap.

817ff0e

Removed redundant call

2914bf7

AswinRajGopal requested review from ekcoh and LeoUnity October 7, 2025 19:04

Revert changes and moved the function changes to NameAndParameters as…

6daabc9

… ListView is editor only.

LeoUnity reviewed Oct 8, 2025

View reviewed changes

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

Are you sure you want to change the base?

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

Conversation

AswinRajGopal commented Sep 25, 2025

Description

Testing status & QA

Overall Product Risks

Comments to reviewers

Checklist

Uh oh!

codecov-github-com bot commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ekcoh commented Oct 2, 2025

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LeoUnity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeoUnity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-github-com bot commented Sep 25, 2025 •

edited

Loading