chore: upgrade to Unity 6000.5.2f1#9392
Conversation
Bump the editor and migrate the DCL code that hit APIs removed in 6000.5: - EntityId replaces the obsolete int instance-id APIs. Physics.BakeMesh now takes an EntityId (Utils, BakeColliderMeshes, CollideTerrainSystem), and the collider-bake job array is typed NativeArray<EntityId>. - MetricsRegistry guards its type scan: IsAssignableFrom can force-load a precompiled dependency type that 6000.5's Mono fails to resolve, which would otherwise throw from the static ctor and take request metrics down. Verified compiling and running on 6000.5.2f1 (editor boots, enters play mode, reaches interactive). Vendored/third-party packages (SoftMask, GPUI Pro, AVPro, UniTask, com.unity.localization, FileBrowserPro, renderfeatures) also need 6000.5 API fixes; those live outside this repo and are tracked separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVEaLqBhBbZTKnvveA48Rx
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — chore: upgrade to Unity 6000.5.2f1
STEP 2 — Root-cause check: ✅ PASS
The PR fixes two distinct breakages introduced by Unity 6000.5:
-
Physics.BakeMeshAPI change — Unity 6000.5 replaced theintinstance-ID parameter withEntityId. The diff migrates all three call sites (Utils.cs,BakeColliderMeshes.cs,CollideTerrainSystem.cs) to the newGetEntityId()API and updates theNativeArray<int>typing toNativeArray<EntityId>. This is a correct root-cause fix — the old APIs were removed, and the new ones are the documented replacements. -
MetricsRegistrystatic-constructor crash —IsAssignableFromcan force-load type definitions that 6000.5's Mono can't resolve, throwing from the static initializer and taking down the entire request-metrics subsystem. The fix wraps the call inIsAssignableSafewith a try/catch, mirroring the existingGetTypesSafelypattern in the same class. This is a correct defensive fix — the root cause is in Unity's Mono runtime (not controllable by this project), and the alternative (letting the static ctor throw) is strictly worse.
STEP 3 — Design & integration: ✅ PASS
No new long-lived units introduced. The only new code is IsAssignableSafe(Type), a private static helper method that extracts error handling from the existing LINQ predicate. No lifecycle management, no persistent state, no new subscriptions or connections.
The EntityId migration is consistent across all three call sites — Utils.AddMeshCollider, CollideTerrainSystem.Update, and BakeColliderMeshes.Execute all use the same API transition. EntityId is a blittable struct in Unity 6000.5, so the [BurstCompile] job and NativeArray<EntityId> remain compatible.
STEP 4 — Member audit: ✅ PASS
IsAssignableSafe(Type type) — private static, single consumer (the Where clause in the TYPES static field initializer). Justified as an error-handling extraction: it keeps the LINQ chain readable and makes the defensive try/catch visible. The existing GetTypesSafely in the same class follows the same "extract-for-safety" pattern.
STEP 5 — Line-level review
One P2 finding (see inline comment on MetricsRegistry.cs).
- Physics API migration (Utils.cs:44, BakeColliderMeshes.cs:11, CollideTerrainSystem.cs:140-144): Correct.
GetEntityId()is the 1:1 replacement for the removedGetInstanceID()in the physics mesh-baking context. TheNativeArray<EntityId>type change is consistent with theBakeColliderMeshesjob field. - LINQ in MetricsRegistry: Runs once in a static field initializer (not a hot path). No performance concern.
- No resource leaks: No new subscriptions, connections, or allocations without matching teardown.
- No nullability violations: No null assignments to non-nullable types.
Security review: ✅ PASS
- No secrets, credentials, or API keys in the diff
- No input validation changes
- No auth/authz modifications
- No sensitive data exposure
- The bare
catchinIsAssignableSafeis defensive (more restrictive, not less) — it cannot introduce a security vulnerability - The
EntityIdmigration is a type-system change with no security surface
STEP 6 — Complexity: SIMPLE
Mechanical API migration across 3 call sites + one defensive helper method. No ECS system logic, async flow, or architectural changes.
STEP 7 — QA: YES
Changes affect runtime physics collider baking (terrain + GLTF meshes) and the metrics registry. Author reports verified compilation and interactive play mode on 6000.5.2f1.
STEP 8 — Warnings
None. Main scene not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Mechanical Unity API migration (GetInstanceID → GetEntityId) across 3 call sites plus a defensive try/catch in a static initializer
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
| private static bool IsAssignableSafe(Type type) | ||
| { | ||
| try { return typeof(RequestMetricBase).IsAssignableFrom(type); } | ||
| catch { return false; } |
There was a problem hiding this comment.
[P2] Minor: bare catch could be narrowed for clarity. The bare catch silently swallows all exception types including non-CLS-compliant ones. While the behavior is correct here (any failure from IsAssignableFrom means the type is unusable, so false is the right answer), catch (Exception) is the conventional C# idiom and would be consistent with GetTypesSafely above which catches a specific exception type.
Tradeoff acknowledged: catching broadly is intentional since IsAssignableFrom can fail with various exception types (TypeLoadException, FileNotFoundException, FileLoadException, etc.), not just the one named in the XML doc. catch (Exception) is functionally equivalent for managed exceptions but signals intent more clearly.
| catch { return false; } | |
| catch (Exception) { return false; } |
Bump the editor and migrate the DCL code that hit APIs removed in 6000.5:
Verified compiling and running on 6000.5.2f1 (editor boots, enters play mode, reaches interactive).
This fixes the NPE on Unity Editor start