UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9)#1575
Conversation
…amera, one box list build_room_unified.cs: renders greybox+depth+normal FROM the client camera (C# fit-ortho port == Python to the digit: 10.5224), builds walls from the authored wall_run props (never invented), renders multi-cell props as ONE spanning box (kills the weak-depth-cue class that caused the crypt furniture relocation), captures depth via WOS/LinDepthRemap over the measured scene range, and emits room_boxes.json — the EXACT volumes the paint was conditioned on. CombatSurfaceClient.cs: (1) per-plate 'boxes' sidecar -> depth-proxy occluders built from the conditioning volumes verbatim (walls included — the playtest-#9 walk-through-wall class); (2) G-key / WORLDOS_TRUTH_OVERLAY=1 engine-truth overlay (cell states + occluder wireframes) — the permanent 'is paint lying?' instrument; (3) QA /shot endpoint (in-app framebuffer capture, no desktop capture, no titlebar calibration). Evidence (qa/evidence/unify-frames/): unified greybox/depth/normal + boxes.json + the flux depth-CN base painted ON the boxes (coherence: sarcophagus 0.30 cells, pillar 0.10, niches 0.00, vs the old pipeline's 4-cell tomb relocation) + the in-player truth-overlay proof shot + the Options-2/3 research synthesis.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (40)
📒 Files selected for processing (27)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
evaOS review status: completedPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Review URL: #1575 (review) |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9)
Head: b192abd23f75636e1e8f16f06c22ab136f392144 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +161/-1 | Unity/gameplay state | Moderate: validated P2 finding |
extensions/renderers/unity/scripts/build_room_unified.cs |
added | +237/-0 | Unity/gameplay state | Moderate: validated P3 finding |
qa/evidence/unify-frames/coherence_verdict.json |
added | +206/-0 | Changed file | Elevated: validated P1 finding |
qa/evidence/unify-frames/proof_inplayer_truth_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/unify-frames/research_synthesis.txt |
added | +193/-0 | Changed file | Low |
qa/evidence/unify-frames/room_boxes.json |
added | +43/-0 | Changed file | Low |
qa/evidence/unify-frames/room_greybox.png |
added | +0/-0 | Changed file | Low |
qa/evidence/unify-frames/room_greybox_depth.png |
added | +0/-0 | Changed file | Low |
qa/evidence/unify-frames/room_greybox_normal.png |
added | +0/-0 | Changed file | Low |
qa/evidence/unify-frames/unified_base_1.png |
added | +0/-0 | Changed file | Low |
Review Signal
Validated inline findings: 5 (P0: 0, P1: 1, P2: 3, P3: 1).
Dropped findings before posting: 3. High-severity findings: 1.
Risk Taxonomy
- Release regression: 3
- Runtime correctness: 1
- Security boundary: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #9, #1543, #1552, #1568, #1569.
Suggested labels: bug, unity.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/electricsheephq/WorldOS/blob/b192abd23f75636e1e8f16f06c22ab136f392144/qa/evidence/unify-frames/proof_inplayer_truth_overlay.png#L1
The repo's AGENTS.md visual-evidence rule caps committed frames at ≤400KB/frame, but this newly added proof frame is 1,261,582 bytes, and room_greybox.png / unified_base_1.png are also over the cap. Reviewers get the same evidence from a compressed JPEG or downsampled PNG, while this violates the artifact contract and bloats every checkout; re-encode before committing.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ate rebuild on swap, boxes/ packaging in EnsurePackaged, /shot POST+persistentDataPath, gated G-key + main-camera-only overlay, rooted-path rejection + TryGetValue + filtered catch, wood substring match, wall-run error cleanup, env-overridable geo path, dead sx2 removed
evaOS review status: completedPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Review URL: #1575 (review) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98ab46178e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| minX=Mathf.Min(minX,w.x); maxX=Mathf.Max(maxX,w.x); minZ=Mathf.Min(minZ,w.z); maxZ=Mathf.Max(maxZ,w.z); | ||
| rSum+=r; nC++; } | ||
| if(nC==0) continue; | ||
| if(kind=="wall_run"){ |
There was a problem hiding this comment.
When a room geometry uses kind: "perimeter_wall", which is already accepted by the existing greybox path (qa/greybox_render_headless.py's _WALL_RUN_KINDS) and covered by test_pre_existing_stone_wall_kind_is_not_treated_as_a_wall_run, this exact comparison falls through to the generic-prop path, leaves nWallRuns at zero, and the new unified renderer returns the no-wall-runs error instead of producing the sidecars. Use the same exact-kind alias set as the current renderer so valid perimeter-wall geometries can be rendered.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,206 @@ | |||
| [check_grid_paint_coherence] crypt_fresh: INCOHERENT (17 checked, worst offset 1.62 cell, threshold 0.5) — torch_door_l painted 1.19 cell off authored cells [[5, 1]] (> 0.5; 68px, NCC 0.240) — actor sits on the cell, paint sits elsewhere; torch_door_r painted 1.62 cell off authored cells [[7, 1]] (> 0.5; 93px, NCC 0.250) — actor sits on the cell, paint sits elsewhere; pilaster_arch painted 0.94 cell off authored cells [[12, 3]] (> 0.5; 54px, NCC 0.361) — actor sits on the cell, paint sits elsewhere; torch_near_l not locatable near authored cells [[1, 4]] (peak NCC 0.137 < 0.2) — paint put no grid-shaped structure on the footprint; rubble_bl painted 0.68 cell off authored cells [[1, 1], [1, 2]] (> 0.5; 39px, NCC 0.427) — actor sits on the cell, paint sits elsewhere; broken_slabs painted 1.50 cell off authored cells [[1, 6], [1, 7]] (> 0.5; 87px, NCC 0.647) — actor sits on the cell, paint sits elsewhere; wall_w_0 painted 1.26 cell off authored cells [[0, 1], [0, 2], [0, 3], [0, 4], [0, 5], [0, 6], [0, 7], [0, 8], [0, 9]] (> 0.5; 73px, NCC 0.630) — actor sits on the cell, paint sits elsewhere | |||
There was a problem hiding this comment.
This artifact is named .json, but the first line is the human summary emitted before the JSON object, so any evidence tooling that loads it as JSON fails immediately (for example, python3 -m json.tool qa/evidence/unify-frames/coherence_verdict.json errors at line 1). Put the summary in a separate .txt/log file or embed it as a JSON field so the recorded verdict remains machine-readable.
Useful? React with 👍 / 👎.
| // identical volumes the plate's depth conditioning was rendered from, so runtime masking and | ||
| // painted geometry cannot disagree (walls included, which the footprint path never covered — | ||
| // the playtest-#9 walk-through-wall class). Legacy footprint path below is untouched. | ||
| if (_plateBoxes != null && _plateBoxes.Count > 0) |
There was a problem hiding this comment.
When the player moves from a plate that loaded _plateBoxes into a location that has no manifest entry (the documented fallback where PlateSwapPending() is false and the current plate stands), this branch still gives the old sidecar priority over the new surface's legacy occluders because _plateBoxes is only cleared inside ApplyPlate(). In that unknown/stale-manifest room, actors are masked by the previous room's boxes instead of the current engine surface; clear or key the sidecar by the applied location before preferring it here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Walkthrough
PR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9)
Head: 98ab46178e49e035ef7343cdea27d2c4ab68b3bc into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +10/-0 | Unity/gameplay state | Moderate: runtime path |
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +180/-2 | Unity/gameplay state | Moderate: validated P2 finding |
extensions/renderers/unity/scripts/build_room_unified.cs |
added | +248/-0 | Unity/gameplay state | Moderate: validated P3 finding |
qa/evidence/unify-frames/coherence_verdict.json |
added | +206/-0 | Changed file | Moderate: validated P2 finding |
qa/evidence/unify-frames/proof_inplayer_truth_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/unify-frames/research_synthesis.txt |
added | +193/-0 | Changed file | Low |
qa/evidence/unify-frames/room_boxes.json |
added | +43/-0 | Changed file | Low |
qa/evidence/unify-frames/room_greybox.png |
added | +0/-0 | Changed file | Low |
qa/evidence/unify-frames/room_greybox_depth.png |
added | +0/-0 | Changed file | Low |
qa/evidence/unify-frames/room_greybox_normal.png |
added | +0/-0 | Changed file | Low |
qa/evidence/unify-frames/unified_base_1.png |
added | +0/-0 | Changed file | Low |
Review Signal
Validated inline findings: 5 (P0: 0, P1: 0, P2: 2, P3: 3).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- API compatibility: 1
- Flaky test risk: 1
- Runtime correctness: 2
- Security boundary: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #9, #1543, #1552, #1568, #1569.
Suggested labels: unity.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| @@ -0,0 +1,206 @@ | |||
| [check_grid_paint_coherence] crypt_fresh: INCOHERENT (17 checked, worst offset 1.62 cell, threshold 0.5) — torch_door_l painted 1.19 cell off authored cells [[5, 1]] (> 0.5; 68px, NCC 0.240) — actor sits on the cell, paint sits elsewhere; torch_door_r painted 1.62 cell off authored cells [[7, 1]] (> 0.5; 93px, NCC 0.250) — actor sits on the cell, paint sits elsewhere; pilaster_arch painted 0.94 cell off authored cells [[12, 3]] (> 0.5; 54px, NCC 0.361) — actor sits on the cell, paint sits elsewhere; torch_near_l not locatable near authored cells [[1, 4]] (peak NCC 0.137 < 0.2) — paint put no grid-shaped structure on the footprint; rubble_bl painted 0.68 cell off authored cells [[1, 1], [1, 2]] (> 0.5; 39px, NCC 0.427) — actor sits on the cell, paint sits elsewhere; broken_slabs painted 1.50 cell off authored cells [[1, 6], [1, 7]] (> 0.5; 87px, NCC 0.647) — actor sits on the cell, paint sits elsewhere; wall_w_0 painted 1.26 cell off authored cells [[0, 1], [0, 2], [0, 3], [0, 4], [0, 5], [0, 6], [0, 7], [0, 8], [0, 9]] (> 0.5; 73px, NCC 0.630) — actor sits on the cell, paint sits elsewhere | |||
| { | |||
| "passed": false, | |||
There was a problem hiding this comment.
P2: Shipped proof artifact reports coherence FAILED (7/17 props drift/unlocated, incl. wall_w_0 1.26 cells off)
The PR's thesis is 'UNIFY-THE-FRAMES ... so what masks an actor at runtime is byte-derived from what conditioned the paint' and 'runtime masking and painted geometry cannot disagree.' Yet the proof artifact committed in this PR (coherence_verdict.json) opens with 'crypt_fresh: INCOHERENT' and reports passed:false — torch_door_r 1.62 cells off, broken_slabs 1.50 cells off, wall_w_0 1.26 cells off (NCC 0.630), and torch_near_l UNLOCATED (NCC 0.137). For a PR whose central claim is paint/geometry coherence, shipping a failing coherence gate as the release evidence undercuts the claim and does not satisfy the repo's built-app proof bar. Either (a) regenerate the plate so the coherence gate passes before merge, (b) explicitly reclassify this evidence as a pre-fix baseline with a passing post-fix verdict in the same PR, or (c) justify why the known-bad paint is acceptable for this room. As committed, the evidence contradicts the PR title.
Category: API compatibility
Why this matters: The repo's RRI/release-readiness contract and AGENTS.md require honest proof. A green PR titled 'proof' containing a red coherence verdict is a proof gap that blocks any release-readiness claim derived from it.
| // identical volumes the plate's depth conditioning was rendered from, so runtime masking and | ||
| // painted geometry cannot disagree (walls included, which the footprint path never covered — | ||
| // the playtest-#9 walk-through-wall class). Legacy footprint path below is untouched. | ||
| if (_plateBoxes != null && _plateBoxes.Count > 0) |
There was a problem hiding this comment.
P2: Plate-box sidecar permanently shadows engine's live occluder set for plated rooms
In RebuildOccluders, once _plateBoxes is populated (ApplyPlate loads the boxes sidecar), the if (_plateBoxes != null && _plateBoxes.Count > 0) { ...; return; } branch unconditionally wins and the legacy _occRaw path is never reached again until the next ApplyPlate. But _occRaw is the engine's DYNAMIC per-surface occluder set, refreshed on every /combat-surface and /move poll that carries occluders (line 606-611, comment: 'the proxies stay live on the move path too'). For any plated location that ships a boxes sidecar, engine-driven occluder changes (opened doors, destroyed walls, spawned blockers, combat-surface-specific occluders) are now silently discarded in favor of the static sidecar geometry. OccSignature (line 680) does not encode _plateBoxes, so even when _occSigParsed changes and a rebuild fires, the new _occRaw is ignored. If the design intent is truly 'render conditioning == runtime occlusion' with no dynamic occluders for plated rooms, document that invariant explicitly and assert/merge rather than silently dropping _occRaw; otherwise this is a regression of the live-occluder path for plated rooms.
Category: Runtime correctness
Why this matters: A plated room whose engine surface reports occluders that differ from the static boxes (e.g. a combat surface adding blockers, or a door state change) will mask actors incorrectly — actors may render through volumes the engine now considers blocking, or be occluded by volumes the engine no longer blocks. This is exactly the paint/occlusion-disagree class the PR claims to eliminate, just shifted from paint-vs-boxes to engine-vs-boxes.
| if(stoneNrm!=null) UnityEngine.Object.DestroyImmediate(stoneNrm); | ||
| } | ||
| sb.AppendLine("unified greybox "+cols+"x"+rows+" ortho="+ortho.ToString("F4")+" wall_runs="+nWallRuns+" props="+np+" boxes="+boxRecords.Count+" (hidden="+hidden+")"); | ||
| return sb.ToString(); |
There was a problem hiding this comment.
P3: Success path leaves editor scene renderers disabled (only the error path re-enables them)
Near the top the tool disables every scene Renderer (r.enabled=false; hidden++) to render only GB_ boxes. The no-wall_run ERROR path correctly re-enables them (line ~'foreach(var r ...){ if(r!=null) r.enabled=true; }'). But the SUCCESS-path cleanup (final block) only destroys GB_ objects, their materials, and the stone textures — it never re-enables the originally-disabled scene renderers, nor restores cam.transform/ortho/aspect/nearClip/farClip/clearFlags or RenderSettings.ambientMode/ambientLight. Since the scene is explicitly 'NOT saved,' reopening restores state, so this is transient editor inconvenience rather than data loss — but it is an asymmetry between the two exit paths and an editor footgun (the scene goes visually blank until reload). Re-enable scene renderers + restore camera/render-settings in the success cleanup for parity with the error path.
Category: Runtime correctness
Why this matters: An authoring tool that silently disables all scene renderers on success can stall a room-authoring session and look like a crash/asset loss until the dev realizes a reload fixes it.
| // claim ships without a screenshot of this overlay agreeing with the paint. | ||
| // gated: QA channel enabled, env-armed, or a debug build — never a bare release toggle | ||
| // (evaos review on #1575: an unguarded G in a shipped player is a griefable debug surface). | ||
| if ((_qaClicks != null || Debug.isDebugBuild || System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null) |
There was a problem hiding this comment.
P3: G-key truth overlay reachable in any Development Build, not only QA env-armed runs
The toggle gate is (_qaClicks != null || Debug.isDebugBuild || WORLDOS_TRUTH_OVERLAY != null). Debug.isDebugBuild is true for every Development Build, which is typically the build config used for playtest/QA .app bundles (and the repo's dist/WorldOS.app playtest evidence). So any player-facing development build exposes an unauthenticated G-key that flips a full-screen debug overlay (cell walkable/impassable state + occluder wireframes). The inline comment claims 'never a bare release toggle,' but Development Build is not a release build. If development builds are ever distributed (playtests, TestFlight-equivalents), this is the griefable debug surface the comment says it avoids. Consider gating strictly on the QA channel + env var only (drop Debug.isDebugBuild), or documenting that dev builds are not player-distributed.
Category: Security boundary
Why this matters: A debug overlay revealing internal grid/impassable/door state is a mild griefing/cheat-assist surface if dev builds reach players; the code's own comment flags this as a concern it intended to close.
| // no desktop/SCK capture, no titlebar calibration, no privacy exposure of other windows. | ||
| // POST-only (parity with /click); writes under persistentDataPath, never a shared /tmp | ||
| // fixed path (symlink hardening — evaos review on #1575). | ||
| _qaShot = 2; resp = "{\"ok\":true,\"path\":\"" + _qaShotPath.Replace("\\", "/") + "\"}"; |
There was a problem hiding this comment.
P3: /shot returns a fixed capture path and the async capture can race/overwrite across concurrent requests
_qaShotPath is a single static file (persistentDataPath/wos_shot.png). ScreenCapture.CaptureScreenshot is asynchronous (writes across frames), and _qaShot is a single volatile counter reset to 2 by every /shot POST. Two rapid /shot requests reset the same counter and both capture into the same file; the caller polling that path cannot tell which frame it got and may read a partially-written PNG during a capture. The path is also interpolated raw into the JSON response (_qaShotPath.Replace('\','/')) — safe for macOS persistentDataPath today, but fragile if the path ever contains a quote/backslash. For a QA-only channel this is low impact, but consider a per-request unique filename (or a monotonic capture id echoed in the response) so the caller can distinguish captures and never read a half-written file.
Category: Flaky test risk
Why this matters: QA/proof screenshot capture that can silently return a torn or wrong-frame PNG undermines the reliability of the very evidence channel this PR adds.
… + the measured 1-cell-prop rule (evidence + FINDINGS)
…azier 0.8->1.2 + brighter) — crypt v3 measured ~0.7-cell drift + dropped pillars at the old widths
evaOS review status: stale headPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review stopped because this queued head is no longer the live PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Details: Superseded by a newer PR head. |
…he full chain (candidate + overlay evidence)
evaOS review status: completedPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Review URL: #1575 (review) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2f6c64e19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| string boxesSrcDir = Path.Combine(projectRoot, "boxes"); | ||
| if (Directory.Exists(boxesSrcDir)) | ||
| { | ||
| string boxesDstDir = Path.Combine(saDir, "boxes"); Directory.CreateDirectory(boxesDstDir); | ||
| foreach (var f in Directory.GetFiles(boxesSrcDir, "*.json")) File.Copy(f, Path.Combine(boxesDstDir, Path.GetFileName(f)), true); |
There was a problem hiding this comment.
In a clean build that follows this commit's new generator, build_room_unified.cs writes room_boxes.json under /home/unity/worldos-unity/Captures-Durable and the committed examples live under qa/evidence, but the packager only scans the Unity-project boxes/ directory here. Unless an operator manually copies/renames the generated sidecar into that directory and wires the manifest, ApplyPlate never loads _plateBoxes in dist/WorldOS.app, so the boxes-as-occluders path silently falls back to legacy footprint proxies.
Useful? React with 👍 / 👎.
| minX=Mathf.Min(minX,w.x); maxX=Mathf.Max(maxX,w.x); minZ=Mathf.Min(minZ,w.z); maxZ=Mathf.Max(maxZ,w.z); | ||
| rSum+=r; nC++; } | ||
| if(nC==0) continue; | ||
| if(kind=="wall_run"){ |
There was a problem hiding this comment.
For indoor geometry exports that include camera-near wall runs (the committed crypt_v33 geometry has wall_w_0/wall_s_0), treating every wall_run as a rendered and exported occluder reintroduces the -x/-z near walls that the canonical build_room_greybox.cs deliberately omits so the camera can see into the room. Since room_boxes.json now feeds directly into the runtime depth proxies, those front/left wall boxes can mask actors or door affordances on visible interior edge cells; skip/flag camera-near wall runs instead of exporting them as occluders.
Useful? React with 👍 / 👎.
| // no desktop/SCK capture, no titlebar calibration, no privacy exposure of other windows. | ||
| // POST-only (parity with /click); writes under persistentDataPath, never a shared /tmp | ||
| // fixed path (symlink hardening — evaos review on #1575). | ||
| _qaShot = 2; resp = "{\"ok\":true,\"path\":\"" + _qaShotPath.Replace("\\", "/") + "\"}"; |
There was a problem hiding this comment.
When a QA client posts /shot, this response returns ok with the fixed path before the main thread has decremented _qaShot and run ScreenCapture.CaptureScreenshot a later frame. If wos_shot.png already exists, evidence tooling that follows the returned path immediately can ingest the previous frame; if it doesn't, the file may simply be missing. Use a unique filename plus a completion poll, or block the response until the scheduled capture has actually been written.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Walkthrough
PR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9)
Head: b2f6c64e194286f7973b0ac4b24df60ce8aa0aca into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +10/-0 | Unity/gameplay state | Moderate: runtime path |
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +180/-2 | Unity/gameplay state | Elevated: validated P1 finding |
extensions/renderers/unity/scripts/build_room_unified.cs |
added | +248/-0 | Unity/gameplay state | Elevated: large change |
qa/evidence/crypt-v3/FINDINGS.md |
added | +39/-0 | Documentation | Low |
qa/evidence/crypt-v3/base_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v31.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v32.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v33.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v3_geometry.json |
added | +959/-0 | Changed file | Elevated: large change |
qa/evidence/crypt-v3/crypt_v3_styled.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v3_styled_1344.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v33.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/greybox_v30_rejected.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/greybox_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/styled_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/styled_v32_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/styled_v33_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/unify-frames/coherence_verdict.json |
added | +206/-0 | Changed file | Elevated: large change |
qa/evidence/unify-frames/proof_inplayer_truth_overlay.png |
added | +0/-0 | Changed file | Low |
6 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 5 (P0: 0, P1: 1, P2: 1, P3: 3).
Dropped findings before posting: 0. High-severity findings: 1.
Risk Taxonomy
- API compatibility: 1
- Runtime correctness: 4
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #9, #1543, #1552, #1568, #1569.
Suggested labels: bug, unity, docs.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| // gated: QA channel enabled, env-armed, or a debug build — never a bare release toggle | ||
| // (evaos review on #1575: an unguarded G in a shipped player is a griefable debug surface). | ||
| if ((_qaClicks != null || Debug.isDebugBuild || System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null) | ||
| && Input.GetKeyDown(KeyCode.G)) { _truthOverlay = !_truthOverlay; Debug.Log("[CSC] truth overlay " + (_truthOverlay ? "ON" : "OFF")); } |
There was a problem hiding this comment.
P1: Two KeyCode.G handlers in Update() — G now toggles two overlays at once
The new truth-overlay toggle at line 1961-1962 (Input.GetKeyDown(KeyCode.G) -> _truthOverlay = !_truthOverlay) is added at the TOP of Update(), but the pre-existing overlay toggle at line 1976 (if (Input.GetKeyDown(KeyCode.G)) ToggleOverlay(); for the walkable-overlay, #Phase3) is still present further down the same Update(). GetKeyDown returns true for the whole frame, so a single G press in a QA/debug build flips BOTH _truthOverlay AND _overlayOn every frame G is pressed. The two overlays are unrelated instruments (walk-cell overlay vs engine-truth occluder overlay) and the PR's own comments treat the truth overlay as a distinct 'is paint lying?' check, so silently co-toggling them defeats the purpose of each and is a gameplay-input regression. Fix: either route both through one key (e.g. the truth overlay on a different key such as 'T', or a modifier), or remove/consolidate the duplicate G binding. Add a focused play-mode test that asserts only one overlay state changes per G press.
Category: Runtime correctness
Why this matters: Pressing G is an established player/QA affordance for the walk overlay; this PR silently hijacks the same key so both overlays fire together, which will be observed as a regression by anyone using the existing overlay and will make the new truth overlay's screenshots ambiguous (two overlapping wireframe sets).
| // the playtest-#9 walk-through-wall class). Legacy footprint path below is untouched. | ||
| if (_plateBoxes != null && _plateBoxes.Count > 0) | ||
| { | ||
| int bn = 0; |
There was a problem hiding this comment.
P2: Unified occluder path bypasses all per-box validation (size/NaN/bounds) before becoming live geometry
In RebuildOccluders(), when _plateBoxes is non-empty each entry's six floats are applied directly as transform.position (b[0..2]) and transform.localScale (b[3..5]) with no validation: NaN, zero, negative, or absurdly large values from a malformed/tampered boxes sidecar become a live invisible cube at runtime (position NaN can poison the parent _occRoot transform and, via the depth-proxy material, break the actor masking the PR is meant to fix). The legacy footprint path uses authoring-derived fixed band heights; this new path trusts the JSON verbatim. Add minimal guards: skip entries where any component is NaN/Infinity or size <= 0, and warn on the count skipped (the load site at line ~3305 already filters kind==floor, but does not range-check the numbers).
Category: Runtime correctness
Why this matters: Occluder volumes are the collision-masking truth for actor visibility; a single bad row in a sidecar can make an entire room's actor occlusion silently wrong or spawn a NaN-scaled GameObject that corrupts the occluder root.
| public string boxesPath; } | ||
| // world-space occluder boxes of the ACTIVE plate ({center,size} rows, kind!=floor), null => legacy path. | ||
| System.Collections.Generic.List<float[]> _plateBoxes; | ||
| bool _truthOverlay = System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") == "1"; // G-key engine-truth overlay (playtest-#9 instrument); env=1 starts ON (QA/proof runs) |
There was a problem hiding this comment.
P3: Env gate for G-toggle uses != null but default uses == "1" — WORLDOS_TRUTH_OVERLAY=0 arms the toggle
The field initializer (_truthOverlay = ...GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") == "1") only starts the overlay ON for exactly "1". But the Update() gate at line 1961 uses System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null, so setting the var to "0" (an operator's natural 'disable' value) still satisfies the gate and arms the G-key toggle. Use the same predicate in both places (e.g. a cached _truthOverlayEnabled = env == "1" used by both the initializer and the G-gate) so intent and behavior agree.
Category: Runtime correctness
| if (_overlayMat != null) return _overlayMat; | ||
| var sh = Shader.Find("Hidden/Internal-Colored"); | ||
| if (sh == null) return null; | ||
| _overlayMat = new Material(sh) { hideFlags = HideFlags.HideAndDontSave }; |
There was a problem hiding this comment.
P3: _overlayMat (HideAndDontSave) is never destroyed
EnsureOverlayMaterial() creates a Material with HideFlags.HideAndDontSave and caches it in _overlayMat. The class's OnDestroy (line 2248) only calls StopQaInput(); there is no if (_overlayMat != null) Destroy(_overlayMat). HideAndDontSave materials are not GC'd and are not auto-destroyed on scene unload, so each CombatSurfaceClient instance (e.g. repeated domain reloads in-editor, or re-entering the surface) leaks one Material + its Shader reference until the editor/app restarts. Add destruction in OnDestroy mirroring the existing cleanup discipline.
Category: Runtime correctness
Why this matters: Material leaks in long-running editor/play sessions are a classic slow memory growth source for Unity surfaces; it is cheap to fix and matches the existing OnDestroy cleanup pattern.
| // no desktop/SCK capture, no titlebar calibration, no privacy exposure of other windows. | ||
| // POST-only (parity with /click); writes under persistentDataPath, never a shared /tmp | ||
| // fixed path (symlink hardening — evaos review on #1575). | ||
| _qaShot = 2; resp = "{\"ok\":true,\"path\":\"" + _qaShotPath.Replace("\\", "/") + "\"}"; |
There was a problem hiding this comment.
P3: /shot response path is not JSON-escaped
The /shot handler interpolates _qaShotPath.Replace("\\", "/") directly into the JSON response string. On Windows the backslash->slash swap avoids the obvious breakage, but the value is still not JSON-escaped (a path containing a double-quote or control char would emit invalid JSON). persistentDataPath is unlikely to contain those today, but the pattern is fragile vs. the JSON-escaped responses used elsewhere. Use JsonUtility.ToJson of a small struct (consistent with the rest of the codebase) instead of hand-concatenating.
Category: API compatibility
…e vocabulary, 2-cell rule, screen-space rhythm)
…/arches/curved lids/door frames) — 'not just squares' at the source; candidate + overlay evidence
evaOS review status: completedPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Review URL: #1575 (review) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33eb91a6e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (Directory.Exists(boxesSrcDir)) | ||
| { | ||
| string boxesDstDir = Path.Combine(saDir, "boxes"); Directory.CreateDirectory(boxesDstDir); | ||
| foreach (var f in Directory.GetFiles(boxesSrcDir, "*.json")) File.Copy(f, Path.Combine(boxesDstDir, Path.GetFileName(f)), true); |
There was a problem hiding this comment.
When a later build removes/regenerates a sidecar, or the Unity-project boxes/ directory is not synced, this only overlays current JSON files into Assets/StreamingAssets/boxes and never deletes the old contents. Because CombatSurfaceClient.ApplyPlate loads entry.boxesPath from StreamingAssets, the built app can keep using a stale room_boxes.json from a previous build instead of the current geometry or fallback, so actors get masked by the wrong room volumes even though packaging succeeds; clear the destination or validate/copy the manifest-referenced sidecars before building.
Useful? React with 👍 / 👎.
| System.IO.Directory.CreateDirectory("/home/unity/worldos-unity/Captures-Durable"); | ||
| System.IO.File.WriteAllBytes("/home/unity/worldos-unity/Captures-Durable/room_greybox.png", t2.EncodeToPNG()); |
There was a problem hiding this comment.
The script now allows WORLDOS_ROOM_GEO so a non-GEX44 host can read geometry from another checkout, but these outputs still always go to /home/unity/worldos-unity/Captures-Durable. On the Mac or any non-box Unity project where that path is absent or unwritable, the run can successfully read the overridden geometry and then fail or leave no local room_boxes.json/frames for packaging; derive the capture directory from an env var or the geometry path as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Walkthrough
PR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9)
Head: 33eb91a6e5eaca5e45a934d1ee1ac440f8051fa1 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +10/-0 | Unity/gameplay state | Moderate: runtime path |
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +180/-2 | Unity/gameplay state | Elevated: validated P1 finding |
extensions/renderers/unity/scripts/build_room_unified.cs |
added | +354/-0 | Unity/gameplay state | Elevated: large change |
qa/evidence/crypt-v3/FINDINGS.md |
added | +50/-0 | Documentation | Low |
qa/evidence/crypt-v3/base_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v31.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v32.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v33.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v34.json |
added | +70/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v34_MOLDED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v3_geometry.json |
added | +959/-0 | Changed file | Elevated: large change |
qa/evidence/crypt-v3/crypt_v3_styled.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v3_styled_1344.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v33.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v34.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/greybox_v30_rejected.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/greybox_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/greybox_v34.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/styled_v32.png |
added | +0/-0 | Changed file | Low |
11 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 5 (P0: 0, P1: 1, P2: 2, P3: 2).
Dropped findings before posting: 0. High-severity findings: 1.
Risk Taxonomy
- Release regression: 1
- Runtime correctness: 2
- Security boundary: 2
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #9, #1543, #1552, #1568, #1569.
Suggested labels: bug, unity, docs.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| // gated: QA channel enabled, env-armed, or a debug build — never a bare release toggle | ||
| // (evaos review on #1575: an unguarded G in a shipped player is a griefable debug surface). | ||
| if ((_qaClicks != null || Debug.isDebugBuild || System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null) | ||
| && Input.GetKeyDown(KeyCode.G)) { _truthOverlay = !_truthOverlay; Debug.Log("[CSC] truth overlay " + (_truthOverlay ? "ON" : "OFF")); } |
There was a problem hiding this comment.
P1: G key now double-bound: truth overlay and walk overlay both toggle on the same press
The new truth-overlay toggle (line 1962) and the pre-existing walkability-overlay toggle (line 1976: if (Input.GetKeyDown(KeyCode.G)) ToggleOverlay();) both fire in the same Update() frame on the same G keydown, both before the _busy gate. Unity's GetKeyDown returns true for the entire frame, so on every G press in any QA/playtest/debug build BOTH state flags flip simultaneously. The two overlays are mutually out-of-phase: pressing G to turn the new truth overlay ON simultaneously turns the established walk grid OFF, and the next G reverses both. The walkability overlay's G-toggle is documented normal-play/QA behavior (lines 454-461, ToggleOverlay at 1139), so this collides with shipped behavior, not an unused path. Fix: bind the truth overlay to a different key, or gate ToggleOverlay off when the truth-overlay gating condition is active.
Category: Runtime correctness
Why this matters: QA and playtest builds (Debug.isDebugBuild, or WORLDOS_QA_INPUT=1) hit this every session. Both overlays share the same key and both flip on one press, so neither overlay can be operated independently — the operator's primary 'is paint lying?' instrument and the primary walkability tool corrupt each other.
| @@ -0,0 +1,206 @@ | |||
| [check_grid_paint_coherence] crypt_fresh: INCOHERENT (17 checked, worst offset 1.62 cell, threshold 0.5) — torch_door_l painted 1.19 cell off authored cells [[5, 1]] (> 0.5; 68px, NCC 0.240) — actor sits on the cell, paint sits elsewhere; torch_door_r painted 1.62 cell off authored cells [[7, 1]] (> 0.5; 93px, NCC 0.250) — actor sits on the cell, paint sits elsewhere; pilaster_arch painted 0.94 cell off authored cells [[12, 3]] (> 0.5; 54px, NCC 0.361) — actor sits on the cell, paint sits elsewhere; torch_near_l not locatable near authored cells [[1, 4]] (peak NCC 0.137 < 0.2) — paint put no grid-shaped structure on the footprint; rubble_bl painted 0.68 cell off authored cells [[1, 1], [1, 2]] (> 0.5; 39px, NCC 0.427) — actor sits on the cell, paint sits elsewhere; broken_slabs painted 1.50 cell off authored cells [[1, 6], [1, 7]] (> 0.5; 87px, NCC 0.647) — actor sits on the cell, paint sits elsewhere; wall_w_0 painted 1.26 cell off authored cells [[0, 1], [0, 2], [0, 3], [0, 4], [0, 5], [0, 6], [0, 7], [0, 8], [0, 9]] (> 0.5; 73px, NCC 0.630) — actor sits on the cell, paint sits elsewhere | |||
| { | |||
| "passed": false, | |||
There was a problem hiding this comment.
P2: Shipped coherence gate returns passed:false (7/17 props drift) while PR title claims 'proof'
The committed verdict is "passed": false, worst offset 1.62 cells, with torch_door_l/torch_door_r DRIFT (1.19/1.62 cells), pilaster_arch DRIFT, torch_near_l UNLOCATED (NCC 0.137 < 0.2), rubble_bl/broken_slabs DRIFT, and wall_w_0 DRIFT (1.26 cells). qa/evidence/crypt-v3/FINDINGS.md:50 explicitly lists 'in-player truth overlay' as a NEXT GATE (unmet). The PR title frames this as 'UNIFY-THE-FRAMES proof' and 'playtest #9'. Before any merge/release language, either update the title/evidence framing to 'candidate/instrument + failing gate', or regenerate the verdict against a room that passes its own 0.5-cell threshold. Do not let a passed:false artifact stand as the named proof.
Category: Release regression
Why this matters: Release-readiness claims anchored to a failing coherence verdict are exactly the harness-contaminated/partial-evidence pattern the repo gates against. A future reviewer reading 'proof' + this JSON will conclude the alignment problem is solved when the shipped gate says it is not.
| [System.Serializable] class QaClick { public int c = -1; public int r = -1; public float vx = float.NaN; public float vy = float.NaN; } | ||
| struct QaCmd { public bool cell; public int c; public int r; public float vx; public float vy; } | ||
| volatile int _qaShot; // /shot countdown -> main-thread framebuffer capture | ||
| static readonly string _qaShotPath = System.IO.Path.Combine(Application.persistentDataPath, "wos_shot.png"); |
There was a problem hiding this comment.
P2: static readonly _qaShotPath calls Application.persistentDataPath in a static field initializer
static readonly string _qaShotPath = System.IO.Path.Combine(Application.persistentDataPath, "wos_shot.png"); invokes a Unity API during type/field initialization rather than lazily in Start/Awake. Unity requires Application.* on the main thread; static initializers run whenever the type is first touched, which is not guaranteed to be main-threaded or after engine init. Resolve the path lazily (cache it on first /shot or in Start) like the existing main-thread-only Screen.* fields are cached, or assign it inside StartQaInput().
Category: Runtime correctness
Why this matters: A static initializer hitting a Unity API before the main thread/engine is ready can throw or yield an empty path on some players; for a /shot capture that would silently write nowhere or hard-fault on first request.
| // claim ships without a screenshot of this overlay agreeing with the paint. | ||
| // gated: QA channel enabled, env-armed, or a debug build — never a bare release toggle | ||
| // (evaos review on #1575: an unguarded G in a shipped player is a griefable debug surface). | ||
| if ((_qaClicks != null || Debug.isDebugBuild || System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null) |
There was a problem hiding this comment.
P3: WORLDOS_TRUTH_OVERLAY gating uses != null, so =0 still arms the G-toggle
The G-toggle guard at line 1961 is System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null, which matches ANY value including "0". But the field init at line 328 uses == "1". Setting WORLDOS_TRUTH_OVERLAY=0 (operator intent: disabled) leaves the overlay starting OFF yet makes the G key live in a non-debug release build, contradicting the =0 intent. Align the guard to == "1" (or treat the env var consistently with the field init) so the toggle is only armed when the overlay is explicitly requested.
Category: Security boundary
Why this matters: A debug surface (truth overlay) becomes key-activatable in a shipped player whenever the env var is set to anything other than unset — a small but real griefable surface the PR's own comments say it wants to avoid.
| { | ||
| try | ||
| { | ||
| string bpath = System.IO.Path.Combine(Application.streamingAssetsPath, entry.boxesPath); |
There was a problem hiding this comment.
P3: boxes sidecar path not sandboxed against ../ traversal (read-only, localhost QA only)
ApplyPlate loads the box sidecar via System.IO.Path.Combine(Application.streamingAssetsPath, entry.boxesPath) after only an IsPathRooted check. A relative path like ../../etc/something passes IsPathRooted==false and Combine yields a path outside StreamingAssets. It is read-only (File.Exists + ReadAllText -> Json.Parse) and the channel is localhost-only (127.0.0.1, env-gated WORLDOS_QA_INPUT=1), so blast radius is low, but for parity with the rooted-path rejection at line 3317 consider also rejecting paths containing '..' or canonicalizing+confirming the result is under StreamingAssets.
Category: Security boundary
Why this matters: Even read-only traversal on a localhost debug surface is worth closing so a malformed manifest can't read arbitrary files; the rooted check already signals the author wanted a sandbox here.
… builder; 16x12 spaced geometry; PROCEDURAL-SCORECARD.md — the generate/grade/refine loop
evaOS review status: completedPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Review URL: #1575 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9)
Head: 143e58419b9d083aa65606c9bea2e7aac8dc6cb2 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +10/-0 | Unity/gameplay state | Moderate: runtime path |
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +180/-2 | Unity/gameplay state | Elevated: validated P1 finding |
extensions/renderers/unity/scripts/build_room_unified.cs |
added | +358/-0 | Unity/gameplay state | Moderate: validated P2 finding |
qa/evidence/crypt-v3/FINDINGS.md |
added | +58/-0 | Documentation | Low |
qa/evidence/crypt-v3/base_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v31.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v32.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v33.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v34.json |
added | +70/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v35.json |
added | +73/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v34_MOLDED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_SPACED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v3_geometry.json |
added | +959/-0 | Changed file | Elevated: large change |
qa/evidence/crypt-v3/crypt_v3_styled.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v3_styled_1344.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v33.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v34.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v35.png |
added | +0/-0 | Changed file | Low |
16 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 5 (P0: 0, P1: 1, P2: 2, P3: 2).
Dropped findings before posting: 1. High-severity findings: 1.
Risk Taxonomy
- Release regression: 1
- Runtime correctness: 2
- Security boundary: 1
- Unity scene/prefab: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #9, #1543, #1552, #1568, #1569.
Suggested labels: bug, unity, docs.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| // identical volumes the plate's depth conditioning was rendered from, so runtime masking and | ||
| // painted geometry cannot disagree (walls included, which the footprint path never covered — | ||
| // the playtest-#9 walk-through-wall class). Legacy footprint path below is untouched. | ||
| if (_plateBoxes != null && _plateBoxes.Count > 0) |
There was a problem hiding this comment.
P1: Plate-box occluders go stale and shadow live engine occluder changes
_plateBoxes is populated only inside ApplyPlate (line 3281/3294) and is never cleared on a non-swap surface update. RebuildOccluders unconditionally takes the _plateBoxes branch (if (_plateBoxes != null && _plateBoxes.Count > 0) at line 721) and returns, so once a plate sidecar is loaded, EVERY later occluder-set change the engine sends for the SAME location (a /move or /combat-surface poll that mutates _occRaw and bumps _occSigParsed != _occSigBuilt) is rebuilt from the OLD plate boxes — the engine's new occluder truth (_occRaw) is ignored entirely. Occluder proxies are supposed to mask actors consistently with the painted geometry; instead they are frozen for the lifetime of the plate. This is exactly the paint-vs-collision divergence the feature claims to eliminate. Fix: invalidate _plateBoxes to null whenever the surface's own occluder signature changes (clear it at the top of the _occRaw-driven path, or gate the box branch on the occluder signature matching the location the boxes were loaded for).
Category: Runtime correctness
Why this matters: Depth-proxy occluders silently decouple from engine truth after the first plate swap; an actor can be masked by stale wall volumes that no longer match where the engine says it can walk — a walk-through-wall / visual-occlusion regression that only manifests in rooms with a boxes sidecar.
| System.IO.File.WriteAllBytes("/home/unity/worldos-unity/Captures-Durable/room_greybox.png", t2.EncodeToPNG()); | ||
| System.Action<string,string> _capPass=(shaderName,outName)=>{ | ||
| var sh=Shader.Find(shaderName); if(sh==null){ sb.AppendLine("MISSING shader "+shaderName); return; } | ||
| var _prt=cam.targetTexture; var _pa=cam.aspect; cam.targetTexture=rt; cam.aspect=(float)W/Hh; |
There was a problem hiding this comment.
P2: Error path restores objects/lights but leaves the main camera mutated
On the nWallRuns==0 error return (and on success too, but there the render is the point), the script mutates Camera.main (orthographic=true, orthographicSize, nearClip/farClip, transform.rotation to Euler(30,45,0), position pulled back -forward80, clearFlags=SolidColor, backgroundColor) and never restores any of it. The cleanup block only destroys GB_ GameObjects, re-enables hidden Renderers, and destroys the stone textures. After an error the operator is left with a camera rig that no longer matches their scene and no rendered output to show for it. Capture and restore cam.{orthographic,orthographicSize,nearClipPlane,farClipPlane,transform.{position,rotation},clearFlags,backgroundColor} before mutating, and restore them on every return path (success + both error returns).
Category: Unity scene/prefab
Why this matters: A dev/GEX44-only editor tool, but a failed run silently corrupts the scene camera state, which is easy to miss and hard to attribute to this script later.
| [System.Serializable] class QaClick { public int c = -1; public int r = -1; public float vx = float.NaN; public float vy = float.NaN; } | ||
| struct QaCmd { public bool cell; public int c; public int r; public float vx; public float vy; } | ||
| volatile int _qaShot; // /shot countdown -> main-thread framebuffer capture | ||
| static readonly string _qaShotPath = System.IO.Path.Combine(Application.persistentDataPath, "wos_shot.png"); |
There was a problem hiding this comment.
P2: static field initializer reads Application.persistentDataPath (fragile at type-init on built players)
static readonly string _qaShotPath = System.IO.Path.Combine(Application.persistentDataPath, "wos_shot.png"); runs during type initialization. This is the ONLY static readonly in the file that touches a Unity API; every other Unity-API read in CombatSurfaceClient happens inside instance lifecycle methods (Awake/Start/Update). static init ordering under IL2CPP / domain-reload / player startup is not guaranteed to be in a state where Application APIs are safe, and a throw here surfaces as TypeInitializationException that fails the whole MonoBehaviour (and the scene). Read the path lazily (an instance property or in Awake, matching the rest of the file) rather than eagerly in a static initializer.
Category: Release regression
Why this matters: A static-init failure on a release build would break the combat surface (the live scene root) with a hard-to-diagnose TypeInitializationException rather than a clean null path.
| // gated: QA channel enabled, env-armed, or a debug build — never a bare release toggle | ||
| // (evaos review on #1575: an unguarded G in a shipped player is a griefable debug surface). | ||
| if ((_qaClicks != null || Debug.isDebugBuild || System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null) | ||
| && Input.GetKeyDown(KeyCode.G)) { _truthOverlay = !_truthOverlay; Debug.Log("[CSC] truth overlay " + (_truthOverlay ? "ON" : "OFF")); } |
There was a problem hiding this comment.
P3: Two independent G-key toggles now fire on the same keypress
Update() now binds G twice: the new truth-overlay toggle (line 1961-1962, gated by _qaClicks!=null || Debug.isDebugBuild || WORLDOS_TRUTH_OVERLAY!=null) and the pre-existing walk-overlay ToggleOverlay() call (line 1976, UNGATED, fires in every build). Pressing G in a release build flips _overlayOn (the walk overlay) every time, and additionally flips _truthOverlay only when the gate holds. The new toggle's release-build gate is correct, but the dual binding is surprising UX and means G is not solely the new instrument's key. If the truth overlay is meant to be the sole new behavior on G, route both through one handler, or pick a different key for the new overlay.
Category: Runtime correctness
Why this matters: Not a release gate (the debug overlay is correctly gated), but the key collision will confuse QA/operators toggling two overlays at once.
| // no desktop/SCK capture, no titlebar calibration, no privacy exposure of other windows. | ||
| // POST-only (parity with /click); writes under persistentDataPath, never a shared /tmp | ||
| // fixed path (symlink hardening — evaos review on #1575). | ||
| _qaShot = 2; resp = "{\"ok\":true,\"path\":\"" + _qaShotPath.Replace("\\", "/") + "\"}"; |
There was a problem hiding this comment.
P3: /shot response JSON-interpolates the capture path unescaped and returns it before the file exists
resp = "{\"ok\":true,\"path\":\"" + _qaShotPath.Replace("\\", "/") + "\"}"; builds JSON by string concatenation; any character needing JSON escaping (a quote or non-ascii in persistentDataPath on some locales) breaks the response. ScreenCapture.CaptureScreenshot is asynchronous and fires ~2 frames later (_qaShot = 2 countdown in Update), so a caller that POSTs /shot and immediately reads the returned path races the file. Localhost-only QA surface so impact is low, but either use JsonUtility to serialize, or document that callers must poll for the file. The existing /debug endpoint already has the same unescaped-interpolation pattern (via _dbgLast/_foeId) — consider fixing both together.
Category: Security boundary
Why this matters: Low severity (loopback QA channel, path not attacker-controlled), but it's newly added and inherits a latent injection pattern that should not spread further.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 143e58419b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,206 @@ | |||
| [check_grid_paint_coherence] crypt_fresh: INCOHERENT (17 checked, worst offset 1.62 cell, threshold 0.5) — torch_door_l painted 1.19 cell off authored cells [[5, 1]] (> 0.5; 68px, NCC 0.240) — actor sits on the cell, paint sits elsewhere; torch_door_r painted 1.62 cell off authored cells [[7, 1]] (> 0.5; 93px, NCC 0.250) — actor sits on the cell, paint sits elsewhere; pilaster_arch painted 0.94 cell off authored cells [[12, 3]] (> 0.5; 54px, NCC 0.361) — actor sits on the cell, paint sits elsewhere; torch_near_l not locatable near authored cells [[1, 4]] (peak NCC 0.137 < 0.2) — paint put no grid-shaped structure on the footprint; rubble_bl painted 0.68 cell off authored cells [[1, 1], [1, 2]] (> 0.5; 39px, NCC 0.427) — actor sits on the cell, paint sits elsewhere; broken_slabs painted 1.50 cell off authored cells [[1, 6], [1, 7]] (> 0.5; 87px, NCC 0.647) — actor sits on the cell, paint sits elsewhere; wall_w_0 painted 1.26 cell off authored cells [[0, 1], [0, 2], [0, 3], [0, 4], [0, 5], [0, 6], [0, 7], [0, 8], [0, 9]] (> 0.5; 73px, NCC 0.630) — actor sits on the cell, paint sits elsewhere | |||
There was a problem hiding this comment.
This file has a console summary prepended before the JSON object, so json.load(...) fails at line 1. When follow-up evidence automation or scorecard tooling loads qa/evidence/unify-frames/coherence_verdict.json to read passed, props, or offsets, the evidence row becomes unusable; keep the human summary in a separate .txt/.md file or inside a JSON field.
Useful? React with 👍 / 👎.
| { var _k=new System.Collections.Generic.List<GameObject>(); foreach(var g in UnityEngine.Object.FindObjectsByType<GameObject>(FindObjectsSortMode.None)){ if(g==null) continue; if(g.name.StartsWith("GB_") || g.GetComponent<Light>()!=null) _k.Add(g); } foreach(var g in _k){ if(g!=null) UnityEngine.Object.DestroyImmediate(g); } } | ||
| int hidden=0; foreach(var r in UnityEngine.Object.FindObjectsByType<Renderer>(FindObjectsSortMode.None)){ if(r.enabled){r.enabled=false;hidden++;} } |
There was a problem hiding this comment.
When this editor script is run in an already-open Unity scene, it destroys all existing lights and disables every existing renderer before adding the transient greybox; the success cleanup only removes GB_* objects/materials, so subsequent captures or packaging in the same editor session see a blank/unlit scene unless the operator remembers to reload. The error path already re-enables renderers, so the normal path should similarly restore the objects it mutates.
Useful? React with 👍 / 👎.
| if (_plateBoxes != null && _plateBoxes.Count > 0) | ||
| { |
There was a problem hiding this comment.
After a plate with a boxes sidecar is applied, _plateBoxes remains populated until another successful ApplyPlate; if the next surface location has no manifest entry, PlateSwapPending() is false and this branch still rebuilds occluders from the previous room's sidecar instead of the new surface's _occRaw. In journeys through generated or otherwise unmapped locations, the old room's invisible volumes can mask actors in the new room, so the sidecar needs to be keyed to _locId or cleared when no swap will occur.
Useful? React with 👍 / 👎.
| if(nWallRuns==0){ | ||
| // cleanup before erroring: this run already hid scene renderers + destroyed lights + spawned GB_ parts — | ||
| // leaving them makes the ERROR path corrupt the editor scene state (codex review, #1575). | ||
| foreach(var g in UnityEngine.Object.FindObjectsByType<GameObject>(FindObjectsSortMode.None)){ if(g!=null && g.name.StartsWith("GB_")) UnityEngine.Object.DestroyImmediate(g); } | ||
| foreach(var r in UnityEngine.Object.FindObjectsByType<Renderer>(FindObjectsSortMode.None)){ if(r!=null) r.enabled=true; } | ||
| if(stoneAlb!=null) UnityEngine.Object.DestroyImmediate(stoneAlb); | ||
| if(stoneNrm!=null) UnityEngine.Object.DestroyImmediate(stoneNrm); | ||
| sb.AppendLine("ERROR: geometry has no wall_run props — author under the extent contract (#1543) first (scene restored)."); | ||
| return sb.ToString(); |
There was a problem hiding this comment.
This hard error rejects every geometry without a wall_run, but open-air rooms are valid in this repo: tools/author_room_geometry.py::author_camp builds the camp with perimeter=False and no wall-run props. Running the new unified pipeline for that camp regeneration will abort before producing the greybox/depth/boxes, so absence of wall runs should mean a props-only/open-air room rather than an error.
Useful? React with 👍 / 👎.
…ziers+torch; SW-corner drift 1.6c + urn kind-drift recorded; scorecard axis 5 = 8
evaOS review status: completedPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Review URL: #1575 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9)
Head: d27b69dcf511c879f81e38470861be1e6ddfd22c into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +10/-0 | Unity/gameplay state | Moderate: runtime path |
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +180/-2 | Unity/gameplay state | Moderate: validated P3 finding |
extensions/renderers/unity/scripts/build_room_unified.cs |
added | +358/-0 | Unity/gameplay state | Moderate: validated P2 finding |
qa/evidence/crypt-v3/FINDINGS.md |
added | +70/-0 | Documentation | Low |
qa/evidence/crypt-v3/base_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v31.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v32.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v33.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v34.json |
added | +70/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v35.json |
added | +73/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v34_MOLDED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_SPACED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v3_geometry.json |
added | +959/-0 | Changed file | Elevated: large change |
qa/evidence/crypt-v3/crypt_v3_styled.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v3_styled_1344.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v33.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/depth_v34.png |
added | +0/-0 | Changed file | Low |
17 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 3 (P0: 0, P1: 0, P2: 1, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Runtime correctness: 1
- Security boundary: 1
- Unity scene/prefab: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #9, #1543, #1552, #1568, #1569.
Suggested labels: unity, docs.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| { var _gb=new System.Collections.Generic.List<GameObject>(); var _mat=new System.Collections.Generic.HashSet<Material>(); | ||
| foreach(var g in UnityEngine.Object.FindObjectsByType<GameObject>(FindObjectsSortMode.None)){ if(g==null) continue; if(g.name.StartsWith("GB_")){ _gb.Add(g); var _r=g.GetComponent<Renderer>(); if(_r!=null && _r.sharedMaterial!=null) _mat.Add(_r.sharedMaterial); } } | ||
| foreach(var g in _gb){ if(g!=null) UnityEngine.Object.DestroyImmediate(g); } | ||
| foreach(var m in _mat){ if(m!=null) UnityEngine.Object.DestroyImmediate(m); } |
There was a problem hiding this comment.
P2: Editor success-path never re-enables scene Renderers it disabled
Before rendering, the script hides EVERY scene Renderer (foreach(var r in FindObjectsByType<Renderer>...){ r.enabled=false; hidden++; }). The ERROR path correctly restores them (foreach(var r ...) r.enabled=true;), which proves restoration is intended. But the SUCCESS-path cleanup block at the end of the script only destroys GB_ objects + their materials + the stone textures — it never sets r.enabled=true back on the scene renderers it disabled. After a successful unity-mcp code execute -f build_room_unified.cs, the operator's scene is left with all its authored renderers disabled (invisible in the editor / next play mode), requiring manual re-enabling or reverting the scene. Add the same foreach(var r in FindObjectsByType<Renderer>(FindObjectsSortMode.None)){ if(r!=null) r.enabled=true; } restoration to the success-path cleanup block. This is editor-only (not compiled into the shipped player via BuildMacOSPlayer.cs), so it is not a release regression, but it is a real scene-state corruption for anyone running the tool.
Category: Unity scene/prefab
Why this matters: A tool that silently disables every renderer in the scene on its happy path will bite the next operator who runs it and then switches back to authoring — the scene looks empty/broken until each renderer is hand-re-enabled. The asymmetry between the error path (restores) and success path (doesn't) shows this is an omission, not a design choice.
| { | ||
| // UNIFY-THE-FRAMES truth overlay (playtest-#9 instrument): G toggles engine-truth rendering over | ||
| // the plate — every cell's floor diamond colored by its live state (walkable/impassable/door) + | ||
| // wireframes of the active occluder volumes. The permanent "is paint lying?" check: no alignment |
There was a problem hiding this comment.
P3: G-key toggle arms on WORLDOS_TRUTH_OVERLAY presence regardless of value
The toggle guard uses System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null to arm the G key, but the overlay's own initializer (line 328) uses == "1". So setting WORLDOS_TRUTH_OVERLAY=0 (intent: explicitly off) still ARMS the G-key toggle in a release build, while leaving the overlay off — surprising and inconsistent with the 'opt-in' framing. Use the same == "1" predicate (or compare against "0") for the toggle-arming condition so the env var has one unambiguous meaning across initializer and toggle.
Category: Runtime correctness
Why this matters: Inconsistent env-var semantics are a classic source of 'why is this debug surface active in my build' confusion. Harmless today (overlay still defaults off), but the asymmetry will mislead future operators reading either site in isolation.
| // swap; RebuildOccluders prefers these over per-cell footprint proxies. Reset the occluder signature | ||
| // so the next ApplyJson rebuild fires even when the surface's occluder set is unchanged. | ||
| _plateBoxes = null; | ||
| if (!string.IsNullOrEmpty(entry.boxesPath) && !System.IO.Path.IsPathRooted(entry.boxesPath)) |
There was a problem hiding this comment.
P3: Relative '..' in boxesPath can escape StreamingAssets (matches existing plate path, so not a new regression)
The !System.IO.Path.IsPathRooted(entry.boxesPath) guard rejects absolute/Windows-rooted paths but not relative traversal like ../../etc/passwd. Path.Combine(streamingAssetsPath, "../../x") resolves outside StreamingAssets and File.Exists/ReadAllText will read it. This is consistent with the existing entry.plate handling (line 3242, which has NO rooted check at all), and the manifest is a packaged trusted asset rather than network/user input, so it does not lower the current threat model. Flagging only so it is a conscious decision: if plates_manifest.json is ever fed from a non-trusted source, both plate and boxes are traversal-reachable. No change required to match current behavior.
Category: Security boundary
Why this matters: Documenting the boundary so a future change to the manifest's trust source doesn't silently widen an attack surface that two code paths now share.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d27b69dcf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| boxRecords.Add("{\"name\":\""+nm+"\",\"kind\":\""+kindTag+"\",\"center\":[" | ||
| +center.x.ToString("F3")+","+center.y.ToString("F3")+","+center.z.ToString("F3")+"],\"size\":[" | ||
| +size.x.ToString("F3")+","+size.y.ToString("F3")+","+size.z.ToString("F3")+"]}"); |
There was a problem hiding this comment.
When this Unity editor script runs under a locale that formats decimals with commas, these ToString("F3") calls emit values like 1,200 into room_boxes.json instead of JSON numbers with . separators. The PNG captures can still be produced, but the packaged player then cannot reliably parse the sidecar (or reads corrupted arrays), so _plateBoxes falls back/misplaces the unified occluders on non-invariant-culture hosts; write these values via CultureInfo.InvariantCulture or a JSON serializer.
Useful? React with 👍 / 👎.
…ue-mass rule (1.33h top = 0 depth delta) + tightened depth remap; honest-negative base registration row + next-cycle levers (best-of-3 edge-recall selection per the promoted recipe)
evaOS review status: stale headPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review stopped because this queued head is no longer the live PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Details: live=f05b8c1a97f9bbb14cfae21e3a2b8353f48f65bb |
…le log + flat-interior defect class
…— ITERATE; wins composition/readability 7.2>6.5; apron-vignette + light-clustering defect classes codified
… removed); flux single-shot style collapse = honest negative that makes best-of-N selection UNIVERSAL (depth-rich exemption falsified same-day)
evaOS review status: completedPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Review URL: #1575 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9)
Head: fc750c473070305515defe266cbf8c46a86c49fd into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +10/-0 | Unity/gameplay state | Moderate: runtime path |
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +180/-2 | Unity/gameplay state | Moderate: validated P2 finding |
extensions/renderers/unity/scripts/build_room_unified.cs |
added | +381/-0 | Unity/gameplay state | Elevated: large change |
qa/evidence/crypt-v3/FINDINGS.md |
added | +92/-0 | Documentation | Low |
qa/evidence/crypt-v3/base_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v36.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v31.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v32.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v33.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v34.json |
added | +70/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v35.json |
added | +73/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v36.json |
added | +71/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v34_MOLDED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_SPACED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_blind_panel.json |
added | +21/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v36_geometry.json |
added | +1069/-0 | Changed file | Elevated: large change |
qa/evidence/crypt-v3/crypt_v3_geometry.json |
added | +959/-0 | Changed file | Elevated: large change |
qa/evidence/crypt-v3/crypt_v3_styled.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v3_styled_1344.png |
added | +0/-0 | Changed file | Low |
32 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 3 (P0: 0, P1: 0, P2: 1, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Runtime correctness: 2
- Security boundary: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #9, #1543, #1552, #1568, #1569.
Suggested labels: unity, docs.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| // gated: QA channel enabled, env-armed, or a debug build — never a bare release toggle | ||
| // (evaos review on #1575: an unguarded G in a shipped player is a griefable debug surface). | ||
| if ((_qaClicks != null || Debug.isDebugBuild || System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null) | ||
| && Input.GetKeyDown(KeyCode.G)) { _truthOverlay = !_truthOverlay; Debug.Log("[CSC] truth overlay " + (_truthOverlay ? "ON" : "OFF")); } |
There was a problem hiding this comment.
P2: G key is bound to two overlays simultaneously — both fire on one press
Update() now contains two independent Input.GetKeyDown(KeyCode.G) handlers. The new one at line 1961 toggles _truthOverlay (when its gate is satisfied); the pre-existing one at line 1976 calls ToggleOverlay() UNCONDITIONALLY on every build (it is not gated). Input.GetKeyDown returns true for the entire frame, so in any context where the truth-overlay gate passes (WORLDOS_QA_INPUT=1, Debug.isDebugBuild, or WORLDOS_TRUTH_OVERLAY set) a single G press flips BOTH _truthOverlay and _overlayOn on the same frame. The line-1975 comment ('#Phase3: overlay toggle (G)') shows the binding was already claimed by the walkable overlay. The proof/QA workflow (where the truth overlay is the whole point of #1575) will have both overlays fighting on the same key. Give the truth overlay a distinct key (e.g. backtick or T) or guard line 1976 with the same inverted condition so only one toggles per press.
Category: Runtime correctness
Why this matters: The truth overlay is the playtest-#9 instrument this PR ships; if G toggles it AND the walkable grid overlay together, the 'is paint lying?' screenshot will be contaminated by the unrelated walkable overlay and the instrument becomes ambiguous exactly when it is needed.
| // claim ships without a screenshot of this overlay agreeing with the paint. | ||
| // gated: QA channel enabled, env-armed, or a debug build — never a bare release toggle | ||
| // (evaos review on #1575: an unguarded G in a shipped player is a griefable debug surface). | ||
| if ((_qaClicks != null || Debug.isDebugBuild || System.Environment.GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null) |
There was a problem hiding this comment.
P3: Truth-overlay toggle gate accepts any WORLDOS_TRUTH_OVERLAY value, not only "1"
The runtime G-toggle gate is GetEnvironmentVariable("WORLDOS_TRUTH_OVERLAY") != null, which is true for ANY present value — including "0" or the empty string. But the field initializer at line 328 (_truthOverlay = ... == "1") and the overlay-on intent both treat "1" as the sole on-signal. A user who sets WORLDOS_TRUTH_OVERLAY=0 intending to force the overlay off will instead ARM the G-key toggle (the overlay starts off but G then turns it on). The PR's own comment frames this as 'never a bare release toggle / a griefable debug surface', so the gate should match the initializer: compare == "1" (or at least != null only inside Debug.isDebugBuild/_qaClicks branches). The localhost QA listener path is fine; the concern is the standalone env-var branch in a shipped player.
Category: Security boundary
Why this matters: An env-var value of '0' silently doing the opposite of its intent is a classic footgun; the initializer and the toggle disagree, so the overlay's on/off semantics depend on which code path armed it.
| // no desktop/SCK capture, no titlebar calibration, no privacy exposure of other windows. | ||
| // POST-only (parity with /click); writes under persistentDataPath, never a shared /tmp | ||
| // fixed path (symlink hardening — evaos review on #1575). | ||
| _qaShot = 2; resp = "{\"ok\":true,\"path\":\"" + _qaShotPath.Replace("\\", "/") + "\"}"; |
There was a problem hiding this comment.
P3: /shot response JSON built by raw string concatenation of a filesystem path
The /shot response is assembled as "{"ok":true,"path":"" + _qaShotPath.Replace('\','/') + ""}". _qaShotPath is Application.persistentDataPath + "/wos_shot.png". On macOS persistentDataPath will not contain a double-quote, so this is safe in practice today, but it is fragile: any future platform/path containing a " or backslash not caught by Replace produces invalid JSON and the QA harness gets an unparseable response with no error surfaced. Since /click already uses JsonUtility.FromJson to parse input symmetrically, mirror that for the response (build a small serializable struct and JsonUtility.ToJson it), or at minimum escape \ and ". Impact is bounded — the endpoint is localhost-only and gated behind WORLDOS_QA_INPUT=1 — so this is hardening, not a blocker.
Category: Runtime correctness
Why this matters: Path-in-JSON-without-escaping is a latent parse-corruption bug; it costs nothing to serialize properly and it removes a platform-dependent failure mode from the QA capture path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc750c4730
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // cleanup before erroring: this run already hid scene renderers + destroyed lights + spawned GB_ parts — | ||
| // leaving them makes the ERROR path corrupt the editor scene state (codex review, #1575). | ||
| foreach(var g in UnityEngine.Object.FindObjectsByType<GameObject>(FindObjectsSortMode.None)){ if(g!=null && g.name.StartsWith("GB_")) UnityEngine.Object.DestroyImmediate(g); } | ||
| foreach(var r in UnityEngine.Object.FindObjectsByType<Renderer>(FindObjectsSortMode.None)){ if(r!=null) r.enabled=true; } |
There was a problem hiding this comment.
On the no-wall_run error path, the fresh cleanup enables every Renderer in the open scene rather than only the renderers this run disabled at startup. If the Unity scene has intentionally disabled reference, variant, or helper renderers, running this on open-air/perimeter-wall geometry will leave those objects visible for the next capture even though the script reports the scene was restored; keep the exact renderer list disabled by this run and restore only that list.
Useful? React with 👍 / 👎.
| // thin the run along its short axis so doorway gaps read (a run is 1 cell thick); | ||
| // EXTEND 0.6 past each end along the long axis so perpendicular runs OVERLAP at corners | ||
| // (owner v3.4 punch list: "the corners of the walls don't connect"). | ||
| if(sx>sz){ sz=Mathf.Min(sz,1.2f); sx+=1.2f; } else { sx=Mathf.Min(sx,1.2f); sz+=1.2f; } |
There was a problem hiding this comment.
When a wall_run is split around a door cell, this unconditional length extension pushes each adjacent segment about 0.6 world units into the gap. For a one-cell doorway the generated wall boxes narrow the 2-unit opening to roughly 0.8 units, and those same boxes are exported as runtime occluders, so actors or door affordances in the walkable doorway can be masked by wall volume that should only overlap at corners; only extend ends that meet other walls, not door splits.
Useful? React with 👍 / 👎.
…TC, byte-identical repro differs; bisects exonerate prompt/remap/geometry; LoRA-routing probe queued
evaOS review status: completedPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Review URL: #1575 (review) |
There was a problem hiding this comment.
Walkthrough
PR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9)
Head: 3d815f86e34973336665c3705bcf76e2c0ea5e7b into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +10/-0 | Unity/gameplay state | Moderate: validated P2 finding |
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +180/-2 | Unity/gameplay state | Moderate: validated P3 finding |
extensions/renderers/unity/scripts/build_room_unified.cs |
added | +381/-0 | Unity/gameplay state | Elevated: large change |
qa/evidence/crypt-v3/FINDINGS.md |
added | +113/-0 | Documentation | Low |
qa/evidence/crypt-v3/base_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v36.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/bisect_noapron.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/bisect_oldremap.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v31.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v32.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v33.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v34.json |
added | +70/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v35.json |
added | +73/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v36.json |
added | +71/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v34_MOLDED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_SPACED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_blind_panel.json |
added | +21/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v36_geometry.json |
added | +1069/-0 | Changed file | Elevated: large change |
qa/evidence/crypt-v3/crypt_v3_geometry.json |
added | +959/-0 | Changed file | Elevated: large change |
36 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 4 (P0: 0, P1: 1, P2: 1, P3: 2).
Dropped findings before posting: 0. High-severity findings: 1.
Risk Taxonomy
- Release regression: 2
- Runtime correctness: 1
- Security boundary: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #9, #1543, #1552, #1568, #1569.
Suggested labels: bug, unity, docs.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| @@ -0,0 +1,206 @@ | |||
| [check_grid_paint_coherence] crypt_fresh: INCOHERENT (17 checked, worst offset 1.62 cell, threshold 0.5) — torch_door_l painted 1.19 cell off authored cells [[5, 1]] (> 0.5; 68px, NCC 0.240) — actor sits on the cell, paint sits elsewhere; torch_door_r painted 1.62 cell off authored cells [[7, 1]] (> 0.5; 93px, NCC 0.250) — actor sits on the cell, paint sits elsewhere; pilaster_arch painted 0.94 cell off authored cells [[12, 3]] (> 0.5; 54px, NCC 0.361) — actor sits on the cell, paint sits elsewhere; torch_near_l not locatable near authored cells [[1, 4]] (peak NCC 0.137 < 0.2) — paint put no grid-shaped structure on the footprint; rubble_bl painted 0.68 cell off authored cells [[1, 1], [1, 2]] (> 0.5; 39px, NCC 0.427) — actor sits on the cell, paint sits elsewhere; broken_slabs painted 1.50 cell off authored cells [[1, 6], [1, 7]] (> 0.5; 87px, NCC 0.647) — actor sits on the cell, paint sits elsewhere; wall_w_0 painted 1.26 cell off authored cells [[0, 1], [0, 2], [0, 3], [0, 4], [0, 5], [0, 6], [0, 7], [0, 8], [0, 9]] (> 0.5; 73px, NCC 0.630) — actor sits on the cell, paint sits elsewhere | |||
| { | |||
| "passed": false, | |||
There was a problem hiding this comment.
P1: Headline proof artifact reports INCOHERENT (passed=false) — contradicts the PR's central alignment claim
The PR title claims "UNIFY-THE-FRAMES proof: ... boxes-as-occluders, truth overlay (playtest #9)" and CombatSurfaceClient.cs:317-325 asserts that with the new box sidecar "runtime masking and painted geometry cannot disagree (walls included)." But the sole coherence-evidence artifact committed with the PR states "passed": false with header line INCOHERENT (17 checked, worst offset 1.62 cell, threshold 0.5). 7 of 17 props DRIFT/UNLOCATED: wall_w_0 painted 1.26 cells off (73px, NCC 0.630), broken_slabs 1.50 off, torch_door_r 1.62 off, torch_near_l UNLOCATED (NCC 0.137 < 0.2), pilaster_arch 0.94 off. A wall drifting >1 cell is precisely the "walk-through-wall" class the PR claims to eliminate (CombatSurfaceClient.cs:719-720). The shipped evidence falsifies the shipped claim. Either the verdict is stale and a passing re-run must replace it, or the unification does not yet hold and the proof label must be downgraded before this can be called a proof.
Category: Release regression
Why this matters: Reviewers and downstream adoption (the FINDINGS.md says crypt v3.x is the adoption candidate) will trust the 'proof' framing. The committed artifact is a negative result; merging it as-is bakes a false proof claim into the release-evidence trail and could promote an incoherent plate whose occluders (now built verbatim from the same mis-registered boxes) mask actors at the wrong world positions.
| // UNIFY-THE-FRAMES (#1575): per-plate occluder-box sidecars (boxes/*.json emitted by | ||
| // build_room_unified.cs) ship exactly like plates — a manifest `boxes` entry with no | ||
| // packaged file silently degrades to footprint proxies (the proof build hit this). | ||
| string boxesSrcDir = Path.Combine(projectRoot, "boxes"); |
There was a problem hiding this comment.
P2: Boxes sidecars are copied only if a source boxes/ dir exists; a referenced-but-missing file silently degrades to footprint proxies
EnsurePackaged now copies boxes/*.json into StreamingAssets when a boxes/ source dir is present (BuildMacOSPlayer.cs:97-104), and the runtime reads a plate's boxes entry via pe.boxesPath (CombatSurfaceClient.cs:3143-3144, 3286-3308). The failure mode is one-way silent: if a plate manifest references boxes but the file is absent from the package, ApplyPlate logs plate boxes file missing ... (falling back to footprint proxies) and proceeds — meaning a room that was authored/conditioned with wall occluders can ship with the OLD per-cell footprint proxies that 'the footprint path never covered' (CombatSurfaceClient.cs:719-720), reintroducing the exact wall-masking gap. There is no packaging-time assertion that every boxes path named in plates_manifest.json resolves to a file actually copied. Add a build-time check (mirror the existing plates_manifest integrity step above it) that fails or loudly warns when a referenced boxes sidecar is not packaged.
Category: Release regression
Why this matters: This is a release-regression vector: a plate that tested correctly on the authoring machine (boxes/ present) can ship with degraded occluders if the boxes dir isn't included in the build inputs, and the only signal is a runtime Debug.LogWarning behind the black cover. Players would see actors walk through painted walls with no build-time guard.
| // swap; RebuildOccluders prefers these over per-cell footprint proxies. Reset the occluder signature | ||
| // so the next ApplyJson rebuild fires even when the surface's occluder set is unchanged. | ||
| _plateBoxes = null; | ||
| if (!string.IsNullOrEmpty(entry.boxesPath) && !System.IO.Path.IsPathRooted(entry.boxesPath)) |
There was a problem hiding this comment.
P3: boxesPath only blocks rooted paths; relative '../' traversal is not sanitized
The new sidecar loader guards only !System.IO.Path.IsPathRooted(entry.boxesPath) (CombatSurfaceClient.cs:3282), then does Path.Combine(Application.streamingAssetsPath, entry.boxesPath) + File.ReadAllText. A value like ../../something.json is not rooted and will read outside StreamingAssets. The plate-path loader above (ApplyPlate:3242) has the identical pattern, so this is consistent with existing code, but the new code compounds the surface (two manifest-string-driven file reads). Because the manifest is a local StreamingAssets file and the QA listener is localhost-only, real-world exposure is low; still, a .. segment check (or canonicalize-then-verify-under-StreamingAssets) would close it cleanly.
Category: Security boundary
Why this matters: Local-file read driven by a manifest string; primarily defense-in-depth against a tampered manifest. Not a remote attack vector given current gating.
| return _overlayMat; | ||
| } | ||
|
|
||
| void OnRenderObject() |
There was a problem hiding this comment.
P3: New GL overlay material is HideAndDontSave and never released; OnRenderObject is Built-in-RP-only
EnsureOverlayMaterial() creates _overlayMat with HideFlags.HideAndDontSave (CombatSurfaceClient.cs:341) and there is no OnDestroy release for it (StopQaInput/OnDestroy at 2241-2249 only tears down the HttpListener). The material leaks for the app lifetime (HideAndDontSave is not GC'd). Also, OnRenderObject (350) only fires in the Built-in Render Pipeline; the code asserts Built-in RP in comments, which is consistent with the rest of the file, but there is no runtime guard (e.g. it silently no-ops if a pipeline asset is ever assigned). Minor; add an OnDestroy _overlayMat?.Destroy() and consider a pipeline assert if RP could ever change.
Category: Runtime correctness
Why this matters: Material leak is negligible for a single long-lived client; flagged for completeness given the Assets/** scene/runtime risk profile.
…town pipeline + loop instruments Adversarial-invariant-verify (6 skeptics) found 3 CONFIRMED holes in the client seams: 1. PATH TRAVERSAL: plate boxes sidecar rejected only absolute paths (IsPathRooted), not '..' traversal — a crafted manifest could read outside StreamingAssets. Now GetFullPath-normalize + containment check + broad catch so a malformed sidecar degrades to footprint proxies and can never abort SwapPlateCo (the old narrow filter let ArgumentException wedge _plateSwapping). 2. STALE-OCCLUDER LEAK: entering a room with no manifest entry / missing plate left the PREVIOUS room's _plateBoxes live → RebuildOccluders placed old-room walls at old-room coords in the new room. Now gated on _plateBoxesLocId==_locId (tracked at box-load, correct through the in-swap rebuild). 3. LOAD CRASH: _qaShotPath was a static readonly field initializer calling Application.persistentDataPath — throws at MonoBehaviour construction (the /shot endpoint's own bug). Moved to StartQaInput (main thread) before the listener starts. Verified: box editor log shows 0 error CS, load exception gone. Plus Phase E town pipeline (tools/generate_town.py N-room orchestrator + qa/seed_gfx_town.py world seed; 4-room DunGen subgraph walked 3 cross_door hops correct) and loop instruments (qa/select_best_draw.py edge-recall selection + qa/overlay_boxes.py overlay+blob-solve).
Adversarial invariant-verify (6 skeptics, pre-merge)Ran the
Compile re-verified clean on the GEX44 box (editor log: warnings only). Mergeable once CI re-greens on the fix commit. |
evaOS review status: completedPR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1575 Review URL: #1575 (review) |
| string p = System.Environment.GetEnvironmentVariable("WORLDOS_QA_INPUT_PORT"); | ||
| if (!string.IsNullOrEmpty(p) && int.TryParse(p, out int pv) && pv > 0) port = pv; | ||
| // main thread — safe to read persistentDataPath here (never in a field initializer, #1575) | ||
| _qaShotPath = System.IO.Path.Combine(Application.persistentDataPath, "wos_shot.png"); |
| try | ||
| { | ||
| string root = System.IO.Path.GetFullPath(Application.streamingAssetsPath); | ||
| string bpath = System.IO.Path.GetFullPath(System.IO.Path.Combine(root, entry.boxesPath)); |
| catch (System.Exception e) | ||
| { Debug.LogWarning("[CSC] plate boxes load rejected/failed: " + e.Message); _plateBoxes = null; } |
There was a problem hiding this comment.
Walkthrough
PR: #1575 - UNIFY-THE-FRAMES proof: Unity-native room source, boxes-as-occluders, truth overlay (playtest #9)
Head: 282d8b6045f55ffae0c79eb9b74d6750d38a8d59 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 5/5 (~70 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/BuildMacOSPlayer.cs |
modified | +10/-0 | Unity/gameplay state | Moderate: runtime path |
extensions/renderers/unity/scripts/CombatSurfaceClient.cs |
modified | +204/-2 | Unity/gameplay state | Moderate: validated P2 finding |
extensions/renderers/unity/scripts/build_room_unified.cs |
added | +381/-0 | Unity/gameplay state | Moderate: validated P3 finding |
qa/evidence/crypt-v3/FINDINGS.md |
added | +113/-0 | Documentation | Low |
qa/evidence/crypt-v3/base_v31.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v32_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/base_v36.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/bisect_noapron.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/bisect_oldremap.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v31.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v32.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v33.json |
added | +46/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v34.json |
added | +70/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v35.json |
added | +73/-0 | Changed file | Low |
qa/evidence/crypt-v3/boxes_v36.json |
added | +71/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v33_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v34_MOLDED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_SPACED_CANDIDATE.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_blind_panel.json |
added | +21/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v35_overlay.png |
added | +0/-0 | Changed file | Low |
qa/evidence/crypt-v3/crypt_v36_geometry.json |
added | +1069/-0 | Changed file | Elevated: large change |
qa/evidence/crypt-v3/crypt_v3_geometry.json |
added | +959/-0 | Changed file | Elevated: large change |
39 additional changed files omitted from this walkthrough.
Review Signal
Validated inline findings: 5 (P0: 0, P1: 0, P2: 2, P3: 3).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- CI/build: 1
- Flaky test risk: 1
- Runtime correctness: 1
- Unity scene/prefab: 2
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - Unity asset/script/project files changed. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #9, #1543, #1552, #1568, #1569.
Suggested labels: unity, docs.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
| // no desktop/SCK capture, no titlebar calibration, no privacy exposure of other windows. | ||
| // POST-only (parity with /click); writes under persistentDataPath, never a shared /tmp | ||
| // fixed path (symlink hardening — evaos review on #1575). | ||
| _qaShot = 2; resp = "{\"ok\":true,\"path\":\"" + _qaShotPath.Replace("\\", "/") + "\"}"; |
There was a problem hiding this comment.
P2: /shot returns the capture path before ScreenCapture has written the file
The /shot handler sets _qaShot=2 on the listener thread and immediately replies {"ok":true,"path":"...persistentDataPath/wos_shot.png"}. The actual capture happens 2 frames later via ScreenCapture.CaptureScreenshot in Update(), and CaptureScreenshot is ITSELF asynchronous — it writes the PNG on a subsequent render frame, not on the call frame. A proof/QA driver that POSTs /shot then reads the returned path will race the writer: the file may not exist (or be a prior capture) when the driver opens it. The truth-overlay screenshot is the PR's central 'is the paint lying?' proof artifact (proof_inplayer_truth_overlay.png), so a flaky/missing capture directly undermines the evidence this PR ships. Add a readiness signal: e.g. capture into a temp name and atomically rename on completion, expose a /shot-status that polls for the rename, or have the driver retry-read until stable mtime. At minimum, lengthen the countdown and document the required post-POST read-delay for callers.
Category: Flaky test risk
Why this matters: The whole UNIFY-THE-FRAMES proof hinges on an in-player truth-overlay screenshot that must be reliably reproducible; a writer/reader race makes the headline evidence non-deterministic.
| { | ||
| _screenW = Screen.width; _screenH = Screen.height; // cache for the off-thread /health | ||
| // /shot: capture the app's OWN framebuffer (countdown lets the request thread return first). | ||
| if (_qaShot > 0 && --_qaShot == 0) |
There was a problem hiding this comment.
P2: /shot capture is skipped while _busy, delaying or stalling screenshot evidence
Update() does if (_busy) return; at line 1994 BEFORE the QA-input block (line 1998) that drains _qaShot. While a move/attack POST is in flight (_busy=true), the if (_qaShot > 0 && --_qaShot == 0) ScreenCapture.CaptureScreenshot(...) branch is never reached, so _qaShot never decrements and the capture is deferred until the busy window closes. /click has the same gate and that is correct for input (one move per frame), but a screenshot is an observation, not an action — gating it behind the combat/move busy flag means a /shot issued during the exact transition the overlay is meant to document (a room swap, a move reveal) is silently delayed past the moment of interest. Consider moving the _qaShot decrement/capture above the if (_busy) return; line, since CaptureScreenshot is a main-thread framebuffer read that does not touch the move/POST state machine.
Category: Unity scene/prefab
Why this matters: The overlay's highest-value use is capturing the live room-transition/disagreement moment; gating capture on not-busy defeats that and makes evidence timing-dependent.
| // PREVIOUS room's boxes live — RebuildOccluders would then place the old room's walls at the old | ||
| // room's world coords in the new room and discard the new room's real footprint occluders. Only | ||
| // trust the sidecar when it was applied FOR the current location (_plateLocId == _locId). | ||
| bool boxesForThisRoom = _plateBoxes != null && _plateBoxes.Count > 0 && _plateBoxesLocId == _locId; |
There was a problem hiding this comment.
P3: Comment names the wrong field (_plateLocId) for the stale-leak guard; code uses _plateBoxesLocId
The stale-leak guard comment block (lines 712-716) says 'Only trust the sidecar when it was applied FOR the current location (_plateLocId == _locId).' The actual guard on line 717 is _plateBoxesLocId == _locId. _plateLocId is a DIFFERENT field (line 406 — the location the backdrop plate texture was committed for, set in SwapPlateCo only on successful apply), whereas _plateBoxesLocId (line 328) is stamped unconditionally in ApplyPlate at line 3294. The code's guard is correct and robust, but the comment references the wrong field, which will mislead the next reviewer into thinking the guard tracks plate-texture commit (it does not — it tracks box-load attempt). Align the comment to say _plateBoxesLocId and note the deliberate difference from _plateLocId (boxes are stamped even on a failed/empty load so a missing sidecar cannot leak a prior room's boxes).
Category: Runtime correctness
| @@ -0,0 +1,381 @@ | |||
| // build_room_unified.cs — UNIFY-THE-FRAMES (Option 2, owner-ratified 2026-07-15): render a room's | |||
There was a problem hiding this comment.
P3: Editor script hardcodes Linux box paths; no guard if run on a non-GEX44 host
The script hardcodes /home/unity/worldos-unity/room_geometry.json and /home/unity/worldos-unity/Captures-Durable/ for reads and writes, with only the geometry path env-overridable (WORLDOS_ROOM_GEO) — the output Captures-Durable path is NOT env-overridable and will throw DirectoryNotFoundException/UnauthorizedAccessException on any host without that exact path (this Mac, the support VM, a contributor machine). File.WriteAllBytes/CreateDirectory will throw inside the unity-mcp execute context. The PR's AGENTS.md ties this tool to GEX44, but the file is tracked under extensions/renderers/unity/scripts and is therefore in the auto-review path for any host. Add a WORLDOS_ROOM_OUT env override (mirroring WORLDOS_ROOM_GEO) for the output directory, or resolve it relative to the project.
Category: CI/build
Why this matters: An editor script that throws on every non-GEX44 host is a latent CI/build-break if it ever gets wired into a menu item or batchmode step, and contradicts the host-portability intent of the env override it already half-implements.
| void OnRenderObject() | ||
| { | ||
| if (!_truthOverlay || Cols <= 0 || Rows <= 0) return; | ||
| if (Camera.current != Camera.main) return; // gameplay camera only (never scene/preview/probes) |
There was a problem hiding this comment.
P3: OnRenderObject truth overlay runs on every camera incl. reflection/probe if Camera.main is null
OnRenderObject guards with if (Camera.current != Camera.main) return;. Camera.main returns null when no main-tagged camera exists (e.g. during loading, or a probe-only render), in which case Camera.current != null is true and the guard PASSES — the overlay would draw for whatever camera is rendering (a reflection probe, a preview, a depth-only occluder pass). In practice the overlay is invisible (depth proxies are depth-only) so this is cosmetic/perf, but the intent stated in the comment ('gameplay camera only — never scene/preview/probes') is not actually enforced when Camera.main is null. Tighten to Camera.main == null || Camera.current != Camera.main.
Category: Unity scene/prefab
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 282d8b6045
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (_plateBoxes != null) | ||
| { | ||
| GL.Color(new Color(1f, 0.3f, 1f, 0.9f)); | ||
| foreach (var b in _plateBoxes) |
There was a problem hiding this comment.
When WORLDOS_TRUTH_OVERLAY is used after moving from a sidecar-backed plate into a room with no manifest entry or missing plate, _plateBoxes remains populated from the previous location and this overlay path still draws those stale magenta volumes. Fresh evidence beyond the earlier stale-occluder fix: RebuildOccluders is now location-gated, but OnRenderObject reads _plateBoxes directly, so the permanent alignment-proof screenshot can show boxes that are not the active runtime occluders for the current room.
Useful? React with 👍 / 👎.
| # door cells beyond the in-town targets stay unwired (doors to unselected rooms) | ||
| world["rooms"].append({"room": rid, "location_id": loc_of[rid], | ||
| "geometry": f"{args.town_id}_{rid}_geometry.json", "doors": pairs}) |
There was a problem hiding this comment.
When --rooms selects a connected subset of a larger layout, doorways to unselected neighbors are kept in the already-written per-room geometry but omitted from this world doors list. seed_gfx_town.py only turns door_pairs into SceneGrid.door_cells, so those extra geometry door gaps render as open arches but have no cyan door/cross_door action and are treated like ordinary walkable floor, leaving players able to walk into dead, non-transitioning exits; filter the geometry door cells to selected adjacencies or wire explicit exits before writing the room geometry/world pair.
Useful? React with 👍 / 👎.
| props += [p for p in geo.get("props", []) if p.get("kind") != "wall_run"] | ||
| prop_cells = {tuple(c) for p in props for c in p["cells"] if p["kind"] != "wall_run"} | ||
| geo["props"] = props | ||
| geo["walls"] = sorted(wall_cells) |
There was a problem hiding this comment.
For generator rooms whose footprint is not the full crop rectangle, such as Tessera cell_positions rooms or any room with interior voids, this overwrites the wall set returned by dungen_to_fixtures.build_geometry with only synthetic perimeter wall runs. seed_gfx_town.py later treats geo["walls"] as the engine's blocked cells, so every original non-floor cell inside the crop becomes walkable/rendered floor even though the generator never carved it; keep the original walls and add wall-run props instead of replacing the wall set.
Useful? React with 👍 / 👎.
| // unused headroom flattens low furniture into a sliver of the depth band (tavern v3 base: tables | ||
| // at h1.3 in a 9-unit range conditioned so weakly that flux relocated/dropped them; crypt only | ||
| // survived because 6.8-unit pillars filled the range). | ||
| float hx2=(cols/2f)*2.0f, hz2=(rows/2f)*2.0f, hy2=wallH+0.5f; |
There was a problem hiding this comment.
For camera_fit rooms using the default wall_height of 5 and any pillar/column prop, the molded pillar geometry above reaches about 7.3 units, but this remap range only samples up to wallH + 0.5. Since WOS/LinDepthRemap saturates outside _WOSDepthNear/_WOSDepthFar, the upper pillar structure is clipped/flattened in room_greybox_depth.png while the RGB render and box sidecar still contain it; derive the depth range from the actual generated boxes or mesh bounds.
Useful? React with 👍 / 👎.
| props.append(SceneProp(id=pid, kind=p["kind"], cells=footprint, | ||
| anchor_cell=footprint[0], occluder=True, | ||
| height_band="mid", silhouette="block")) |
There was a problem hiding this comment.
When a generated town is run without a packaged boxes sidecar, or a sidecar load fails, the client falls back to engine occluders emitted from SceneProp.height_band. This seeder stamps every non-wall prop as occluder=True with height_band="mid", so low furniture/campfires and tall trees both get mid-height runtime proxy masks that no longer match the generated geometry; derive occluder and height_band from the prop kind instead of hard-coding them.
Useful? React with 👍 / 👎.
…1575-squash) (#1576) * gemini-restyle route: greybox->Gemini = PoE2-caliber beauty (panel 7.5, beats incumbent 5.5) routing around the flux endpoint regression, BUT ~0.8-cell Gemini recompose drift = not yet the walkable recipe; honest tradeoff banked * Phase D room-class #4: THRONE HALL generalizes (120 walkable connected, corners/spacing clean) + molded dais/throne/banner kinds (vocab gap: altar/sarcophagus read as tombs). Structural pipeline proven on a new interior class; paint flux-blocked. * codify: room-class coverage table + town command chain + greybox->Gemini beauty route; throne-hall painterly preview frame --------- Co-authored-by: Eva <arncalso@gmail.com>
1 participant
The proof (template rule: frames embedded)
In-player, truth overlay ON — magenta wireframes = the occluder volumes (the exact boxes the depth conditioning was rendered from); red = engine-impassable cells; gold = walkable; cyan = door cells:
The unified base plate (flux depth-CN over the Unity render — structure lands ON the boxes):
The depth conditioning (full-range WOS/LinDepthRemap — the tomb is finally an unmissable monument to the CN):
Owner-ratified Option 2, proven end-to-end on the crypt in one session
The architecture: room truth renders INSIDE Unity from the client's own camera. One authored geometry →
build_room_unified.csrenders greybox + full-range depth + normals at the exact runtime camera (C# fit-ortho == Python_fit_ortho_sizeto the digit: 10.5224) and emitsroom_boxes.json— the volumes the depth was rendered from. flux depth-CN paints over THAT render. The same boxes ship as the plate's occluder sidecar: what masks an actor at runtime is byte-derived from what shaped the paint. The three reference frames (authoring render / diffusion paint / client display) are now ONE.The proof shot (in-player, truth overlay ON):
qa/evidence/unify-frames/proof_inplayer_truth_overlay.png— magenta occluder wireframes wrap the painted tomb/pillars/walls exactly; red impassable diamonds on painted furniture; gold walkable on painted floor; cyan door cell on the painted arch; actors grounded on legal cells. Engine walk enforcement separately verified live (tomb/pillar/wall walks rejected).Why the old failure classes die by construction:
coherence_verdict.json.New permanent instruments:
WORLDOS_TRUTH_OVERLAY=1/ G-key engine-truth overlay (no alignment claim ships without it agreeing with the paint) and QA/shot(in-app framebuffer capture — kills SCK titlebar calibration and desktop-capture privacy exposure).Known follow-ups (filed in-body, not blockers):
BuildMacOSPlayer.EnsurePackagedmust learn theboxes/dir (proof build injected it post-build); the base plate needs the Gemini style pass + panel before any adoption talk (this PR adopts nothing — the canonical manifest is untouched); tavern + camp regen through this pipeline; box editor scene renderers left disabled by greybox runs (reload scene before next lane).Research synthesis (6-angle workflow, Options 2 + 3):
qa/evidence/unify-frames/research_synthesis.txt— Infinity Engine/PoE production history validates the one-scene-one-camera architecture; Option 3 (full 3D + painterly texturing) assessed research-frontier with its multi-view value wasted on our fixed camera; its cheap decisive spike (camera-nudge test) documented for later.CU spent this proof: 9 (one flux base seed). Refs: playtest #9, #1543, #1552, #1568, PR #1569.