Extent contract (#1543, M-ALIGN): painted room == playable grid#1555
Conversation
The tavern plate out-painted a room LARGER than its authored 12x10 grid (unreachable painted floor, invisible walls at grid edges; owner playtest #8). Root cause: the fixed ortho=13 greybox leaves canvas margins around the room and the style pass out-paints them into "more room". RECIPE half (fix by construction, strictly OPT-IN so existing rooms' math is byte-identical — the coherence/drift/journey instruments share the rig): - greybox_render_headless: CAMERA-FIT mode (--camera-fit / geometry field camera_fit:true) fits the ortho SCALE from the room's grid extent so the diamond + wall band fills the frame edge-to-edge. Only the ortho scale changes; cell_to_world/world_to_screen default to ORTHO_SIZE untouched. - author_room_geometry: explicit PERIMETER WALL BAND as continuous wall RUNS (kind wall_run, one box per edge run split at doors — the #1539 no-crenellation rule) so painted walls sit on impassable cells by construction and the door stays walkable. New tavern_fit reference room emits both; tavern stays as-is (pinned by engine-grid tests). HOTFIX half: tools/mask_plate_extent.py feathers everything outside the grid diamond + wall band to a dark vignette (deterministic, PIL-only). Masked the tavern plate into qa/evidence/1543/ (before/after) — no plates_manifest/deploy change; adoption is a separate orchestrator call. Compat: all room geometries + default greybox renders byte-identical vs main; engine walkslice/scene_grid (39) + qa coherence/drift/journey/ derive/dungen (39) + new extent-contract (9) tests green.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 47 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 (5)
📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
evaOS review status: completedPR: #1555 - Extent contract (#1543, M-ALIGN): painted room == playable grid evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1555 Review URL: #1555 (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: #1555 - Extent contract (#1543, M-ALIGN): painted room == playable grid
Head: 26c0f251757fb6620ee8da798e4168f11bd8f627 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 4/5 (~56 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
qa/evidence/1543/tavern_fit_geometry.json |
added | +1/-0 | Changed file | Low |
qa/evidence/1543/tavern_fit_greybox_camerafit.png |
added | +0/-0 | Changed file | Low |
qa/evidence/1543/tavern_fit_greybox_camerafit_cutaway.png |
added | +0/-0 | Changed file | Low |
qa/evidence/1543/tavern_fit_greybox_fixed13.png |
added | +0/-0 | Changed file | Low |
qa/evidence/1543/tavern_mask_before_after.png |
added | +0/-0 | Changed file | Low |
qa/evidence/1543/tavern_truegrey_v1_masked.png |
added | +0/-0 | Changed file | Low |
qa/greybox_render_headless.py |
modified | +102/-25 | Changed file | Low |
qa/test_greybox_extent_contract.py |
added | +136/-0 | Test coverage | Moderate: validated P3 finding |
Review Signal
Validated inline findings: 1 (P0: 0, P1: 0, P2: 0, P3: 1).
Dropped findings before posting: 1. High-severity findings: 0.
Risk Taxonomy
- Flaky test risk: 1
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - WorldOS repo profile implies Unity runtime risk. 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: #1543, #8, #1539.
Suggested labels: tests.
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.
| fit = tmp_path / "fit.png" | ||
| g.render(geo, str(fixed), camera_fit=False) | ||
| g.render(geo, str(fit), camera_fit=True) | ||
| assert _width_fill(fixed) < 0.75 # fixed rig leaves a wide margin |
There was a problem hiding this comment.
P3: Fixed-rig width-fill threshold (<0.75) leaves little headroom
test_fixed_rig_leaves_margin_that_camera_fit_removes asserts _width_fill(fixed) < 0.75. For the 12x10 tavern under ortho=13 (half_w = 131.75 = 22.75), the floor diamond's max |cam_r| = (12/22 + 10/2*2)/sqrt(2) ~ 15.56, so the diamond spans ~68% of frame width; walls extend it slightly upward. The <0.75 bound leaves <7 percentage points of headroom. A modest future change to default wall_height, a new tall prop at the edge, or a sub-pixel anti-aliasing difference across PIL versions could trip this assert without indicating any real defect in the extent contract. Consider widening to <0.85 (or asserting relative to _width_fill(fit) only, which is the actual contract) so the test pins intent rather than a fragile absolute.
Category: Flaky test risk
Why this matters: This test gates the defect/fix comparison the PR is built around. A threshold this close to the measured value makes the test brittle to unrelated renderer tweaks and can produce a red CI signal that does not correspond to a regression in the camera-fit contract.
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: 26c0f25175
ℹ️ 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".
| geo = _geometry(12, 10, "worn wooden planks", props, perimeter=False, | ||
| door_cells=[[8, 0]], camera_fit=True) |
There was a problem hiding this comment.
When this geometry is taken through the documented room pipeline, greybox_render_headless.render() now generates the plate with a fitted ortho, but the manifest/drift/coherence consumers still project with the fixed ORTHO_SIZE: tools/derive_room_manifest.py calls project_cell_bbox()/world_to_screen() without any camera-fit metadata, and qa/check_grid_paint_coherence.py rebuilds bboxes/templates the same way. That means a correctly registered camera_fit plate gets screen_bbox, cell pitch, and templates in the old ortho=13 frame, so drift/coherence evidence for tavern_fit will be incomparable or falsely fail. Please carry the fitted ortho/camera flag through those consumers before emitting camera-fit geometries.
Useful? React with 👍 / 👎.
| ("barrels", "barrel", [[2, 6], [3, 6], [2, 7], [3, 7]]), | ||
| ] | ||
| # --- explicit continuous perimeter wall band, door (8,0) left open --- | ||
| props += _perimeter_wall_run_props(12, 10, door_cells=[[8, 0]]) |
There was a problem hiding this comment.
Emitting perimeter walls as wall_run props means they are included in derived manifests, but the downstream geometry math still treats unknown kinds via _spec_for_kind() as square default boxes; tools/derive_room_manifest.py::_prop_box_corners() and qa/check_grid_paint_coherence.py::prop_box_screen_bbox() do not use the new rectangular wall-run extents. For long edge runs this produces oversized square occlusion/bbox/template regions instead of a thin wall band, which can over-occlude the room and make wall coherence checks fail even when the rendered wall is correct. Add the same wall-run extent handling there or filter these structural walls out of prop-style derivation.
Useful? React with 👍 / 👎.
| geo = _geometry(12, 10, "worn wooden planks", props, perimeter=False, | ||
| door_cells=[[8, 0]], camera_fit=True) |
There was a problem hiding this comment.
If this same tavern_fit geometry is sent through the existing GEX44/Unity generation path (qa/gen_room_from_scene_grid.sh invokes extensions/renderers/unity/scripts/build_room_greybox.cs), the renderer still hardcodes orthographicSize=13 and draws one cube per prop cell, so it ignores both camera_fit and the continuous wall_run contract. That path would regenerate exactly the fixed-rig margins/crenellated wall band this change is trying to eliminate; either update the Unity renderer or make the new geometry/headless-only scope impossible to use through that lane.
Useful? React with 👍 / 👎.
1 participant
Closes #1543 (milestone M-ALIGN).
Problem
The tavern plate out-painted a room larger than its authored 12×10 grid — unreachable painted floor, invisible walls at the grid edges, edge cells on painted furniture (owner playtest #8). Root cause: the fixed
ortho=13greybox leaves canvas margins around the room, and the style pass happily out-paints those margins into "more room".RECIPE half — fix by construction (strictly OPT-IN)
Camera-fit and the wall band are opt-in so existing rooms render byte-identical — the registration/coherence instruments (
check_grid_paint_coherence.py,check_plate_drift.py,journey_visual_sweep.py) share the fixed rig and are untouched.qa/greybox_render_headless.py— CAMERA-FIT mode (--camera-fit/ geometry fieldcamera_fit: true): computes the ortho scale from the room's own grid extent so the diamond + perimeter wall band fills the 1344×768 frame edge-to-edge. Only the ortho scale changes;cell_to_world/world_to_screendefault toORTHO_SIZE(an added optional param — all 8 existing callers omit it → identical).tools/author_room_geometry.py— explicit PERIMETER WALL BAND as continuous wall runs (kindwall_run, one box per edge run split at doors — the docs(runbook): fold in the three cold-agent gaps from the CRYPT-RICH run #1539 no-crenellation rule) so painted walls sit on impassable cells by construction and the door gap stays walkable. Newtavern_fitreference room emits both;tavernstays as-is (its geometry is pinned by the engine-grid tests).Camera-fit greybox of
tavern_fit— the diamond + wall band fills the frame (width fill 0.961, vs 0.684 under the fixed rig):HOTFIX half — mask already-painted plates
tools/mask_plate_extent.pyfeathers everything outside the grid diamond + wall band to a dark vignette (deterministic, PIL-only). Masked the tavern plate (worst offender) intoqa/evidence/1543/. Noplates_manifest.json/ deploy change — adoption of the masked plate is a separate orchestrator call.Before (out-paints past the grid) | After (fades to darkness):
Compat proof
tavern/crypt/camp/crypt_rich) byte-identical vs main; default greybox renders byte-identical vs main.test_walkslice_seed_grid+test_scene_grid(39,uv run), qagrid_paint_coherence/plate_drift_gate/journey_eval/derive_room_manifest/dungen_to_fixtures(39),greybox_sidecars_headless(2), newtest_greybox_extent_contract(9).Test:
python3 -m pytest qa/test_greybox_extent_contract.py -q -p no:xdist— the deterministic pixel assert (camera-fit tavern ≥90% frame width) + opt-in byte-identity + wall-band/door invariants.