Repository navigation
feat(backend): bridge base-scene/v0 metres to the placement wire scene - #272
Conversation
Add server/placement/scene_bridge.py: a pure convert_scene(document) that turns the backend base-scene/v0 contract (metres, meter_anchored/v0 frame, Scene-validated) into the placement wire scene the unchanged top-level POST /v1/placements server accepts (feet, server/schemas/scene.schema.json). Lengths convert by dividing by exactly 0.3048; s coordinates are already measured from the meter in both contracts, so spans convert without rebasing — and every declared s value (wall s_range_m, chain ends, bounded object spans, the meter anchor's s=0 centre) is cross-checked against the plan geometry at a 1e-6 m float-noise bound, so a self-contradictory scene is refused, never silently accepted. The initial supported scope is one straight observed house wall, an observed meter anchor (point or wall_rect) and level ground; everything outside it raises SceneBridgeError with code placement_scene_unsupported (valid scene, outside the scope) or placement_scene_invalid (the source contradicts itself or the schema), each carrying a JSON pointer naming the offending input. Unmappable provenance (generated/inferred evidence, unmapped classes, sources, materials) and unrepresentable geometry (stand-offs, heightfields, sloped ground, grid coverage, keyframes, generated regions) are refused rather than guessed at; nothing converts to a default silently. The new test module computes every expected value from first principles (metres / 0.3048 written out, plan geometry with math.hypot) and validates each converted scene through the real root parser (server/scene.py via placement/adapter.load_solver) and scene.schema.json, including a rotated/translated wall with a nonzero s origin. Full backend suite: 1248 passed, 17 skipped, 5 failed — the documented baseline (SSR hygiene + 4 eval gates) identical on base; ruff check and format clean. Co-authored-by: obvious-autobuild[bot] <262744130+obvious-autobuild[bot]@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Independent review (Obvious, Autobuild + Max) Verdict: ready — as a draft, nothing blocks ready-for-review. Every load-bearing claim in the body checks out on a fresh checkout of Findings (ranked)
What I verified (commands and results)Environment: fresh clone at
Cross-checks against the pinned consumer (read, not run): the wire schema's object/wall/meter/ground/overhead/facing/coverage field names and enums match the bridge's output field-for-field ( What I couldn't verify
Body accuracy otherwise: every numeric claim I could measure matched (14 tests, 1248/17/5 full suite, ruff clean, stacked-on-#270, draft). Refusal-case count is the one mismatch (finding 3). |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Review skipped Codemetrics is installed on this repository, but it hasn't been added to your team yet. To enable AI code reviews for this repo, go to your dashboard and add BaseScanning to your team's repositories. |
[Medium impact] Adds a coordinate conversion bridge for placement scene data. Fix coverage selection and missing ground depths before merging.
|
| for sensitivity_index, sensitivity in enumerate(scene.coverage.sensitivity or []): | ||
| for band_index, band in enumerate(sensitivity.bands): | ||
| band_out(band, f"/coverage/sensitivity/{sensitivity_index}/bands/{band_index}") |
There was a problem hiding this comment.
Coverage alternatives become extra observations
_coverage combines the main bands with every alternative in sensitivity. These alternatives describe coverage under different viewing-angle rules, not extra observations. A ground strip absent under the main rule but present under a looser alternative therefore becomes observed in the output. The placement parser joins these strips, so checks can treat unseen ground as clear.
Select one declared rule, or refuse alternatives the wire scene cannot represent, rather than combining them.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/placement/scene_bridge.py
Line: 599-601
Comment:
**Coverage alternatives become extra observations**
`_coverage` combines the main bands with every alternative in `sensitivity`. These alternatives describe coverage under different viewing-angle rules, not extra observations. A ground strip absent under the main rule but present under a looser alternative therefore becomes observed in the output. The placement parser joins these strips, so checks can treat unseen ground as clear.
Select one declared rule, or refuse alternatives the wire scene cannot represent, rather than combining them.
**Knowledge Base Used:**
- [Analysis API and schemas](https://app.greptile.com/s-gu/-/custom-context/knowledge-base/samgu-nrx/basescanning/-/docs/analysis-api-and-schemas.md)
- [Site analysis solver](https://app.greptile.com/s-gu/-/custom-context/knowledge-base/samgu-nrx/basescanning/-/docs/site-analysis-solver.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if band.out_m is not None: | ||
| record["out_ft"] = _ft(band.out_m) | ||
| observed.append(record) |
There was a problem hiding this comment.
Ground bands lack required depth
The source contract allows a ground band without out_m, but the wire schema requires out_ft for every ground band. The bridge returns that band without a depth, so convert_scene succeeds while the real placement parser rejects its result.
Refuse a ground band without a measured depth using SceneBridgeError; do not guess one. Add a test that checks this case against the root parser.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/placement/scene_bridge.py
Line: 593-595
Comment:
**Ground bands lack required depth**
The source contract allows a ground band without `out_m`, but the wire schema requires `out_ft` for every ground band. The bridge returns that band without a depth, so `convert_scene` succeeds while the real placement parser rejects its result.
Refuse a ground band without a measured depth using `SceneBridgeError`; do not guess one. Add a test that checks this case against the root parser.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if observed: | ||
| out["observed"] = observed |
There was a problem hiding this comment.
Band counts exceed wire limits
The source contract puts no size limit on coverage.bands, but the wire scene allows at most 500 observed bands. The bridge copies them all without merging or checking the count. A valid source with 501 bands therefore converts successfully but cannot be read by the placement server.
Merge equivalent neighboring bands where possible, or return a pointed SceneBridgeError when the output would exceed the limit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/server/placement/scene_bridge.py
Line: 603-604
Comment:
**Band counts exceed wire limits**
The source contract puts no size limit on `coverage.bands`, but the wire scene allows at most 500 observed bands. The bridge copies them all without merging or checking the count. A valid source with 501 bands therefore converts successfully but cannot be read by the placement server.
Merge equivalent neighboring bands where possible, or return a pointed `SceneBridgeError` when the output would exceed the limit.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
What
Adds
backend/server/placement/scene_bridge.py— a pureconvert_scene(document: dict) -> dictthat turns the backendbase-scene/v0scene contract (metres, themeter_anchored/v0frame, pydanticScene-validated) into the placement wire scene the unchanged top-levelPOST /v1/placementsserver accepts (server/schemas/scene.schema.json, feet). This is the pinned consumer interface the placement stage'splacement/scene.jsoninput has been waiting on — until nowbackend/server/placement/adapter.pycould only fall back to a committed reference fixture (#268) because nothing published reconstructed scenes to the solver.Also adds
backend/server/placement/tests/test_scene_bridge.py(14 tests).Mapping
0.3048(correctly rounded division, never a rounded reciprocal multiplier). Both contracts measuresalong the wall chain from the meter, positive to the right seen from outside, so spans convert without rebasing:wire_s_ft = source_s_m / 0.3048.height_ft,plus_minus_ft; no wiresourceguess (the source contract records how wall points were supported, not how the line was found).window/door/garage_door/gas_meter/vent/downspout/pool/batteryidentity,ac_unit→ac,gas_regulator/gas_pipe→gas_meter,disconnect/panel/wall_box/meter_can→elec_box), source kindsvlm/tap/tape,confunchanged (recognition confidence in both contracts),attrscarryingoperable/wellwhen present — a missing attribute stays missing, read as unknown by both sides.coverage.bandsandsensitivity[].bands); an absent band stays unobserved.Refusals
Everything raises
SceneBridgeError, aValueErrorwithcodeand a JSON-pointerpointernaming the offending input:placement_scene_unsupported— valid scene, outside the bounded scope: more than one wall, non-house/non-observed wall roles, generated or inferred evidence on anything, unmapped classes/sources/materials, stand-off wall_rects, heightfield or non-level ground, sloped regions, facinglower_bound, grid coverage, keyframes, generated regions. Nothing is promoted or guessed.placement_scene_invalid— anythingSceneitself rejects, plus cross-checks of every declared s value against the plan geometry at a 1e-6 m float-noise bound: walls_range_mvs the baseline and meter anchor, chain ends at the wall's ends, the meter's wall_rect span centred on s=0, bounded hazard spans inside the wall, reversed spans, non-finite numbers (pointer at the exact list element), negative/inverted heights.Purity
convert_scenereads its argument and nothing else — loads no fixture, reads no environment, calls no solver — never mutates its argument, and returns an equal dict for an equal document on every call (tested, including at depth).Tests
Every expected value in the test module is computed from first principles — metres ÷ 0.3048 written out in the test file, plan geometry with
math.hypot— never from the bridge's own constants. Each converted scene is additionally validated through the real root parser (server/scene.pyviaplacement/adapter.load_solver, the code behindPOST /v1/placements) andscene.schema.json, including a rotated and translated wall with a nonzero s origin, where the test independently recomputes where the hazard span must land in plan coordinates and checks the parser finds exactly that point from the converted feet.Refusal expectations were verified case-by-case against the actual behavior (56 refusal cases): e.g. non-wall_rect geometry kinds are refused upstream by the pydantic discriminator union with the union tag in the error location, and an unknown chain wall reference is caught by the source contract's consistency check before the bridge's own chain check.
Verification
server/eval/tests/test_gates.py), identical on base.ruff checkandruff format --checkclean across the backend.Base: stacked on
obv/burst6-bs-status-evidence(head of #270), like #271. Draft; not merging.