FIX: ISXB-1674 - Input actions asset not converted correctly when upgrading from 1.14.1 (Regression) #2244
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2244 +/- ##
===========================================
+ Coverage 68.14% 76.72% +8.57%
===========================================
Files 367 465 +98
Lines 53685 87919 +34234
===========================================
+ Hits 36584 67455 +30871
- Misses 17101 20464 +3363 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 100 files with indirect coverage changes 🚀 New features to boost your workflow:
|
ekcoh
changed the title
|
I noticed this PR title was incorrect so I changed it to remove that failure. |
| if (mapJson.actions == null || mapJson.actions.Length == 0) | ||
| continue; | ||
|
|
||
| for (var ai = 0; ai < mapJson.actions.Length; ++ai) |
There was a problem hiding this comment.
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.
| if (mapJson.actions == null || mapJson.actions.Length == 0) | ||
| continue; | ||
|
|
||
| for (var ai = 0; ai < mapJson.actions.Length; ++ai) |
There was a problem hiding this comment.
It looks like 'ai' isn't really used so why not e.g foreach instead?
| if (parts.Length == 0) | ||
| continue; | ||
|
|
||
| var tokens = new List<string>(); |
There was a problem hiding this comment.
Since this is inside a loop I would recommend moving tokens outside the loop and just do clear here to avoid GC pressure.
| if (tokens.Count == 0) | ||
| continue; | ||
|
|
||
| var parsed = new List<NameAndParameters>(tokens.Count); |
There was a problem hiding this comment.
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); |
| continue; | ||
| } | ||
|
|
||
| var dict = new Dictionary<string, string>(nap.parameters.Count, System.StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
This dict could also be outer scope?
| 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)) |
There was a problem hiding this comment.
I do not understand what the goal is here, could you add inline comments explaining?
| } | ||
| } | ||
|
|
||
| if (!changedThisProcessor) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sure I'll add comments in detail.
There was a problem hiding this comment.
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.
| 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)")) |
There was a problem hiding this comment.
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.
| if (parsedJson.version >= JsonVersion.Version1) | ||
| return; | ||
| if ((parsedJson.maps?.Length ?? 0) > 0 && (parsedJson.version) < JsonVersion.Version1) | ||
|
|
There was a problem hiding this comment.
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...
| "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), |
There was a problem hiding this comment.
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.
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:
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: