qa: registration instrument — per-room paint-vs-collision agreement score + honest baseline (#1680)#1681
Conversation
…e + honest baseline (#1680) Adds qa/registration_score.py: for each plates_manifest room, the screen-space agreement between the PAINTED-object mask (paint_coherence per-cell coverage) and the COLLISION set (walkmask + boxes-sidecar footprints projected through the contract camera). Names the two navigation-truth defects per cell — invisible walls (painted open, collides) and walk-through furniture (painted blocked, walkable) — with a disagreement heatmap + JSON report per room, a 0.99 ship bar, and the honest full-manifest baseline (every room FAILS, 28-69% agreement). - qa/registration_score.py: instrument + check/score-rooms CLI (tri-state exit) - qa/test_registration_score.py: 10 deterministic cores (synthetic plate + pure math) - qa/evidence/registration/: baseline reports + heatmaps for all 8 manifest rooms - .github/workflows/ci.yml: blocking test core + non-blocking baseline w/ artifact upload
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 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 (8)
📒 Files selected for processing (12)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
evaOS review status: completedPR: #1681 - qa: registration instrument — per-room paint-vs-collision agreement score + honest baseline (#1680) evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1681 Review URL: #1681 (review) |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| import argparse | ||
| import json | ||
| import statistics |
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import json |
| ortho = _fit_ortho_size(9, 9) | ||
| model = P.derive_room(geo) | ||
| collision = R.collision_cells(model, set()) | ||
| invisible_wall = (4, 4) # the interior prop footprint cell we leave painted OPEN (on-frame) |
![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: #1681 - qa: registration instrument — per-room paint-vs-collision agreement score + honest baseline (#1680)
Head: 435e61e329f68eed1bf1120078ee496174bef6a1 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 |
|---|---|---|---|---|
.github/workflows/ci.yml |
modified | +23/-0 | Changed file | Moderate: validated P3 finding |
qa/evidence/registration/camp_clearing_heatmap.png |
added | +0/-0 | Changed file | Low |
qa/evidence/registration/camp_clearing_night_heatmap.png |
added | +0/-0 | Changed file | Low |
qa/evidence/registration/camp_clearing_night_registration.json |
added | +2192/-0 | Changed file | Elevated: large change |
qa/evidence/registration/camp_clearing_registration.json |
added | +2192/-0 | Changed file | Elevated: large change |
qa/evidence/registration/crypt_heatmap.png |
added | +0/-0 | Changed file | Low |
qa/evidence/registration/crypt_registration.json |
added | +2352/-0 | Changed file | Elevated: large change |
qa/evidence/registration/dwing_room_1_heatmap.png |
added | +0/-0 | Changed file | Low |
qa/evidence/registration/dwing_room_1_registration.json |
added | +1116/-0 | Changed file | Elevated: large change |
qa/evidence/registration/shop_heatmap.png |
added | +0/-0 | Changed file | Low |
qa/evidence/registration/shop_registration.json |
added | +1624/-0 | Changed file | Elevated: large change |
qa/evidence/registration/summary.json |
added | +15264/-0 | Changed file | Elevated: large change |
qa/evidence/registration/tavern_heatmap.png |
added | +0/-0 | Changed file | Low |
qa/evidence/registration/tavern_registration.json |
added | +1924/-0 | Changed file | Elevated: large change |
qa/evidence/registration/tavern_snug_heatmap.png |
added | +0/-0 | Changed file | Low |
qa/evidence/registration/tavern_snug_registration.json |
added | +1500/-0 | Changed file | Elevated: large change |
qa/evidence/registration/throne_hall_heatmap.png |
added | +0/-0 | Changed file | Low |
qa/evidence/registration/throne_hall_registration.json |
added | +2340/-0 | Changed file | Elevated: large change |
qa/registration_score.py |
added | +454/-0 | Changed file | Moderate: validated P3 finding |
qa/test_registration_score.py |
added | +196/-0 | Test coverage | Low |
Review Signal
Validated inline findings: 2 (P0: 0, P1: 0, P2: 0, P3: 2).
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
- Flaky test risk: 1
- Runtime correctness: 1
Validation and Proof
2 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.
- required: CI/release smoke proof - CI, release, launchd, or package metadata changed. Proof: green GitHub check; release-status; coverage-audit; rollback note.
Proof status: missing - 2 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: #1680, #1, #83.
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.
| # mismatch' — every existing room is below the 0.99 ship bar; that is the finding, not a | ||
| # regression). The uploaded summary + per-room heatmaps are the acceptance ruler for the #83 | ||
| # 3D-first rebuild chain; a room only clears this once rebuilt. | ||
| continue-on-error: true |
There was a problem hiding this comment.
P3: continue-on-error masks harness-ERROR (exit 2) indistinguishably from expected-red (exit 1)
The 'Registration BASELINE' step has continue-on-error: true to accommodate the intentionally-red baseline. That is correct for the below-bar case (exit 1). But qa/registration_score.py defines a tri-state CLI where exit 2 means an indeterminate/harness ERROR (a room's plate/geometry/ortho/boxes could not be measured) and is explicitly 'never a pass'. With continue-on-error, GitHub marks the step yellow/x-passed-with-warning for BOTH exit 1 and exit 2, so a future regression that turns a manifest room unmeasurable (e.g., a cameraPin.ortho lost to 0, a renamed geometry stem, a missing boxes sidecar) would be indistinguishable from the expected-red baseline in the job's pass/fail signal. Consider gating the harness-ERROR path separately — e.g., have score_manifest_rooms write an errored flag into the uploaded summary and add a tiny blocking follow-up step (if [ -f .../summary.json ]; then python -c "import json,sys; sys.exit(1 if json.load(open('...'))['errored'] else 0)") so an indeterminate batch is loud while the expected-red agreement score stays non-blocking. The blocking pytest step (test_score_manifest_rooms_smoke) mitigates a total measurement break, but only asserts at least one room is measurable, so a partial harness break (some rooms error, others still score red) would still slip through.
Category: Flaky test risk
Why this matters: The whole point of the tri-state design is that a broken harness must never read green OR red — it must read ERROR. continue-on-error collapses that distinction at the CI-status layer, so the exit-2 guarantee the docstring promises is not actually visible to a reviewer looking at the check run.
| out_dir.mkdir(parents=True, exist_ok=True) | ||
| report = {"bar": bar, "block_threshold": block_t, "passed": True, "errored": False, "rooms": []} | ||
| for reg_key, entry in registry.items(): | ||
| stem = _resolve_geometry_stem(reg_key, entry) |
There was a problem hiding this comment.
P3: _resolve_geometry_stem fallback can silently bind a new manifest room to the wrong-named geometry
For manifest keys not in the explicit _GEO_STEM bridge, _resolve_geometry_stem falls back to candidate stems derived from the boxes filename (with a trailing _v stripped) and finally the key itself. This is a reasonable best-effort, but the manifest convention is inconsistent: e.g. throne_hall ships boxes 'throne_hall_v1_boxes.json' yet its geometry stem is 'throne_hall' (pinned explicitly), while shop pins 'shop' against boxes 'shop_v1_boxes.json'. A newly added room whose boxes path is 'foo_v2_boxes.json' but whose geometry file is 'foo_v2_geometry.json' would correctly resolve via the non-stripped candidate, but one whose geometry is 'foo_geometry.json' would resolve via the _v2-stripped candidate. Either way it resolves silently — there is no assertion that the resolved geometry actually matches the room the plate was conditioned on, so a mis-bound geometry (wrong cols/rows) would produce a plausible-looking but wrong agreement score rather than an error. Recommend either (a) requiring every manifest entry to name its geometry explicitly (a 'geometry' field) instead of inferring it, or (b) logging the resolved stem at score time so a mismatch is auditable in the uploaded summary. Today all 8 keys are pinned so this is latent, not active.
Category: Runtime correctness
Why this matters: This instrument is positioned as the acceptance ruler for the #83 3D-first rebuild chain — a room only clears once it genuinely agrees. If the geometry binding is inferred rather than declared, a future room could 'clear' against the wrong footprint and the ruler would be measuring the wrong room.
![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: 435e61e329
ℹ️ 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 boxes_rel: | ||
| boxes_path = _BOXES_DIR / Path(boxes_rel).name | ||
| if boxes_path.is_file(): | ||
| boxes_list = json.loads(boxes_path.read_text(encoding="utf-8")).get("boxes", []) |
There was a problem hiding this comment.
When a plates_manifest room declares a boxes sidecar but the file is absent, this branch leaves boxes_list as None and the room is still scored as if it had no proxy collision footprints (boxes: false). That makes the baseline and heatmap silently wrong for any typo/missing artifact instead of treating the room as unmeasurable; malformed sidecars also parse outside the per-room try, so they can abort the batch before writing the summary.
Useful? React with 👍 / 👎.
| # mismatch' — every existing room is below the 0.99 ship bar; that is the finding, not a | ||
| # regression). The uploaded summary + per-room heatmaps are the acceptance ruler for the #83 | ||
| # 3D-first rebuild chain; a room only clears this once rebuilt. | ||
| continue-on-error: true |
There was a problem hiding this comment.
In this CI context, continue-on-error: true masks every nonzero exit from score-rooms, not just the expected below-bar exit 1; GitHub's workflow syntax documents that this lets the job pass when the step fails. If the scorer returns exit 2 because a room's plate/geometry/ortho/boxes cannot be measured, paint-drift-gate still stays green and uploads an indeterminate baseline, so the step should only tolerate exit 1 and re-raise exit 2.
Useful? React with 👍 / 👎.
| boxes = json.loads(Path(args.boxes).read_text()).get("boxes", []) if args.boxes else None | ||
| res = run_room(args.plate, geo, args.ortho, boxes=boxes, bar=args.bar, | ||
| block_t=args.block_threshold, heatmap_path=args.heatmap) | ||
| except (HarnessError, OSError, ValueError) as exc: |
There was a problem hiding this comment.
When check is run with malformed geometry, run_room()/derive_room() can raise KeyError for required fields such as cols or rows, but this handler does not catch it. That bad-input path produces a traceback with Python's generic exit 1, which automation cannot distinguish from a below-bar registration verdict; catch it as a harness error like the batch path so bad geometry exits 2.
Useful? React with 👍 / 👎.
| if not model.walkable: | ||
| raise HarnessError(f"{model.room}: geometry has no walkable cells") | ||
| box_cells = box_footprint_cells(boxes or [], model.cols, model.rows) | ||
| collision = collision_cells(model, box_cells) |
There was a problem hiding this comment.
For any room where a boxes sidecar covers a geometry-walkable cell, this line promotes that renderer-only volume into collision, so a painted table/sarcophagus can be counted as agreement even though runtime /move still routes from the engine's grid_impassable derived from SceneGrid and surfaced as impassable. That undercounts the exact walk-through furniture failures this metric is supposed to expose; the scorer should compare against the engine-authored walkmask/campaign state, with boxes reported separately if useful.
Useful? React with 👍 / 👎.
1 participant
What
The REGISTRATION instrument chartered in #1680 — the measuring stick for the project's #1 demo blocker: per-room agreement between the PAINTED room and its COLLISION set. It makes the owner's "98% mismatch" (walking through painted tables, slamming into invisible walls) a single honest number per room, and names the two failure modes cell-by-cell.
qa/registration_score.py+qa/test_registration_score.py, the honest full-manifest baseline inqa/evidence/registration/, and a CI step.Method (3 lines)
Euler(30,45,0),pos=-fwd*80, the room's pinned ortho —greybox_render_headless.world_to_screen+cell_to_world): the walkmask (walls ∪ non-wall_runprop footprints − doors, viapaint_coherence.derive_room) ∪ the boxes-sidecar proxy footprints (every non-floor box volume inboxes/<room>_boxes.json, its x/z extent mapped to the cells it overlaps).paint_coherence.py's per-cell coverage machinery (floor-baseline +coverage_score) at per-cell grain;painted_blocking= score ≥COVERED_T(the same "covered" definition the coherence gate uses).{room, agreement_pct, invisible_wall_cells, walkthrough_cells, per_cell}. Ship bar--bar 0.99.Honest baseline (every existing room FAILS — that is the finding, not a regression)
Not one existing room is anywhere near the 0.99 ship bar (28–69% agreement). This is the honest per-room measure #1680 asked for — the acceptance ruler for whatever construction path #83 decides; a room only clears once rebuilt through the 3D-first chain (rebuild-not-patch).
Evidence / heatmaps
Per-room reports + disagreement heatmaps committed to
qa/evidence/registration/(<room>_registration.json,<room>_heatmap.png,summary.json). Thecrypt_heatmap.pngreads exactly like the owner's complaint: cyan walk-through cells sit over the painted sarcophagus/pillars (you see the tomb, you walk through it) while the red invisible-wall footprints land a row down-screen (you see floor, you can't walk there).CI
Added to the
paint-drift-gatejob (already has the Pillow+numpy lane):qa/test_registration_score.py, 10 tests);continue-on-error, like the coherence-freshness lint) that runs the scorer over every manifest room and uploads the summary + heatmaps as theregistration-baselineartifact. Non-blocking because the baseline is expected-red until rooms are rebuilt.Tests
python -m pytest qa/test_registration_score.py— 10 passed. Red-first: a synthetic contract-frame plate with a deliberate invisible wall + walk-through asserts the exact agreement % and both disagreement lists; pure-math cores assert the agreement/binarization; box-footprint cores assert the world-volume→cell mapping.Notes / deviations
agreement_pctis measured over all on-frame cells (perimeter included). Floor-plane perimeter-wall cells often project to painted-open regions (the wall silhouette rises up-screen), which the instrument honestly counts as invisible walls — consistent with how collision/walkmask is keyed to floor-plane cells.