FIX: Custom processors serialises enum by index rather than by value. #2164

AswinRajGopal · Apr 8, 2025

Description

Bug: https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1474
Port 1.13.1

The issue is that the DropdownField expects a selected index (starting at 0), but the enum’s underlying values (10, 20, 30..) don't match these indices. Instead of passing the enum’s integer value directly, I have mapped the value to the correct index in the list of enum options.

An automated test added which validates that a custom input processor with an enumerated parameter integrates correctly with both the Unity Input System’s asset serialization and its UI.

The issue is that

Testing status & QA

Verified manually in windows machine with the provided repro project.
Authored an automated test to cover the fix.

Observer details of the test run:

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.

unity-cla-assistant · Apr 8, 2025

All committers have signed the CLA.

…anging the value.

…tps://github.com/Unity-Technologies/InputSystem into isxb-1482/fix-custom-processor-serializesbyIndex

…byIndex

ekcoh · Apr 22, 2025

@AswinRajGopal Its generally recommended to add one developer on the team and one QA as reviewers on the PR when published. This will help making sure you receive feedback on it.

Looks like there are CI failures for non-required tests and that branch is outdated. I would recommend updating the branch (which will automatically re-run CI).

ekcoh · Apr 22, 2025

@AswinRajGopal I notice you have published internal URLs in this public-facing PR. This should be avoided. Instead use public URLs to issue tickets when available and otherwise only use issue number (without link). I will remove the URLs for now.

ekcoh

Thanks for looking into this @AswinRajGopal! Left some comments on things to address.

Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs

Packages/com.unity.inputsystem/CHANGELOG.md

…byIndex

… added, made classes and enums internal and refactor.

…tps://github.com/Unity-Technologies/InputSystem into isxb-1482/fix-custom-processor-serializesbyIndex

Assets/Tests/InputSystem.Editor/CustomProcessorEnumTest.cs

ekcoh

Thanks for addressing comments. Changes look good to me now.

Pauliusd01

One concern I noticed is that the enum field is blank after upgrading to the new version, requiring the user to repick and save. Real problem? Any way to fix it?

AswinRajGopal · May 1, 2025

One concern I noticed is that the enum field is blank after upgrading to the new version, requiring the user to repick and save. Real problem? Any way to fix it?

Hi @Pauliusd01 could you please give me the steps to repro this issue? which version should I upgrade? Thanks.

Pauliusd01 · May 2, 2025

One concern I noticed is that the enum field is blank after upgrading to the new version, requiring the user to repick and save. Real problem? Any way to fix it?

Hi @Pauliusd01 could you please give me the steps to repro this issue? which version should I upgrade? Thanks.

Your version. Steps are: Open the user project -> Upgrade the version to yours (package manager -> add from disk -> point to the version with your changes) -> observe the user's enum field is blank

Packages/com.unity.inputsystem/InputSystem/Editor/AssetEditor/ParameterListView.cs

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs

codecov-github-com · May 24, 2025

Codecov Report

Attention: Patch coverage is 87.69231% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...System/Editor/AssetImporter/InputActionImporter.cs	84.78%	7 Missing ⚠️
...nputSystem/Editor/AssetEditor/ParameterListView.cs	94.73%	1 Missing ⚠️

@@             Coverage Diff             @@
##           develop    #2164      +/-   ##
===========================================
+ Coverage    65.45%   67.69%   +2.23%     
===========================================
  Files          367      367              
  Lines        53505    53565      +60     
===========================================
+ Hits         35024    36261    +1237     
+ Misses       18481    17304    -1177

Flag	Coverage Δ
linux_2021.3_project	`70.39% <45.65%> (-0.03%)`	⬇️
linux_2022.3_project	`65.23% <32.30%> (-0.04%)`	⬇️
linux_6000.0_project	`67.57% <87.69%> (+2.21%)`	⬆️
linux_6000.1_project	`67.57% <87.69%> (+2.22%)`	⬆️
linux_trunk_project	`67.57% <87.69%> (+2.21%)`	⬆️
mac_2021.3_pkg	`5.41% <0.00%> (-0.01%)`	⬇️
mac_2021.3_project	`70.39% <45.65%> (-0.03%)`	⬇️
mac_2022.3_pkg	`5.19% <0.00%> (-0.01%)`	⬇️
mac_2022.3_project	`65.25% <32.30%> (-0.04%)`	⬇️
mac_6000.0_pkg	`5.19% <0.00%> (-0.01%)`	⬇️
mac_6000.0_project	`67.61% <87.69%> (+2.23%)`	⬆️
mac_6000.1_pkg	`5.19% <0.00%> (-0.01%)`	⬇️
mac_6000.1_project	`67.60% <87.69%> (+2.23%)`	⬆️
mac_trunk_pkg	`5.19% <0.00%> (-0.01%)`	⬇️
mac_trunk_project	`67.59% <87.69%> (+2.22%)`	⬆️
win_2021.3_pkg	`5.41% <0.00%> (-0.01%)`	⬇️
win_2021.3_project	`70.47% <45.65%> (-0.03%)`	⬇️
win_2022.3_pkg	`5.19% <0.00%> (-0.01%)`	⬇️
win_2022.3_project	`65.33% <32.30%> (-0.04%)`	⬇️
win_6000.0_pkg	`5.20% <0.00%> (-0.01%)`	⬇️
win_6000.0_project	`67.67% <87.69%> (+2.22%)`	⬆️
win_6000.1_pkg	`5.20% <0.00%> (-0.01%)`	⬇️
win_6000.1_project	`67.67% <87.69%> (+2.22%)`	⬆️
win_trunk_pkg	`5.20% <0.00%> (-0.01%)`	⬇️
win_trunk_project	`67.66% <87.69%> (+2.22%)`	⬆️

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

Files with missing lines	Coverage Δ
...nputSystem/Editor/AssetEditor/ParameterListView.cs	`41.11% <94.73%> (+41.11%)`	⬆️
...System/Editor/AssetImporter/InputActionImporter.cs	`56.12% <84.78%> (+6.36%)`	⬆️

... and 34 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

Changes look ok to me, added some suggestions to motivate complex code parts for increased readability. Would also recommend checking the codecov bot suggestions for logic not being tested.

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs

LeoUnity

I think the PR as it is increases the tech debt in a way that is not worth the fix.
I believe there are better ways to achieve the desired outcome, and if not I would rather take the fix without the migration.
Take a look at migrating the data during importing, let me know if you have any questions

… the enums by value in the processors.

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs

…o handle migration.

ekcoh

Marking this Request Changes based on code being in impropriate places given that this is handling the migration in-memory (See comments).

While this PR now handles multi-version parsing I struggle with two main issues:

The parsing is inside the importer which is fine in one sense, but it implies this won't work (produce incorrect results), for a JSON loaded in a player via LoadFromJson directly.
The migration code is heavily modifying strings which is unfortunate but maybe not possible to avoid since the processor definitions are actually strings to begin with. So as long as we mark dirty the user is likely not experiencing this much anyway if existing assets are migrated in edit-mode.

It might be that migrating directly in https://github.com/Unity-Technologies/InputSystem/blob/develop/Packages/com.unity.inputsystem/InputSystem/Actions/InputActionMap.cs#L1609 reduces the complexity since this is where an action is parsed. However doing this might require the version to be parsed first prior to parsing the asset to be able to reason about the version.

ekcoh · Jun 2, 2025

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

@@ -946,10 +947,12 @@ private void OnDestroy()
        [NonSerialized] internal InputActionRebindingExtensions.ParameterOverride[] m_ParameterOverrides;

        [NonSerialized] internal InputActionMap.DeviceArray m_Devices;
+        [SerializeField] internal int m_Version;


The introduced "version" seems related to the the file format and not the asset object right? Hence I would recommend removing this field. I.e. in a code-only project version doesn't play any role, hence its misplaced IMO and we would better not associate it with the asset.

I am unresolving this since I do not see why we need this. In a project without assets version has no purpose. Only the file format is (needs to) be versioned?

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

Packages/com.unity.inputsystem/InputSystem/Editor/AssetEditor/ParameterListView.cs

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs

…arser, moved the migration funciton to the InputActionAsset.

ekcoh

Thanks for updating. Some additional (hopefully final) batch of feedback.

ekcoh · Jun 5, 2025

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


        [Serializable]
        internal struct WriteFileJson
        {
+            public int? version;


I would prefer that this is an int (not nullable), it could default to version 0 (file format version)

ekcoh · Jun 5, 2025

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

@@ -946,10 +947,12 @@ private void OnDestroy()
        [NonSerialized] internal InputActionRebindingExtensions.ParameterOverride[] m_ParameterOverrides;

        [NonSerialized] internal InputActionMap.DeviceArray m_Devices;
+        [SerializeField] internal int m_Version;


I am unresolving this since I do not see why we need this. In a project without assets version has no purpose. Only the file format is (needs to) be versioned?

ekcoh · Jun 5, 2025

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

@@ -379,6 +383,12 @@ public void LoadFromJson(string json)
                throw new ArgumentNullException(nameof(json));

            var parsedJson = JsonUtility.FromJson<ReadFileJson>(json);
+
+            m_Version = parsedJson.version;
+            if (m_Version <= 13)


I would propose introducing documented constants for the JSON versions introduced by this PR, e.g.

private static struct JsonVersion { /// <summary>The original JSON version format for InputActionAsset.</summary> public static const int Version0 = 0, /// <summary>Updated JSON version format for InputActionAsset.</summary> /// <remarks>Changes representation of parameter values from being serialized by value to being serialized by value.</remarks> public static const int Version1 = 1; /// <summary>The current version.</summary> public static const int Current = Version1; }

To create a natural place to call out all changes in-between those formats and remove "magic values". Hence that constant could be checked against here.

I might have incorrectly pushed against matching this with importer version previously but not sure that was the correct move.

ekcoh · Jun 5, 2025

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

@@ -298,6 +301,7 @@ public string ToJson()
        {
            return JsonUtility.ToJson(new WriteFileJson
            {
+                version = (m_Version >= 13) ? (int?)m_Version : null,


Shouldn't this always be the latest (current version)? I see no need to generate obsolete formats?

version = JsonVersion.Current;

ekcoh · Jun 5, 2025

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

            public string name;
            public InputActionMap.ReadMapJson[] maps;
            public InputControlScheme.SchemeJson[] controlSchemes;

            public void ToAsset(InputActionAsset asset)
            {
+                asset.m_Version = version;


Why do we need to store this? I think it should be removed.

ekcoh · Jun 5, 2025

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

+                        continue;
+
+                    var rebuilt = new List<string>(parsedList.Count);
+


Nitpick: Remove empty line

ekcoh · Jun 5, 2025

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

+                        {
+                            var paramString = string.Join(",", dict.Select(kv => $"{kv.Key}={kv.Value}"));
+                            var newNamedValues = NamedValue.ParseMultiple(paramString);
+


Nitpick: remove empty line

ekcoh · Jun 5, 2025

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

+                        var dict = nap.parameters.ToDictionary(pv => pv.name, pv => pv.value.ToString());
+
+                        bool anyChanged = false;
+


Nitpick: Remove empty line

ekcoh · Jun 5, 2025

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

+                        }
+
+                        var procType = InputSystem.TryGetProcessor(nap.name);
+


Nitpick: Remove empty line

ekcoh · Jun 5, 2025

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

+                foreach (var action in map.actions)
+                {
+                    var raw = action.processors;
+


Nitpick: Remove empty line

AswinRajGopal added 2 commits April 7, 2025 20:18

Added test to cover the fix

6504c53

Determine index from enum value and serialise accordingly for the asset.

e9cff62

AswinRajGopal added 3 commits April 17, 2025 19:10

Fix test failure after fix, had to focus the dropdown field before ch…

7284150

…anging the value.

Determine index from enum value and serialise accordingly for the asset.

73890bd

Merge branch 'isxb-1482/fix-custom-processor-serializesbyIndex' of ht…

9020c5e

…tps://github.com/Unity-Technologies/InputSystem into isxb-1482/fix-custom-processor-serializesbyIndex

AswinRajGopal marked this pull request as ready for review April 17, 2025 14:00

AswinRajGopal and others added 2 commits April 17, 2025 19:30

Merge branch 'develop' into isxb-1482/fix-custom-processor-serializes…

9819aab

…byIndex

Update CHANGELOG.md

43278ad

AswinRajGopal changed the title ~~Isxb 1482/fix custom processor serializesby index~~ FIX: Custom processors serialises enum by index rather than by value. Apr 19, 2025

ekcoh requested changes Apr 22, 2025

View reviewed changes

ekcoh requested a review from Pauliusd01 April 22, 2025 18:40

Pauliusd01 and others added 4 commits April 23, 2025 12:14

Merge branch 'develop' into isxb-1482/fix-custom-processor-serializes…

a06e9d3

…byIndex

Address PR feedbacks, changelog changed, test name modified, comments…

580e931

… added, made classes and enums internal and refactor.

Address PR feedbacks, changelog changed, test name modified, comments…

774190a

… added, made classes and enums internal and refactor.

Merge branch 'isxb-1482/fix-custom-processor-serializesbyIndex' of ht…

6d50a6b

…tps://github.com/Unity-Technologies/InputSystem into isxb-1482/fix-custom-processor-serializesbyIndex

AswinRajGopal requested a review from ekcoh April 23, 2025 11:42

ekcoh reviewed Apr 23, 2025

View reviewed changes

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

ekcoh approved these changes Apr 23, 2025

View reviewed changes

Pauliusd01 reviewed Apr 24, 2025

View reviewed changes

Upgrade compatibility to fill the dropdown field with value

7c7b927

Pauliusd01 self-requested a review May 21, 2025 06:34

ekcoh approved these changes May 22, 2025

View reviewed changes

AswinRajGopal requested a review from ekcoh May 25, 2025 03:40

ekcoh approved these changes May 26, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs Outdated Show resolved Hide resolved

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs Outdated Show resolved Hide resolved

LeoUnity reviewed May 26, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs Outdated Show resolved Hide resolved

LeoUnity requested changes May 26, 2025

View reviewed changes

Implement migrator for the old input action asset as we now serialize…

a521b32

… the enums by value in the processors.

AswinRajGopal force-pushed the isxb-1482/fix-custom-processor-serializesbyIndex branch from 6bfc950 to a521b32 Compare May 30, 2025 11:26

AswinRajGopal requested a review from LeoUnity May 30, 2025 11:31

ekcoh reviewed May 30, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs Outdated Show resolved Hide resolved

ekcoh reviewed May 30, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs Show resolved Hide resolved

Removed additional version constant and used the existing k_Version t…

8c65e40

…o handle migration.

ekcoh requested changes Jun 2, 2025

View reviewed changes

LeoUnity reviewed Jun 2, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Editor/AssetImporter/InputActionImporter.cs Outdated Show resolved Hide resolved

AswinRajGopal added 2 commits June 4, 2025 13:35

Handle migration for LoadFromJson aswell and used NameAndParamerter p…

37d1dfb

…arser, moved the migration funciton to the InputActionAsset.

Remove log statements and unused header.

8bc3ef0

AswinRajGopal force-pushed the isxb-1482/fix-custom-processor-serializesbyIndex branch from accd69f to 8bc3ef0 Compare June 4, 2025 08:13

AswinRajGopal requested review from LeoUnity and ekcoh June 4, 2025 08:14

Fix test failures and compiler errors.

359f03d

ekcoh requested changes Jun 5, 2025

View reviewed changes

		var dict = nap.parameters.ToDictionary(pv => pv.name, pv => pv.value.ToString());

		bool anyChanged = false;

Search code, repositories, users, issues, pull requests...

FIX: Custom processors serialises enum by index rather than by value. #2164

Are you sure you want to change the base?

FIX: Custom processors serialises enum by index rather than by value. #2164

Conversation

AswinRajGopal commented Apr 8, 2025 • edited by ekcoh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing status & QA

Overall Product Risks

Comments to reviewers

Checklist

Uh oh!

unity-cla-assistant commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekcoh commented Apr 22, 2025

Uh oh!

ekcoh commented Apr 22, 2025

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Pauliusd01 left a comment

Choose a reason for hiding this comment

Uh oh!

AswinRajGopal commented May 1, 2025

Uh oh!

Pauliusd01 commented May 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ekcoh left a comment

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!

ekcoh 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AswinRajGopal commented Apr 8, 2025 •

edited by ekcoh

Loading

unity-cla-assistant commented Apr 8, 2025 •

edited

Loading

codecov-github-com bot commented May 24, 2025 •

edited

Loading