fix: add missing Genesis City roads and repair road data - #9568
Conversation
- Add 66 single-parcel roads missing from RoadData.asset, sourced from the deployed catalyst road scenes (model + rotation from each scene's content). Covers all still-valid parcels from #3279; 43,-112 is excluded because it is no longer a road. - Repair 4 dead-end entries that carried an invalid zero quaternion (their scenes were deployed with an undefined rotation). - Remap 72 parcels whose road model has no prefab to the nearest same-family variant, instead of the generic OpenRoad_0 fallback (e.g. Crossroads_D -> Crossroads_C). HalfFork* parcels keep their names on purpose: no same-shape prefab exists and OpenRoad_0 is the closest fit. - Regenerate the GPU-instancing bake (IndirectLODGroups) from the completed descriptions. - Sync SingleParcelRoadInfo.json to mirror the asset so a future ParseRoadsFiles regeneration cannot revert hand-applied fixes. - Fix CollectGPUInstancingLODGroups not persisting: Undo.RecordObject defers the dirty flag, so SaveAssetIfDirty in the same call stack skipped the write; mark the asset dirty explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 1 — Scope
Files changed: 3
Explorer/Assets/DCL/Roads/Settings/RoadSettingsAsset.cs— 3 lines added (editor-only bug fix)Explorer/Assets/DCL/Roads/Data/SingleParcelRoadInfo.json— full JSON re-sync with assetExplorer/Assets/DCL/Roads/Settings/RoadData.asset— road descriptions + regenerated GPU instancing bake (93k/149k lines, dominated by auto-generated transform matrices)
Subsystem: Roads / GPU Instancing (editor tooling + serialized data).
STEP 2 — Root-cause check
Problem: Genesis City roads were missing because RoadData.asset had drifted out of sync with the road scenes deployed on the catalyst.
Root cause identified and fixed: The persistence bug in CollectGPUInstancingLODGroups — Undo.RecordObject defers the dirty flag to end-of-frame, so SaveAssetIfDirty in the same call stack saw a clean object and skipped the write. The fix adds EditorUtility.SetDirty(this) before the save call, which is the standard Unity pattern for this exact scenario. The data corrections (66 new entries, 4 quaternion repairs, 72 model remaps, JSON re-sync) address the accumulated drift.
✅ PASS — the diff fixes the cause, not a symptom.
STEP 3 — Design & integration
No new long-lived units introduced. The only code change is a single EditorUtility.SetDirty(this) call added to an existing editor method on the existing RoadSettingsAsset ScriptableObject. No new systems, plugins, managers, or state-holding helpers.
Owner search: CollectGPUInstancingLODGroups is the method that owns the bake lifecycle — it creates the instancing data, writes it to the asset's IndirectLODGroups, and saves. The SetDirty call is placed exactly at the right point: after all modifications (ExtractSameRenderers()) and before the save. This is the natural home for the fix.
Teardown trace: No subscriptions, callbacks, connections, or disposable resources added. The method runs to completion in the editor and writes to the asset. No teardown needed.
✅ PASS — no design concerns.
STEP 4 — Member audit
No new public properties, accessors, or methods added. The SetDirty call is a Unity Editor API invocation within an existing method. No audit targets.
✅ PASS.
STEP 5 — Line-level review
Pass A — Blocking issues: None found.
- The
SetDirtyfix is correct and well-commented. - No null safety issues —
thisis always non-null in an instance method. - No LINQ in runtime paths — the existing LINQ usage (
Select,OrderBy,ToList) is inside#if UNITY_EDITOR, editor-only. - No resource leaks, no async patterns, no new allocations.
- The
UnityEditor.EditorUtilityis fully qualified, consistent with the existingUnityEditor.AssetDatabaseandUnityEditor.Undousage in the same method.
Pass B — Design & encapsulation smells: None found.
- No new members, no naming issues, no magic values.
- The comment accurately explains why
SetDirtyis needed without narrating external behavior.
Data validation (RoadDescriptions):
- 66 newly added road entries — each has a valid coordinate, rotation (non-zero quaternion), and model name. ✅
- 4 zero quaternion repairs (
{x:0, y:0, z:0, w:0}→{x:0, y:0, z:0, w:1}identity) — correct, matches the existing runtime guard in the same method (lines 73–77). ✅ - 72 model remaps — all follow consistent family patterns (
Crossroads_D→C,DeadEnd_B→A,DeadEnd_D→C,EmptyFork_A/C→B,OpenCorner_D→C,OpenFork_D→C). These map to variants that exist inRoadAssetsReference. ✅ - No remaining
(0,0,0,0)quaternions in added lines. ✅ HalfForkLeft_0/HalfForkRight_0intentionally kept (no equivalent prefab exists; falls back toOpenRoad_0via the existing runtime guard). ✅
✅ PASS — no blocking issues.
STEP 6 — Security review
No security issues found. The change is editor-only tooling code and static road geometry data. No secrets, credentials, user input handling, auth, or network changes.
STEP 7 — Non-blocking warnings
No Main.unity or its .meta in the changed files. No warnings.
Verdict
Clean PR. The 3-line SetDirty fix is the correct and standard Unity solution for the Undo.RecordObject + SaveAssetIfDirty interaction bug. The data corrections are thorough and well-validated. No code quality, performance, or security concerns.
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: 3-line editor-only bug fix in RoadSettingsAsset.cs plus serialized road data corrections — no runtime logic, ECS, async, or system changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Warnings count reduced: 13974 => 13764 |
|
All Unity tests passed ✅
|
ExtractSimilarMeshes bakes SharedMaterial.color (_BaseColor) into per-instance tints, but the hedge materials were reworked after the previous bake and _BaseColor no longer holds the foliage colour (pale blue on HedgeLeafGreen/HedgeLeafYellow), so the re-bake turned all instanced hedges white. Restore the last-known-good tints from the previous bake: foliage green for HedgeLeafGreen instances and autumn orange for HedgeLeafYellow instances (8,767 instances across the *_HedgeLeafGreen groups). Verified in-editor against the old bake's rendering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: fix: add missing Genesis City roads and repair road data
STEP 2 — Root-cause check: PASS ✅
The PR correctly identifies and fixes the root cause of the historical asset/JSON drift: Undo.RecordObject defers its dirty flag to end-of-frame, so SaveAssetIfDirty in the same call stack saw a clean object and silently skipped the write. The EditorUtility.SetDirty(this) call is the canonical Unity Editor fix for this specific timing problem — it makes the dirty flag synchronously visible so the save actually persists. This is a genuine root-cause fix, not a symptom workaround.
STEP 3 — Design & integration: PASS ✅
No new systems, plugins, managers, or controllers are introduced. The code change is a 1-line API call added to an existing editor-only method (#if UNITY_EDITOR). No runtime code paths are affected.
Lifecycle / owner analysis: CollectGPUInstancingLODGroups lives in RoadSettingsAsset, which is the ScriptableObject that owns the GPU instancing bake data. The method is called from the custom inspector button in RoadSettingsAssetEditor.cs (line 100). The fix operates on this — the asset itself — which is the correct owner. No parallel mechanism or reconciler is introduced.
The data changes (66 new parcels, 4 repaired dead-ends, 72 remapped models, regenerated bake) are corrections to the serialized asset data, validated against the live catalyst. The JSON sync prevents future ParseRoadsFiles runs from reverting manual fixes.
STEP 4 — Member audit: N/A
No new public properties or accessors are added.
STEP 5 — Line-level review: No issues
Code change (3 lines):
EditorUtility.SetDirty(this)is the correct Unity API for this purpose. ✅Undo.RecordObject(line 53) is correctly placed before mutations for undo support —SetDirtyis complementary, not redundant. ✅- The comment explains why the call is needed (Undo timing quirk) — follows CLAUDE.md conventions (explains what annotated code does/guarantees, not caller behavior). ✅
- Editor-only code stays within
#if UNITY_EDITOR— no runtime impact. ✅
Subscription/resource leak trace: No subscriptions, events, connections, or disposables are added. N/A.
Security review: No security issues. No runtime code, no user input handling, no auth changes, no secrets. Data files contain non-sensitive road coordinates, model names, and quaternion rotations.
Pre-existing observation (out of scope): The "Update road" button in RoadSettingsAssetEditor.cs (line 69-74) modifies RoadDescriptions without Undo.RecordObject, SetDirty, or save — the same category of bug this PR fixes for the bake path. Worth a follow-up.
STEP 6 — Complexity: SIMPLE
3 files changed, 3 lines of meaningful code change. The remainder is regenerated serialized asset data and a JSON data sync. No ECS, async, plugin, or runtime changes.
STEP 7 — QA: YES
The data changes affect road rendering at runtime (visible road pieces and GPU-instanced vegetation tints). Manual visual verification at the listed coordinates is warranted.
STEP 8 — Non-blocking warnings: None
Main scene is not modified.
CI Status
- ✅ All Unity tests passed (EditMode: 24,332 / PlayMode: 236)
- ✅ Semantic title check passed
- ⏳ Build (Windows/macOS), Lint pending
- ⏳ QA approval pending
Review Agents Used
- Architecture strategist — PASS (fix is canonical Unity pattern; data approach appropriate)
- Security sentinel — PASS (no security issues)
- Pattern recognition / simplicity — PASS (minimal fix, good comment, no anti-patterns)
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Editor-only bug fix (3 lines) plus regenerated road data assets; no ECS, async, or runtime logic changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack
- Initialize the serialized collections (CS8618); Unity's deserializer overwrites them on load. - Make the editor-only road-tile cache nullable and assign it with ??=, avoiding the non-nullable CombinedLODGroupData equality operator (CS8604, always-false null check). - Add GPU and LOD to the ReSharper abbreviations list so InspectCode stops suggesting Gpu/Lod casing for the codebase-wide GPU*/LOD* names (renaming IndirectLODGroups would also orphan the serialized bake data in RoadData.asset). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Pull Request Description
What does this PR change?
Closes #3279
Several roads were missing in Genesis City because
RoadData.asset(the runtime source for road placement) had drifted out of sync with the road scenes actually deployed on the catalyst.A full sweep of the map against the catalyst found and fixed more than the issue's list:
.glbname and the rotation embedded in itsgame.js).43,-112from the issue is deliberately not added: the catalyst now serves a private scene there, so it is no longer a road.(0,0,0,0)— their scenes were deployed with an undefined rotation; identity is substituted (same as the parser's own zero-rotation guard produces).RoadAssetsReferenceto the nearest same-family variant (Crossroads_D→C,DeadEnd_B→A,DeadEnd_D→C,EmptyFork_A/C→B,OpenCorner_D→C,OpenFork_D→C). These previously rendered as a generic straightOpenRoad_0— e.g. a crossroads drawn as a straight road.HalfForkLeft_0/HalfForkRight_0(35 parcels) intentionally keep their names: no same-shape prefab exists,OpenRoad_0is the closest fit for their wide-junction-edge geometry, and they self-heal if the prefabs are ever added.IndirectLODGroups, 75k instance transforms) from the completed descriptions — this is most of the diff churn.ExtractSimilarMeshesbakesSharedMaterial.color(_BaseColor) into per-instance tints, but the hedge materials were reworked after the previous bake and_BaseColorno longer holds the foliage colour (it is a pale blue onHedgeLeafGreen/HedgeLeafYellow) — a fresh bake therefore renders all instanced hedges white. This PR restores the last-known-good tints (foliage green / autumn orange, 8,767 instances) from the previous bake. Follow-up needed (rendering team): fix the extractor or the materials, otherwise the nextCollect GPU Instancing LOD Groupsrun reintroduces white plants.SingleParcelRoadInfo.jsonto be a full value mirror of the asset (it was 112 entries behind, including 16 hand-applied fixes present only in the asset), so a futureDecentraland/Roads/ParseRoadsFilesregeneration cannot silently revert manual fixes.CollectGPUInstancingLODGroupsnot persisting its result:Undo.RecordObjectdefers the dirty flag to end-of-frame, soSaveAssetIfDirtyin the same call stack saw a clean object and skipped the write. The asset is now marked dirty explicitly. This bug is a likely cause of the historical asset/JSON drift.Data validation performed: 7,799 unique entries, zero duplicates, zero invalid quaternions, asset↔JSON full value agreement, all entries confirmed against live catalyst road tiles, every one of the 66 new parcels verified present in the regenerated instancing bake, and hedge tint palette verified identical to the previous bake. Verified in-editor: new roads render at the issue coordinates with correctly coloured vegetation.
Test Instructions
Steps (standard run):
Expected result:
Roads render at the previously-missing parcels listed below (both up close and from a distance/height, which exercises the GPU-instanced path), with normal green/orange hedge vegetation.
Steps (fresh account):
Expected result:
Same as standard run — road rendering does not depend on account state.
Prerequisites
Test Steps
37,-117and look east/west along the road row — the gaps at42,-117and45,-117are now filled.41,-115,41,-113,41,-112,32,-115,20,-112,30,-117,28,-117,52,-115,52,-116,3,-92,8,-92— a road piece renders at each.43,-112does NOT render a road (the parcel is a private scene now).20,-112(corner) — it renders as a corner, not a straight road.41,-115or38,-115) — hedges are green (some autumn orange), not white.Additional Testing Notes
N/A
Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.