renderer: build_room_kit r8 — albedo separation for the flat-light placement gate (#83)#1687
Conversation
…acement gate (#83) r7 flat-light score 67.19% (lit 72.40%) vs the 99 bar; all 35 shared invisible-wall residuals were one class — a correctly-placed dark stone mass on a similar-toned floor, scoring 0.33-1.73 vs BLOCK_T=1.85. Make the flat capture albedo-separated so it measures PLACEMENT: 1. Floor: one shared KitFloor_PaleStone (pale warm-grey, cloned from the tile's own material to keep the kit texture) on every floor tile. 2. Mass: one shared KitMass_DarkMasonry forced onto walls/parapets/ buttresses/plinths/pillars/rubble. Tomb footprint (7-9,5-6) was the worst residual cluster (cells 0.13-0.84, dead-on-floor) -> darkened with KitProp_Stone (focal-prop mid-stone). 3. Brazier bases: dark stone plinth 1.3x0.45x1.3 under each brazier. 4. Rubble raised to 0.85u + dark masonry. 5. Wooden fallback boxes get KitProp_DarkWood (barrel (10,7) scored 0.33). 6. Lighting rig / braziers / capture logic / pillar sizing / buttress pass unchanged from r7. Runtime-only shared materials, reset per build, never written to the AssetDatabase; deterministic, no RNG.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 3 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 selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
evaOS review status: stale headPR: #1687 - renderer: build_room_kit r8 — albedo separation for the flat-light placement gate (#83) 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: #1687 Details: live=7f54284611ab145022052bbe10ca16ecd0a58b10 |
Flat ambient 0.6 crushed the score frame into a narrow dim band; the coverage stat (calibrated on bright painterly plates) under-read every mass — measured flat score DROPPED 67.19→59.90 after the r8 separation pass. At 1.0 flat, surfaces render at their albedo and the authored 0.72-vs-0.34 separation reaches the scorer.
evaOS review status: stale headPR: #1687 - renderer: build_room_kit r8 — albedo separation for the flat-light placement gate (#83) 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: #1687 Details: live=a124a1b7cc1e86c63a329bd78e5e2e23bb4d6cb0 |
…r8c) Albedo tints cannot separate floor from mass through the kit textures (everything converges to warm brown; flat score 67.19→59.90 across r7→r8 measured). The gate question is 'which floor quads are covered by mass through the contract camera' — render it as exactly that: floor renderers flat near-white, all other renderers flat near-black, unlit, lights off. Materials restored in finally; beauty stays in the lit frame.
Tall seg-black masses screen-occlude the floor quads of walkable cells behind them (measured: 50 false walk-throughs in r8c). The gate is a FOOTPRINT question — flatten Walls/Props/Impassable to 0.02x-height floor slabs for the seg render (y-scale+y-pos scaling preserves floor seating) and hide door-frame renderers so lintels/arches don't drop onto their open door cells. All transforms restored in finally.
evaOS review status: completedPR: #1687 - renderer: build_room_kit r8 — albedo separation for the flat-light placement gate (#83) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1687 Review URL: #1687 (review) |
![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: #1687 - renderer: build_room_kit r8 — albedo separation for the flat-light placement gate (#83)
Head: 33abb2ed520d5c893b8eea4dcf19479712092b26 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 1/5 (~12 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
extensions/renderers/unity/scripts/build_room_kit.cs |
modified | +176/-19 | Unity/gameplay state | Moderate: validated P3 finding |
Review Signal
Validated inline findings: 4 (P0: 0, P1: 0, P2: 0, P3: 4).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Runtime correctness: 3
- 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: #83, #84.
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.
| // floor. Its natural material lands near the (now pale) floor, so darken it with | ||
| // KitProp_Stone (a mid stone — keeps the focal-prop character, not the full mass dark) | ||
| // so it reads as a placed mass. Applied to the tomb AND its lid so they read as one. | ||
| FixMaterials(tomb, false, ref nMatFix, force: true, overrideMat: PropStoneMat()); |
There was a problem hiding this comment.
P3: tomb/lid override uses force:true unconditionally despite 'conditional' comment
The header and inline comment (L345-350) describe the KitProp_Stone darkening as CONDITIONAL — "the tomb ... UNLESS it lands near the floor" / "tomb ONLY if it lands near the pale floor". The actual call at L351 is unconditional: FixMaterials(tomb, false, ref nMatFix, force: true, overrideMat: PropStoneMat()) is applied to every sarcophagus regardless of its seated height or proximity to the floor, and the lid (L367) matches. This means every tomb in every room is recolored mid-stone, not only the ones that fail to separate from the floor. Either implement the stated height/proximity condition (e.g., only darken when the tomb's seated bounds stay under a floor-proximity threshold) or correct the comment to reflect that the darkening is unconditional, so the lit-frame visual change to the focal sarcophagus is intentional and reviewable.
Category: Runtime correctness
Why this matters: The sarcophagus is a focal prop whose lit-frame appearance is part of the room's beauty read; applying a permanent mid-stone recolor to ALL tombs (not just the residual-cluster case) is a broader lit-frame regression than the conditional language implies, and the comment/code mismatch makes future tuning error-prone.
| // reset the material-defensive caches for a clean run | ||
| _matResolved = false; _stoneMat = null; _woodMat = null; _loggedBadMats = new HashSet<string>(); | ||
| // r8: reset the shared albedo material instances so each build creates fresh runtime materials | ||
| _floorPaleMat = null; _massDarkMat = null; _propStoneMat = null; _propWoodMat = null; |
There was a problem hiding this comment.
P3: r8 shared albedo materials are leaked (nulled, never destroyed) on each BuildRoom
_floorPaleMat, _massDarkMat, _propStoneMat, _propWoodMat are reset to null at L153 but the prior Material instances are never released (no UnityEngine.Object.DestroyImmediate). Each is created via new Material(src) (Recolor, L921-927), producing native Unity objects. Unlike the seg frame's segWhite/segDark (which ARE DestroyImmediate'd at L634) and MakeStd brazier materials, these four shared mats are orphaned on every BuildRoom re-run. Over many re-runs in a long editor session this leaks native material objects. Destroy the previous instances before nulling them at L153 (guard for null), mirroring the seg-frame cleanup idiom.
Category: Runtime correctness
Why this matters: Native Material leaks are not GC-collected and accumulate across re-runs of the Build Room menu item; in a long authoring/CI-editor session doing many kit-room iterations this can pressure GPU memory and is inconsistent with the file's own cleanup discipline for segWhite/segDark.
| // near-black, unlit, lights off. Materials restored in finally; beauty lives in the lit frame. | ||
| var segRends = new List<Renderer>(); | ||
| var segMats = new List<Material[]>(); | ||
| var segWhite = new Material(Shader.Find("Unlit/Color")); segWhite.color = new Color(0.92f, 0.92f, 0.92f); |
There was a problem hiding this comment.
P3: seg-frame material construction outside try leaks light/ambient state if Unlit/Color is missing
In CaptureRoom, room lights are disabled (L576-577) and RenderSettings.ambientMode/ambientLight are captured (L578-579) BEFORE the inner try block. The seg materials are then constructed at L587-588 with new Material(Shader.Find("Unlit/Color")), which sit OUTSIDE the inner try (which begins at L597). If Shader.Find("Unlit/Color") returns null (the shader is not always guaranteed present — it can be absent from alwaysIncludedShaders or stripped in some project configurations), new Material(null) throws NullReferenceException/ArgumentNullException before the inner try is entered, so the inner finally (L629-638) never runs: the room's lights remain disabled and RenderSettings ambient stays mutated (Flat / 1.0 white) in the live editor scene until manually reset. Move the new Material(...) calls inside the inner try, or null-guard them and fall back to MakeStd-style Standard materials, so the restore block always runs.
Category: Runtime correctness
Why this matters: CaptureRoom is documented as side-effect-free ("capture never saves the scene", restore in finally). A thrown shader lookup breaks that invariant for the in-editor scene state (lights off, ambient blown out), which can silently corrupt subsequent captures or a later BuildRoom's lighting baseline without any on-disk trace.
| if (doorGroup != null) | ||
| foreach (var rr in doorGroup.GetComponentsInChildren<Renderer>()) | ||
| if (rr != null && rr.enabled) { rr.enabled = false; hidRends2.Add(rr); } | ||
| foreach (var rr in root.GetComponentsInChildren<Renderer>()) |
There was a problem hiding this comment.
P3: seg material capture skips inactive renderers but flatten may deactivate none — verify door-leaf disabled children
The seg material-swap loop uses root.GetComponentsInChildren<Renderer>() WITHOUT the includeInactive=true flag (L614). This is fine for the Doors group because door renderers are explicitly disabled and tracked in hidRends2 (L610-613) and are not expected in the seg swap. However StripDoorLeaf (called during BuildRoom, L282/795) disables door-leaf GameObjects under door instances that are children of the Doors group; those disabled leaf renderers are also skipped by the no-includeInactive capture, which is intended. The risk is the inverse: any renderer that is active but whose parent was deactivated by StripDoorLeaf under Walls/Props would be missed by both the seg capture AND the flatten (which iterates foreach (Transform ch in g) — also skips inactive children). Confirm no Walls/Props child is deactivated by leaf-stripping; otherwise its materials/transforms escape the seg frame entirely and it renders with its r8 albedo into the 'unlit' seg capture, polluting the score. Low likelihood given StripDoorLeaf only targets Doors, but worth a guard (iterate with includeInactive or assert).
Category: Unity scene/prefab
Why this matters: The segmentation score frame is the placement gate; a renderer that escapes both the white/dark swap and the flatten renders with its real (dark) albedo at full height into the unlit frame, which can either mask a walkable cell (false invisible-wall) or, if tall, occlude a real one — directly skewing the gate this PR exists to fix.
1 participant
build_room_kit r8 — albedo separation so the flat-light frame measures PLACEMENT (#83/#84)
Why
r7's flat-light score frame read 67.19% (lit 72.40%) vs the 99 bar
(
kit_crypt_r7_registration.json/kit_crypt_r7_lit_registration.json). Per-cellattribution showed all 35 shared invisible-wall residuals were ONE class: a stone
mass that is correctly placed but scores 0.33–1.73 against BLOCK_T=1.85 because it
sits on a similar-toned floor (needs ~2× contrast). The masses ARE placed — they just
don't SEPARATE tonally. Design decision (orchestrator): the flat
_score.pngis theplacement gate, the lit render is the beauty artifact. r8 makes the flat capture
albedo-separated so the gate measures placement, not tone.
Changes (single file:
extensions/renderers/unity/scripts/build_room_kit.cs)KitFloor_PaleStone(pale warm-grey0.72,0.70,0.66),cloned from the placed tile's OWN material so the kit shader/texture survives (color
multiplies over it), assigned to every floor renderer.
KitMass_DarkMasonry(0.34,0.30,0.27) force-assigned viathe FixMaterials force path onto walls / parapets / buttresses / plinths / pillars / rubble.
Tomb (item 2 conditional): verified from r7 evidence that the tomb footprint
(7-9,5-6) was the WORST residual cluster — every cell an invisible wall scoring
0.13–0.84 (dead-on-floor). Darkened with
KitProp_Stone(0.42,0.40,0.38, a focal-propmid-stone, not the full mass dark) on the tomb AND its lid so they read as one mass.
1.3 × 0.45(h) × 1.3centered on each brazier cell,seated on the floor (the thin stem covered almost none of the impassable cell quad).
0.85u+KitMass_DarkMasonry(r7 rubble cells (2,9),(2,10),(3,10)scored 0.92–1.49, read as floor).
KitProp_DarkWood(0.30,0.24,0.18) — r7 barrel (10,7) scored 0.33.sizing, buttress pass. The lit render now reads darker-walled / paler-floored — acceptable;
beauty is tuned in the painterly post stage.
Acceptance
Deterministic (no RNG). All r8 materials are runtime-only shared instances, created once
per build (lazily) and reset at the top of
BuildRoom— never written to the AssetDatabase;idempotent under rebuild (root destroyed+rebuilt each run). C# compiled + rendered on the GEX44
Unity box by the orchestrator's render loop (no box work here).
Packet:
/Volumes/LEXAR/Codex/session-notes/2026-07-22/g4-build/dispatch/kit-room-r8-spec.md