FIX: Custom processors serialises enum by index rather than by value. #2164
Conversation
|
|
…anging the value.
…tps://github.com/Unity-Technologies/InputSystem into isxb-1482/fix-custom-processor-serializesbyIndex
AswinRajGopal
changed the title
|
@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). |
|
@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. |
There was a problem hiding this comment.
Thanks for looking into this @AswinRajGopal! Left some comments on things to address.
… added, made classes and enums internal and refactor.
… added, made classes and enums internal and refactor.
…tps://github.com/Unity-Technologies/InputSystem into isxb-1482/fix-custom-processor-serializesbyIndex
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
Show resolved
Hide resolved
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
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2164 +/- ##
===========================================
+ Coverage 67.78% 68.15% +0.36%
===========================================
Files 367 367
Lines 53505 53570 +65
===========================================
+ Hits 36268 36509 +241
+ Misses 17237 17061 -176
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
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
…tps://github.com/Unity-Technologies/InputSystem into isxb-1482/fix-custom-processor-serializesbyIndex
| @@ -379,6 +398,11 @@ public void LoadFromJson(string json) | |||
| throw new ArgumentNullException(nameof(json)); | |||
|
|
|||
| var parsedJson = JsonUtility.FromJson<ReadFileJson>(json); | |||
| if ((parsedJson.maps?.Length ?? 0) > 0 && (parsedJson.version ?? 0) < JsonVersion.Current) | |||
There was a problem hiding this comment.
JsonVersion.Current here should probably be JsonVersion.Version1 since if we move on to version 2 later we do not need to do this migration step for version 1 json files?
There was a problem hiding this comment.
Additionally this may be skipped if the action:
- Do not contain actions
- Do not contain processors
There was a problem hiding this comment.
Minor: but you might as well move these "optimization" checks into the Migrate Json to remove the complexity from here and call it unconditionally, e.g. "MigrateJsonFromVersion0ToVersion1IfNeeded(ref parsedJson)"
There was a problem hiding this comment.
Yes we should skip migration in case the input action doesn't contain action and processors.
| parsedJson.maps[mi] = mapJson; | ||
| } | ||
| // Bump the version so we never re-migrate | ||
| parsedJson.version = JsonVersion.Current; |
There was a problem hiding this comment.
This should be assigned JsonVersion.Version1 right? Current might be Version 2 or higher in the future which would make this incorrect then
| internal void MigrateJson(ref ReadFileJson parsedJson) | ||
| { | ||
| var existing = parsedJson.version ?? JsonVersion.Version0; | ||
| if (existing >= JsonVersion.Current) |
There was a problem hiding this comment.
This should compare to JsonVersion.Version1 instead of Current right?
There was a problem hiding this comment.
Yes right, my bad I mis aligned the enum values. I'll modify.
|
I fixed the conflict and updated the branch based on develop. Hopefully CI rerun is all green now. |
There was a problem hiding this comment.
The value is no longer blank after updating to the new version but it still different, is that expected? Example steps are: Open the user project -> observe that that the enum is set to Option D -> update the package to your version -> observe that the enum is set to Option A.
@Pauliusd01 I've uploaded the repro, it works for me, let me know if Im missing something here. AfterFix.mp4 |
@Pauliusd01 works for me again. this repro is loading a new extracted user project and upgrading the package. |
@Pauliusd01 it couldnt be the reason as the code changes regardless of window state, looks for the asset during its import time and perform the migration, it works for me here is the recording. ClosedFix.mp4 |
There was a problem hiding this comment.
LGTM. So after quite a bit of back and forth the latest issue seems to only reproduce on first open and only in the first few minutes. Likely a library rebuild issue and it is probably not something worth worrying about as even on a rare case it occurs to the user, it would only be visual and the value returns to normal on reopening the project.
Things checked: making sure the enum values are preserved after updating the package, changing the values with manual and auto saves, changing the values in runtime, checking whether the field still works correctly in the inspector dropdown
| internal void MigrateJson(ref ReadFileJson parsedJson) | ||
| { | ||
| var existing = parsedJson.version ?? JsonVersion.Version0; | ||
| if (existing >= JsonVersion.Version1) |
There was a problem hiding this comment.
Shouldn't this be "existing >= JsonVersion.Current"
There was a problem hiding this comment.
Hi @LeoUnity We are targeting exactly the format bump that we just added(Version 1), for this enum migration
JsonVersion.Current == Version1, however in the future Current may advance to version2, so we need the migration exactly run once.
There was a problem hiding this comment.
I was assuming that this first check would be the checking if any migration needs to happen at all, and then below there would be specific checks for each migration, because you do the check again in the line below:
if ((parsedJson.maps?.Length ?? 0) > 0 && (parsedJson.version ?? 0) < JsonVersion.Version1)
So either this is checking for the specific upgrade needed, and then we remove this check on the line below, or this is a generic check if a migration is needed, and we keep the check ont he line below
| @@ -950,6 +971,7 @@ private void OnDestroy() | |||
| [Serializable] | |||
| internal struct WriteFileJson | |||
| { | |||
| public int? version; | |||
There was a problem hiding this comment.
Do we need the nullable here? the default int value is 0, so not writing to this member would get us the correct value.
Having to remember to check the null state everywhere where this is used usually makes the code a bit harder to read.
There was a problem hiding this comment.
Same in the ReadFileJson
There was a problem hiding this comment.
Hi @LeoUnity True, there are few tests failing because of this we need nullable version to avoid those failures where we omit version check for empty Json or action asset is created with minimal Json from LoadFromJson versioning will be added unnecessarily.
There was a problem hiding this comment.
I think I commented on the same thing previously. Are the tests failing due to the parser not being capable of handling non existing fields? If the nullable allows the parser to deal with non existing fields in file and still parse without errors (keeping the default value) I think it's ok to have. If we do not need it to handle parser limitations I also think we should not use nullable due to boxing.
What was the failures again caused when not having nullable?
There was a problem hiding this comment.
The minimal json in the test doesnt contain version: 0 so had to do this null check.
static string MinimalJson(string name = null)
{
if (name != null)
return "{\n "name": "" + name + "",\n "maps": [],\n "controlSchemes": []\n}";
return "{\n "maps": [],\n "controlSchemes": []\n}";
}
There was a problem hiding this comment.
Ah ok, the nullable is to make the field optional to serialize... I understand it now...
I'm wondering if we should just update those tests, from this version onwards the version field will always exist...
Is the same problem for the ReadFileJson? If you try to deserialize a json without the version field it would cause problems?
There was a problem hiding this comment.
@LeoUnity Yes, same issues with ReadFileJson as well for other set of tests, it requires patching the assets generated manually, as we only migrate in memory. So had to do this.
| } | ||
| } | ||
| // Bump the version so we never re-migrate | ||
| parsedJson.version = JsonVersion.Version1; |
There was a problem hiding this comment.
This should probably be JSonVersion.Current
There was a problem hiding this comment.
In my perspective this looks correct if this migration step only handles v0 to v1. It would need to be followed by a v1 to v2 if Current was e.g. 2
There was a problem hiding this comment.
Yeah I guess I can see that, I was expecting that after running this function we would always end up on the latest version anyways, and we would always need to remember to update the version here... but its a nitpick
There was a problem hiding this comment.
- If we’re already at or beyond the Current format, early out. This means parsedJson.version will never be re-migrated.
- Now, version-specific migration. Only assets with at least one map and older than Version1 need the enum-by-value migration.
…tps://github.com/Unity-Technologies/InputSystem into isxb-1482/fix-custom-processor-serializesbyIndex
| using UnityEngine.InputSystem.Utilities; | ||
|
|
||
| // NOTE: This is mostly auto-generated from DefaultInput.actions. To regenerate, temporarily toggle on "Generate C# Class" and |
There was a problem hiding this comment.
Yeah this diff is a bit scary, I don't have the historical context to advise here, my gut feeling is to just do the minimal changes needed manually, but I'll defer to @ekcoh here
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
Comments to reviewers
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: