Repository navigation
recon: handoff to the server team - #20
Conversation
…riteria Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…o the server team The worker turns a scan bundle or Measure Lab session into a GLB mesh, fitted wall and ground, occlusion-aware coverage and an updated scene.json posted to the placement server. On ETH3D electro it claims 0.46 ft (laser as LiDAR) and 0.39 ft (MoGe-2 rescaled with the poses) of wall no photo saw, under the pre-registered 0.5 ft. HANDOFF.md collects everything measured overnight for the server team. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (27)
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 |
…ts own error; content-keyed MoGe cache - coverage.seen_by: a frame sees a wall or ground sample only when its depth there lies within the tolerance of the sample's depth. A farther depth only showed nothing occludes the sample, so an opening bridged by the wall line could export as observed wall. - coverage.wall_and_ground: the ground grid ends at 3.048 m, so the 10 ft level is reachable. - scene.build: the reconstructed wall is source "mesh" with the depth source's plus_minus_ft (MoGe-2 0.56 ft, LiDAR the server's 0.5 ft mesh default), replacing a tap's source and bound. - eth3d.false_observed: a pixel with no laser return is unseen, not seen. - depth.moge: the cache folder is a hash of every frame's image bytes, pose and intrinsics and the model script, lockfile and preprocessing; a stored key that differs is recomputed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ixes Both rows still pass the 0.5 ft bar. Laser as LiDAR: claimed 22.5 of 31.9 ft (was 25.5), false-observed 0.07 ft (was 0.46). MoGe-2: claimed 12.8 of 15.8 ft (was 14.8), false-observed 0.39 ft (unchanged). Geometry rows are unchanged. The oracle now counts a pixel with no laser return as unseen, which can only raise false-observed for a fixed claim, so the laser row's drop comes from the worker no longer claiming 10.15-10.48 ft once each sample needs a surface there. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Walk-to-walk spread is disagreement between ARKit and the reference, not a bound on ARKit's error. - The modern-phone pose setting is labelled synthetic (2% scale, 1 cm, 0.1 degrees; evals/ar_poses.py) wherever it appears. - Answer 3 and 4 numbers follow the evals rescoring on comparable seed groups. - Acceptance lines follow the rerun after the coverage and oracle fixes. - Section references point at experiments/evals/METHODS.md. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
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. |
![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: c6a8be3ef9
ℹ️ 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".
| CELL_M = 0.1524 # half a foot, the app's cell | ||
| WALL_TOP_M = 1.9812 # 6.5 ft, the headroom the export's wall band claims | ||
| WALL_ROWS = 9 # about 0.25 m apart, so a box mounted between rows cannot slip through | ||
| GROUND_MAX_M = 3.048 # 10 ft out |
There was a problem hiding this comment.
Ground sampling stops at 10 ft, and observed() consequently cannot emit evidence beyond that distance, while docs/00-overview.md states that pool and driveway checks require up to 13 ft of photographed ground. Those checks therefore remain UNSURE even when the capture contains suitable views between 10 and 13 ft; obtain the required range from the rules rather than fixing this rule-dependent limit in reconstruction code.
AGENTS.md reference: AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed completeness gap, with an important qualification: a fixed 13 ft cap is not a general repair. The server requests D + rule clearance + position error, so even the public pool rule exceeds 10 ft before drift. This currently produces UNSURE rather than a demonstrated unsafe pass. The manager must settle a rule/request-driven range contract with the server owner; do not substitute another unexplained reconstruction constant.
There was a problem hiding this comment.
Not fixed here, by the manager's decision. The limit errs toward UNSURE, never toward a pass: ground past 10 ft is simply not reported as observed. A 13 ft constant would be another unexplained number, because the server's request is D + r + e, which already exceeds 12.3 ft for the public pool rule before drift. The reach should come from the server's request.
1c2686b records this in recon/HANDOFF.md, section 5, as a known limit for the server team, who own that request.
There was a problem hiding this comment.
Re-reviewed at1c2686b. Accepted as an explicit scope deferral, not a code fix. The manager directed this PR to retain10ft reach and document the conservative UNSURE outcome. HANDOFF now states the server needs D+r+e, explains the pool limitation, and assigns the future requested-range contract to the server team. Recon CI reports38 passing tests plus lint and formatting.
…AR, visible-view scoring - scene.json: the fitted wall carries source mesh (LiDAR) or plane (photos only) and no plus_minus_ft, so the server applies its per-source default plus drift. An explicit bound switched the drift off (0.5 ft at 30 ft instead of 5.3 ft). - Facing gap and clear distance are measured from the reconstructed wall front, not the plane. - A keyframe saved without LiDAR depth gets none; the other frames keep their LiDAR. - The ground is kept as a plane and evaluated at the meter's foot. - choose_wall picks the matching stretch whose extent is nearest the meter. - ETH3D scores each laser point from the nearest view that sees it by the laser's visibility. Rerun: laser pair p90 2.1 -> 1.8 in, MoGe-2 2.8 -> 1.5 in (a different stretch of the same wall); false-observed 0.07 and 0.00 ft. - A refused site plan raises, and stale result.json and site-plan.svg are removed first. - HANDOFF: the 10 ft ground reach as a known limit, evals intervals after the seed-group repair. - docs/04: MoGe-2 weights at the pinned revision are MIT per Hugging Face's metadata. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c2686b03f
ℹ️ 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".
| GROUND_MAX_M = 3.048 # 10 ft out | ||
| FACING_MAX_M = 6.0 | ||
| FACING_BAND_M = (0.3, 1.8) | ||
| OVERHEAD_OUT_M = (0.1, 0.56) # above the battery's footprint (1.83 ft deep) |
There was a problem hiding this comment.
For any configured battery deeper than 1.83 ft, such as the older 3 × 3 ft units documented in docs/00-overview.md, the worker scans only the first 0.56 m and can report overhead clearance while an obstruction intersects the unsampled outer footprint; the initial 0.1 m next to the wall is also never tested. The server cannot detect this mismatch because the emitted clearance does not describe the sampled footprint, so pass the selected model's footprint from the sourced rules instead of hardcoding this range.
AGENTS.md reference: AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed P1 with model dependence. Overhead samples omit the first0.10m and stop near0.56m, which does not cover a deeper selected battery. A low eave in an unsampled strip can be missed. The manager directed a Known defect entry for this draft handoff; no generic footprint repair is claimed.
There was a problem hiding this comment.
Recorded, not fixed, in d072729: recon/HANDOFF.md section 6, "Known defects", second entry. It gives recon/coverage.py:42, 231-242, a repro (an eave 0.05 m out, or 0.7 m out over a 3 ft deep battery, goes unfound), and what a fix needs: the footprint of the battery the chosen rule places, passed from the server's rules or request.
There was a problem hiding this comment.
Resolved as a documented draft-handoff deferral, not a code fix, under the manager's explicit decision. At d072729, HANDOFF.md section 6 lists this confirmed defect, its source locations, a reproduction and the needed future repair. It explicitly says none of these defects is fixed. This unused worker remains draft for Sam or Hunter to decide; the known behavior is not accepted for production placement decisions.
| # 1 to 3 m spans at an assumed 2% pose error, 6.7 in (experiments/evals README section 3 on | ||
| # t3/evals). LiDAR: no measured value exists here; 0.5 ft is the server's default for mesh | ||
| # measurements (errors.mesh_ft, a day-1 estimate). | ||
| PLUS_MINUS_FT = {"moge2-triangulated": 0.56, "lidar": 0.5} |
There was a problem hiding this comment.
In the photos-only path, this explicit 0.56 ft bound is attached to facing gaps and overhead clearances, but it comes from the upper interval for wall pair error rather than the edge measurements used to locate those clearances. The repository's measured evidence says edges remain at 8 in or worse, while 0.56 ft is only 6.72 in; because the server treats plus_minus_ft as the uncertainty when deciding PASS, FAIL, or UNSURE, borderline photo-derived clearances can receive a definitive result with an understated error bar. Use the measured edge-error bound or leave the measurement unresolved until one is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed P1, but substituting an edge-pair percentile is not a justified numerical repair. The0.56ft comes from surface-interior pair-error p90 CI; neither that nor the edge-pair CI calibrates the fused-volume clearance estimator. Final server903d86f can PASS7.10±0.56ft against6.50ft. Omitting the bound defaults to0.5ft and does not solve this. The manager directed a Known defect entry until an accepted estimator uncertainty model or unresolved gate exists.
There was a problem hiding this comment.
Recorded, not fixed, in d072729: recon/HANDOFF.md section 6, "Known defects", fourth entry. It gives recon/pipeline.py:29-35 and states the bound's source: the surface pair-error p90 interval, not a calibration of the fused boundary. It quotes surface 5.64 in [4.63, 6.66] and edges 9.65 in [4.93, 15.01]. The repro is a photos-only overhead clearance of 7.10 ft passing the 6.5 ft headroom rule. A fix needs the clearance estimator's own measured uncertainty, or a gate that returns the check unresolved. No edge percentile was substituted.
There was a problem hiding this comment.
Resolved as a documented draft-handoff deferral, not a code fix, under the manager's explicit decision. At d072729, HANDOFF.md section 6 lists this confirmed defect, its source locations, a reproduction and the needed future repair. It explicitly says none of these defects is fixed. This unused worker remains draft for Sam or Hunter to decide; the known behavior is not accepted for production placement decisions.
…efects - capture.inside refuses an img, depth.file or depth.confidenceFile that is absolute, has a `..` component, or resolves outside the manifest's folder through a symlink (UnsafePath), in both the scan bundle and the Measure Lab adapter. Tests cover all three references in both formats. - HANDOFF: a Known defects section for four confirmed review findings, links to issues #95 and #96, and LiDAR's reach cost corrected to 8.9 ft. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d072729bcd
ℹ️ 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".
- capture.frame_id refuses a keyframe id that is not one file-name component (letters, digits, `.`, `_`, `-`; not `.` or `..`) with BadFrameId, in the scan bundle and the Measure Lab adapter. The MoGe-2 cache names files after ids, so `/tmp/x` or `../../x` wrote outside it. - HANDOFF known defects: partly occupied slabs counted as clear, and LiDAR without a confidence map fused unfiltered. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ource lines - depth.cache_file refuses an upright JPEG or depth NPZ path that does not resolve directly inside the cache folder (UnsafePath), a second check behind capture.frame_id for any Capture built without the readers. - HANDOFF: the two new defect entries cite coverage.py:223,235 and depth.py:90-92. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 294c0fd88f
ℹ️ 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".
Depth maps and poses are keyed by id while coverage walks every frame record, so two keyframes sharing an id let one depth view count from two camera positions and turned two-view coverage from false to true. Capture.__post_init__ now raises DuplicateFrameId naming the id, for both readers and any capture built directly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74f7a98f52
ℹ️ 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".
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| break | ||
| if hits == 0: | ||
| facing_clear[c] = out - front | ||
| oo = front + np.arange(OVERHEAD_OUT_M[0], OVERHEAD_OUT_M[1] + 1e-9, step) |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: recon/recon/coverage.py
Line: 231
Comment:
**Overhead strip misses obstructions** The overhead test samples only 0.10–0.56 m in front of the wall, rather than the entire area a placed battery can occupy. An eave close to the wall or an obstruction farther out over the battery is missed, so the cell can be reported as having clear headroom despite that obstruction.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| # 1 to 3 m spans at an assumed 2% pose error, 6.7 in (experiments/evals README section 3 on | ||
| # t3/evals). LiDAR: no measured value exists here; 0.5 ft is the server's default for mesh | ||
| # measurements (errors.mesh_ft, a day-1 estimate). | ||
| PLUS_MINUS_FT = {"moge2-triangulated": 0.56, "lidar": 0.5} |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: recon/recon/pipeline.py
Line: 35
Comment:
**Clearance error is uncalibrated** Photos-only facing and overhead measurements receive a ±0.56 ft bound derived from errors over surface-point spans, not from the accuracy of the reconstructed clearance boundary. A measured 7.10 ft overhead clearance is therefore treated as clearing a 6.5 ft minimum after subtracting 0.56 ft, although that metric does not establish the boundary’s error. Calibrate the clearance estimator or leave these checks unresolved.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
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 house-scanning-master to your team's repositories. |
Widen ruff ignores so a10 and e2e experiment folders lint like the other breadth paths.
2 participants
What changed
Adds
recon/, a reconstruction worker, andrecon/HANDOFF.mdfor the server team, who now own photos to 3D model to criteria.The worker takes the app's scan bundle (
scene.jsonand keyframe photos with AR poses) or a Measure Lab session. It builds depth per keyframe: LiDAR when at least two keyframes carry it, otherwise MoGe-2 rescaled per photo by features triangulated with the AR poses. A keyframe that a LiDAR capture saved without depth gets no depth, so what only it saw stays unobserved instead of being filled in by a coarser source. The worker then fuses the depth into one volume and writes:model.glb, in the meter's frame);geometry.json, feet);coverage.json);scene.json, posted to the placement server, with its answer and site plan saved. A refused site plan fails the run, and the previous run'sresult.jsonandsite-plan.svgare deleted before posting.make run BUNDLE=… OUT=…runs it. The manifest is untrusted: the worker refuses any image, depth or confidence file it names that lies outside the manifest's folder, whether by an absolute path,..or a symlink. Keyframe ids name cache files, so an id must be a single file name, and each cache file must resolve inside the cache folder.Coverage counts a cell as observed only when, from two positions, the frame's own depth puts a surface at the cell and the fused volume shows nothing in front of it. A depth that is merely farther, as through a window or an open gate, doesn't count. That is the depth test the evals found necessary: the app's coverage map claims wall no photo saw (PR #12, METHODS.md section 7). Facing gaps and clear distances are measured from the reconstructed wall's front, so in front of a pilaster they start at its face.
When the worker replaces the tapped wall with the reconstructed one, it labels the wall's
sourceasmesh(LiDAR) orplane(photos only) and sends noplus_minus_ft. The server then applies that source's default error plus its drift per foot walked from the meter. An explicit bound would switch the drift off: a LiDAR wall 30 ft from the meter would keep 0.5 ft instead of 5.3 ft.The ground is fitted as a plane and evaluated at the meter's foot. When RANSAC splits a wall at a doorway into collinear stretches, the worker takes the one nearest the meter.
The handoff collects what was measured, what to reuse with exact commands, the pitfalls, recommendations for the 3D path, the interfaces, and one known limit for the server team: ground is sampled only 10 ft out, while the public pool rule asks for at least 12.3 ft, so that check stays UNSURE. Its "Known defects" section lists six confirmed review findings that remain unfixed, each with a file and lines, a repro and what a fix needs. Issues #95 and #96 track two more.
Known defects: Unbounded ZIP processing and EXIF geometry. Both remain unfixed in this draft worker, which the app and demo do not use.
Verification
make accepton ETH3D electro against its laser scan (results). The 0.5 ft coverage bar was committed before the first acceptance run (b875f05).Each laser point is now scored from the nearest photo in which the laser shows it unoccluded, not the nearest photo regardless. The laser path's wall did not move, so its 2.1 to 1.8 in change is that fix alone. Both paths fit the same wall. The MoGe-2 path now fits a 20.5 ft stretch of it instead of 15.7 ft, because fitting the ground as a plane moved RANSAC's split, so its row mixes both causes. Two runs gave identical numbers.
The server's own parser on t3/server (14dd051) accepts
scene.jsonbuilt by the worker from both ETH3D depth paths. It reads the LiDAR wall asmeshat 0.50 / 2.10 / 5.30 ft error at 0 / 10 / 30 ft, and the photos-only wall asplaneat 0.75 / 2.35 / 5.55 ft.At c6a8be3 the ADVIO replay and the app's Simulator bundle each ran end to end to a server result (
manual_review); they were not rerun at this head. The Simulator bundle needs--move-meter, because its replayed wall has no reconstructed surface within 4 ft.make test: 81 unit tests with ruff, run by the path-filteredRecon checksjob. They cover the adapters, depth helpers, fusion geometry and normals, the coverage rule's surface requirement and 10 ft ground reach, the wall's source with no bound, facing distance from a proud face, per-frame LiDAR, the ground plane on asymmetric sloped support, the wall stretch past a doorway, the site-plan refusal, the acceptance view choice, manifest paths that escape the packet (absolute,../and symlink, for each file field in both formats), unsafe keyframe ids and cache files outside the cache folder, the content-keyed MoGe-2 cache and the GLB format. Each new test fails when its fix is reverted.Peak memory at this head: 1.04 and 1.13 GB for the two acceptance runs, with MoGe-2's depth cached so the model did not run. MoGe-2 inference itself peaked at 2.95 GB at 640 px in an earlier run.
Risk
Checklist
recon/, a path-filtered.github/workflows/recon.yml, and one licence row indocs/04-prior-art-and-codes.md🤖 Generated with Claude Code