fix: Unity 2021.3 compat — compile errors, Mono crash, 19 test failures#1036
Merged
dsarno merged 4 commits intoCoplayDev:betafrom Apr 6, 2026
Merged
Conversation
…st fixes - Add #if UNITY_2022_2_OR_NEWER guards for FrameTimingManager.IsFeatureEnabled, FrameTiming detailed fields, and ProfilerCategory.Physics2D - Skip member reflection on open generic types (List<T>, Dictionary<TKey,TValue>) to avoid Mono segfault in mono_metadata_generic_param_equal_internal on 2021.3 - Fix UnityReflectTests to use FQN for Transform (ambiguous with log4net) - Fix CLI characterization test passing non-existent --force flag to prefab save Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix ErrorResponse field mismatch: tests reading "message" on ErrorResponse which serializes as "error" (ManageGraphics, Screenshot, MultiScene tests) - Fix ManageUITests: account for EnsureEditorExtensionMode injecting editor-extension-mode attribute; expect UXML importer errors with LogAssert - Fix ManageGraphicsTests: skip PipelineGetSettings on built-in pipeline - Fix ManageProBuilderTests: accept "not installed" when ProBuilder absent - Fix ManagePhysicsTests: offset raycast to avoid hitting unrelated scene objects - Use proper UXML content with xmlns declarations in UI tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ComponentOps.SetObjectReference handled instance IDs, asset paths, and scene names but not plain 32-char hex GUID strings. Add GUID resolution via AssetDatabase.GUIDToAssetPath before falling back to scene object lookup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Reviewer's GuideRestores green CI on Unity 2021.3 by adding missing Unity-version guards around newer profiler APIs, preventing a Mono crash in UnityReflect’s type inspection for open generic types, fixing 19 test failures (mainly error-field expectations and UXML assertions), and adding GUID shorthand resolution when setting SerializedProperty object references. Sequence diagram for GUID shorthand resolution in SetObjectReferencesequenceDiagram
actor User
participant Server
participant ComponentOps
participant AssetDatabase
User->>Server: Submit_patch_with_object_reference_value
Server->>ComponentOps: SetObjectReference(prop, value, context, out_error)
alt value_is_string
alt resolved_by_existing_instance_id_or_path
ComponentOps->>ComponentOps: AssignObjectReference(prop, resolved, context, out_error)
ComponentOps-->>Server: return_true
else not_resolved_yet
alt length_32_and_IsHexString
ComponentOps->>AssetDatabase: GUIDToAssetPath(strVal)
alt asset_path_found
ComponentOps->>AssetDatabase: LoadAssetAtPath(assetPath)
alt resolved_not_null
ComponentOps->>ComponentOps: AssignObjectReference(prop, resolved, context, out_error)
ComponentOps-->>Server: return_true
else resolved_null
ComponentOps->>ComponentOps: ResolveSceneObjectByName(prop, strVal, context, out_error)
ComponentOps-->>Server: return_result
end
else no_asset_path
ComponentOps->>ComponentOps: ResolveSceneObjectByName(prop, strVal, context, out_error)
ComponentOps-->>Server: return_result
end
else not_guid_like
ComponentOps->>ComponentOps: ResolveSceneObjectByName(prop, strVal, context, out_error)
ComponentOps-->>Server: return_result
end
end
else value_not_string
ComponentOps-->>Server: handle_non_string_value_unmodified
end
Server-->>User: Patch_result
Updated class diagram for ComponentOps and UnityReflect changesclassDiagram
class ComponentOps {
+bool SetObjectReference(SerializedProperty prop, JToken value, object context, out string error)
+bool AssignObjectReference(SerializedProperty prop, UnityEngine_Object resolved, object context, out string error)
+bool ResolveSceneObjectByName(SerializedProperty prop, string name, object context, out string error)
-long GetSpriteFileId(Sprite sprite)
-bool IsHexString(string str)
}
class UnityReflect {
+object GetTypeInfo(ToolParams p)
}
class SuccessResponse {
+SuccessResponse(string message, object data)
}
class AssetDatabase {
+string GUIDToAssetPath(string guid)
+UnityEngine_Object LoadAssetAtPath~UnityEngine_Object~(string assetPath)
}
class System_Type {
+bool IsGenericTypeDefinition
+string Name
+string FullName
+string Namespace
+System_Reflection_Assembly Assembly
}
class System_Reflection_Assembly {
+System_Reflection_AssemblyName GetName()
}
class System_Reflection_AssemblyName {
+string Name
}
ComponentOps ..> AssetDatabase : uses
UnityReflect ..> SuccessResponse : returns
UnityReflect ..> System_Type : inspects
System_Type ..> System_Reflection_Assembly : uses
System_Reflection_Assembly ..> System_Reflection_AssemblyName : returns
Flow diagram for FrameTimingOps.GetFrameTiming with Unity version guardsflowchart TD
Start([GetFrameTiming_called]) --> CheckFeature{UNITY_2022_2_OR_NEWER_and_feature_check}
CheckFeature -- feature_disabled_on_2022_2_or_newer --> Error[Return_ErrorResponse_about_enabling_Frame_Timing_Stats]
CheckFeature -- feature_enabled_or_pre_2022_2 --> Capture[FrameTimingManager.CaptureFrameTimings]
Capture --> Alloc[Allocate_single_element_FrameTiming_array]
Alloc --> Query[FrameTimingManager.GetLatestTimings]
Query --> HasData{timings_length_greater_than_zero}
HasData -- no --> NoData[Return_response_with_available_false]
HasData -- yes --> Read[Read_first_FrameTiming_t]
Read --> BaseFields[Set_base_fields:
cpu_frame_time_ms,
gpu_frame_time_ms,
cpu_time_present_called,
cpu_time_frame_complete,
height_scale,
width_scale]
BaseFields --> NewerFields{UNITY_2022_2_OR_NEWER}
NewerFields -- yes --> ExtraFields[Also_set_fields:
cpu_main_thread_frame_time_ms,
cpu_main_thread_present_wait_time_ms,
cpu_render_thread_frame_time_ms,
frame_start_timestamp,
first_submit_timestamp]
NewerFields -- no --> SkipExtra[Skip_additional_timing_fields]
ExtraFields --> ReturnSuccess[Return_SuccessResponse_with_timing_data]
SkipExtra --> ReturnSuccess
NoData --> ReturnSuccess
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds GUID-string → asset resolution to object-reference deserialization, gates Physics2D and additional frame-timing fields behind UNITY_2022_2_OR_NEWER, returns early for open generic type definitions in reflection helpers, and standardizes/relaxes multiple test assertions (error field usage and UXML resilience). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the UXML tests where you toggle
LogAssert.ignoreFailingMessages, consider wrapping the command invocation in atry/finallyblock to ensure the flag is always restored to its previous value even if an assertion fails or an exception is thrown. - The updated UXML content assertions in
ManageUITestsnow only check for a couple of substrings; to keep these tests effective at catching unintended changes, you might want to normalize out the injectededitor-extension-modeattribute (e.g., via a small helper) and still perform a stronger comparison on the full content.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the UXML tests where you toggle `LogAssert.ignoreFailingMessages`, consider wrapping the command invocation in a `try/finally` block to ensure the flag is always restored to its previous value even if an assertion fails or an exception is thrown.
- The updated UXML content assertions in `ManageUITests` now only check for a couple of substrings; to keep these tests effective at catching unintended changes, you might want to normalize out the injected `editor-extension-mode` attribute (e.g., via a small helper) and still perform a stronger comparison on the full content.
## Individual Comments
### Comment 1
<location path="TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs" line_range="686-691" />
<code_context>
string path = $"{TempRoot}/NoNs_{Guid.NewGuid():N}.uxml";
string content = "<ui:UXML><ui:Label text=\"hi\" /></ui:UXML>";
+ // Unity's UXML importer logs an error for undeclared 'ui' prefix
+ LogAssert.ignoreFailingMessages = true;
+
</code_context>
<issue_to_address>
**issue (testing):** Avoid globally flipping LogAssert.ignoreFailingMessages without a try/finally to prevent leaking state across tests
In this and the `Create_WrongRootElement_WritesWithWarning` test, `LogAssert.ignoreFailingMessages` is set to `true` and later reset without `try/finally`. If the test throws before the reset, the flag remains `true` and may hide real failures in other tests. Wrap the change in a `try`/`finally`, or preferably use `LogAssert.Expect` for the specific expected errors instead of globally ignoring failing messages.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
MCPForUnity/Editor/Tools/Profiler/Operations/CounterOps.cs (1)
123-151: Consider unifying category-resolution behavior across profiler tools.
CounterOps.ResolveCategoryfails unknown categories explicitly, whileMCPForUnity/Editor/Tools/Graphics/RenderingStatsOps.cscurrently falls back toProfilerCategory.Render(Lines 147-169 in that file). A shared resolver (or consistent error policy) would reduce drift and surprising behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/Profiler/Operations/CounterOps.cs` around lines 123 - 151, Extract the category-resolution logic into a single shared helper (e.g., a new static ProfilerCategoryResolver.Resolve(string name, out string error) that contains the mapping now in CounterOps.ResolveCategory and exposes ValidCategories), then change both CounterOps.ResolveCategory to delegate to that helper and update RenderingStatsOps to call the same helper (remove its hard fallback to ProfilerCategory.Render) so both tools use the same resolution and error policy; ensure you preserve the existing error string format using ValidCategories and keep the same conditional compilation entries like Physics2D.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs (1)
3-3: Remove unused import.
System.Text.RegularExpressionsis imported but never used in this file.Proposed cleanup
-using System.Text.RegularExpressions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs` at line 3, Remove the unused using directive "using System.Text.RegularExpressions;" from ManageUITests.cs in the TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools namespace (the file containing the ManageUITests class) so there are no unused imports; simply delete that using line and save the file to eliminate the unused reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Tools/UnityReflect.cs`:
- Around line 173-191: GetMemberInfo currently performs member reflection even
for open generic types and can trigger the Mono segfault; add the same
IsGenericTypeDefinition guard used in GetTypeInfo at the start of GetMemberInfo
so that if type.IsGenericTypeDefinition is true it returns a minimal
SuccessResponse (include found=true, name, full_name, `@namespace`, assembly,
is_generic_type_definition=true and a hint) without calling GetGenericArguments
or enumerating members; reference the GetMemberInfo method and mirror the
guard/response structure used in GetTypeInfo to keep behavior consistent.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/Profiler/Operations/CounterOps.cs`:
- Around line 123-151: Extract the category-resolution logic into a single
shared helper (e.g., a new static ProfilerCategoryResolver.Resolve(string name,
out string error) that contains the mapping now in CounterOps.ResolveCategory
and exposes ValidCategories), then change both CounterOps.ResolveCategory to
delegate to that helper and update RenderingStatsOps to call the same helper
(remove its hard fallback to ProfilerCategory.Render) so both tools use the same
resolution and error policy; ensure you preserve the existing error string
format using ValidCategories and keep the same conditional compilation entries
like Physics2D.
In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.cs`:
- Line 3: Remove the unused using directive "using
System.Text.RegularExpressions;" from ManageUITests.cs in the
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools namespace (the file
containing the ManageUITests class) so there are no unused imports; simply
delete that using line and save the file to eliminate the unused reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce6b3c9f-fa93-4ca1-b452-1c6946dc9227
📒 Files selected for processing (12)
MCPForUnity/Editor/Helpers/ComponentOps.csMCPForUnity/Editor/Tools/Profiler/Operations/CounterOps.csMCPForUnity/Editor/Tools/Profiler/Operations/FrameTimingOps.csMCPForUnity/Editor/Tools/UnityReflect.csServer/tests/test_cli_commands_characterization.pyTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePhysicsTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProBuilderTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneHierarchyPagingTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneMultiSceneTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageUITests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/UnityReflectTests.cs
- Add IsGenericTypeDefinition guard in GetMemberInfo (CodeRabbit) — same Mono crash path as GetTypeInfo, just not hit by current tests - Wrap LogAssert.ignoreFailingMessages in try/finally (Sourcery) — prevents state leak if test throws before reset - Remove unused System.Text.RegularExpressions import (CodeRabbit) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dsarno
force-pushed
the
fix/profiler-unity-version-compat
branch
from
2d08ded to
17dd7fb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context: CI has been red for 39 days
Unity Tests CI has not passed since February 24, 2026 — over 5 weeks and 36 feature/fix PRs ago (107 total merges including automated version bumps). Python Tests last passed on March 27. During this period, compile errors, a runtime crash, and test regressions accumulated undetected.
Going forward, it would help the project to confirm that both Python and Unity tests pass (including any new coverage the PR introduces) before merging. When CI is red, regressions compound silently — this PR fixes 19 test failures and a Unity-crashing segfault that were all introduced incrementally across many PRs.
What this PR fixes
Compile errors on Unity 2021.3 (7 errors)
The profiler tools (
FrameTimingOps.cs,CounterOps.cs) used APIs added in Unity 2022.2 without version guards, whilepackage.jsondeclares minimum version 2021.3.FrameTimingManager.IsFeatureEnabled()#if UNITY_2022_2_OR_NEWERFrameTiming.cpuMainThreadFrameTime(+ 4 related fields)#if UNITY_2022_2_OR_NEWERProfilerCategory.Physics2D#if UNITY_2022_2_OR_NEWERMono segfault crashing Unity (fatal)
UnityReflect.GetTypeInfo()calledGetGenericArguments(),GetInterfaces(), and member reflection on open generic type definitions (List<T>,Dictionary<TKey,TValue>). This triggers a segfault inmono_metadata_generic_param_equal_internalon Unity 2021.3 Mono — crashing the editor on every full test run.Fix: Early return with minimal type info (name, namespace, assembly) for
IsGenericTypeDefinitiontypes, avoiding all Mono metadata operations that trigger the crash.Test failures (19 fixed)
result["message"]butErrorResponseserializes as"error"result["error"]in ManageGraphics, Screenshot, MultiScene testsEnsureEditorExtensionMode()injectseditor-extension-mode="False"into UXML; tests expected exact matchDoes.Contain; addedLogAssert.ignoreFailingMessagesfor expected UXML importer errors; used proper xmlns declarationsTransformambiguous withlog4net.Util.TransformUnityEngine.TransformComponentOps.SetObjectReferencehandled instance IDs, paths, and scene names but not 32-char hex GUIDsAssetDatabase.GUIDToAssetPathbefore scene name fallbacktest_prefab_workflow_open_modify_savepassed--forcetoprefab savewhich has no such flag--forcePipelineGetSettingsexpects success but built-in pipeline has no assetAssume.Thatwhen no URP/HDRPBug fix: GUID shorthand in object references
ComponentOps.SetObjectReferencenow resolves plain 32-character hex GUID strings to assets viaAssetDatabase.GUIDToAssetPath. This was a missing code path — the JSON deserializer (UnityTypeConverters) already handled GUIDs, but the SerializedProperty patch handler did not.Test results after this PR
Test plan
cd Server && uv run pytest tests/ -v— 1210 passedUnityReflectgeneric type resolution does not crash🤖 Generated with Claude Code
Summary by Sourcery
Restore compatibility and stability on Unity 2021.3 by guarding newer profiler APIs, preventing Mono crashes in reflection utilities, and aligning tests and helpers with current engine and tooling behavior.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests