FIX: ISXB-1746 Fix for non-deterministic behaviour in asset import stages. #2273
Conversation
ekcoh
changed the title
ekcoh
requested review from
Darren-Kelly-Unity,
Pauliusd01,
jfreire-unity and
ritamerkl
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent |
![u-pr-agent[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
| var importer = GetAtPath(assetPath) as InputActionImporter; | ||
| if (importer != null) | ||
| { | ||
| var asset = InputActionAsset.FromJson(File.ReadAllText(assetPath)); | ||
| if (importer.m_GenerateWrapperCode) | ||
| GenerateWrapperCode(assetPath, asset, importer.m_WrapperCodeNamespace, importer.m_WrapperClassName, importer.m_WrapperCodePath); | ||
| } |
There was a problem hiding this comment.
Suggestion: The current implementation reads and parses the asset file even if code generation is disabled, which is inefficient. Additionally, it lacks error handling for file I/O and JSON parsing, which can crash the asset import process. To improve robustness and performance, first check if code generation is enabled, then wrap the file operations in a try-catch block and check for a null asset. [possible issue, importance: 8]
| var importer = GetAtPath(assetPath) as InputActionImporter; | |
| if (importer != null) | |
| { | |
| var asset = InputActionAsset.FromJson(File.ReadAllText(assetPath)); | |
| if (importer.m_GenerateWrapperCode) | |
| GenerateWrapperCode(assetPath, asset, importer.m_WrapperCodeNamespace, importer.m_WrapperClassName, importer.m_WrapperCodePath); | |
| } | |
| var importer = GetAtPath(assetPath) as InputActionImporter; | |
| if (importer != null && importer.m_GenerateWrapperCode) | |
| { | |
| try | |
| { | |
| var asset = InputActionAsset.FromJson(File.ReadAllText(assetPath)); | |
| if (asset != null) | |
| GenerateWrapperCode(assetPath, asset, importer.m_WrapperCodeNamespace, importer.m_WrapperClassName, importer.m_WrapperCodePath); | |
| } | |
| catch (Exception e) | |
| { | |
| Debug.LogError($"Failed to generate wrapper code for '{assetPath}': {e}"); | |
| } | |
| } |
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent
There was a problem hiding this comment.
My bad, will update to fix. Also realise we can keep error as importer error by adjusting it slightly. Will update.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2273 +/- ##
===========================================
+ Coverage 76.70% 76.81% +0.10%
===========================================
Files 465 476 +11
Lines 87919 88726 +807
===========================================
+ Hits 67442 68155 +713
- Misses 20477 20571 +94 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 24 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
LGTM, I checked the original repro steps and did some exploratory testing such as trying to generate the script while importing something, moving/renaming the generated scripts, using symbols for action and map names, reimports, etc
Also looked for previously logged issues related to generated C# classes - ISXB-1257 (still fixed), ISXB-529 (was closed as won't fix, still happening), ISXB-1367 (unfixed and still occurring)
ritamerkl
left a comment
ritamerkl
left a comment
There was a problem hiding this comment.
Looks good, it would be great to have a test too.
The importer could definitely benefit from better test coverage but since it's indirectly covered by many tests, e.g. InputActionReferenceEditorTests and others for the "happy flow" and covered in most manual test cases I will not expand this as part of this PR. |
Description
This is a suggested fix for HUP: https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1746
Follows suggested approach in: https://discussions.unity.com/t/changes-to-assetdatabase-apis-when-called-during-import/1689358, but not exactly since that approach suggest generating a .cs file in library temp folder in that case which doesn't really solve anything for this use case.
This PR do not fix the existing problems:
Generally, IMO we should consider adding this feature to the list of potential things to remove.
Testing status & QA
Only tested manually with 6000.3 beta.
Overall Product Risks
We need to ensure there is not new side-effects to projects and/or CI with the change in approach.
Comments to reviewers
Nothing special to call out apart from this being modified previously due to CI hanging in batch mode: #2091
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: