Repository navigation
ios: ground needs a plausible meter height and never rises; estimated meter taps are checked and re-fitted to the detected wall - #108
AidenJohnston wants to merge 10 commits into
Conversation
…he walk Ground (#62): - GroundPlaneChoice.choose: the meter must be 0.9-2.0 m above the plane (replaces the 0.3 m minimum drop). A measured ground is never raised by more than 0.1 m. Among floor-first candidates, the plane under where the phone stood to mark the meter wins, then the one containing the meter's foot, then the lowest. Returns the plane and the reason; refineGround and markMeter log the plane id, class and reason. Meter tap (#69): - LiveCapture passes vertical planes (WallPlaneEvidence) and the meter anchor's id with each frame. - MeterTap.refusal refuses an estimated-plane hit more than 2 m from the phone, turned more than 30 degrees from its view, or more than 0.1 m behind a detected wall/unclassified plane its line of sight crosses first. markMeter stays on the meter step and shows aimAtWallForMeter, whose copy is now "Hold on, finding your wall". - MeterTap.refit / ScanEngine.refitWallToDetectedPlane: during the close-up only, a detected wall plane on the tap's line of sight that disagrees by more than 0.2 m or 10 degrees moves the meter onto it, turns the wall to its normal and re-anchors the meter. ingest skips anchor corrections from frames that carry the old anchor. All thresholds are guesses, named and documented. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (14)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe scan now carries ground and wall plane evidence, selects ground planes using position and height constraints, validates estimated meter taps, and can refit the wall to detected planes. It also tracks meter-anchor corrections and presence across frames. ChangesMeter and ground alignment
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant LiveCapture
participant SourceFrame
participant ScanEngine
participant MeterTap
LiveCapture->>SourceFrame: Add meter anchor ID and plane evidence
SourceFrame->>ScanEngine: Supply frame evidence
ScanEngine->>MeterTap: Check estimated meter hit
MeterTap-->>ScanEngine: Return refusal or no refusal
ScanEngine->>MeterTap: Check for wall-plane refit
MeterTap-->>ScanEngine: Return qualifying refit or no refit
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The alignment changes have no established merge-blocking defect. Complete normal checks and the planned device validation before release. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes are contained to on-device scan alignment and strengthen checks on estimated geometry. A remaining reset-isolation uncertainty could allow an old frame to influence a restarted scan. No new remote access or privilege expansion was established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR implements the main coding behavior in [ Resolution Add packet or scene export records for ground changes and meter tap/plane disagreement. Include the relevant time, plane or anchor identifiers, classifications, and correction or refusal data. Add automated tests for serialization and presence of these records. Implement or track the remaining
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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 |
AidenJohnston
left a comment
AidenJohnston
left a comment
There was a problem hiding this comment.
Adversarial review of PR #108 (U2, head 9c2a567, base t3/ios-mvf at its current tip). I read the full diff, the changed files in full, every call site of the changed symbols, #62 and #69, and the public build 4.1 run 1 and run 3 analyses.
Must fix
None found. I checked the Swift by reading it, not compiling it. SourceFrame's memberwise order still fits all three initializers (LiveCapture pose and sampled frames, ReplayPlayer.deliver). MeterPlaneSource has two cases, so the switch in MeterTap.refusal is exhaustive. The new HouseScanKit API is public wherever the app uses it. describe is nonisolated and uses only Sendable values. The OSLog interpolations use Float and String overloads that exist. The re-fit pose uses the same axes as wallHitPose in SpatialSourceTests. nearlyEqual and portraitCamera exist in TestSupport, and I traced every new #expect by hand. The existing GroundPlaneChoiceTests still pass under the 0.9–2.0 m band (lawn 1.5 m, floor 1.4 m, lower 1.9 m, table and sill rejected). Nothing here has been compiled, though (see Notes).
Should fix
-
The no-raise rule makes the run 3 fix depend on which plane ARKit finds first.
ios/HouseScanKit/Sources/HouseScanKit/Geometry/GroundPlaneChoice.swift:108;ios/HouseScan/Runtime/ScanEngine.swift:389-391.- Scenario: the run 3 shape, two
floorplanes with the ground at the wall 0.25 m above the other. Suppose the lower plane is detected first and reaches the foot line. refineGroundtakes it with reason.lowestand setsgroundMeasured. When the correct plane arrives, it contains the phone, but it is 0.25 m abovecurrentand is filtered out before the containment rules run.- The wrong ground then stays for the whole scan, and
GroundPlaneTests.swift:83pins exactly this. - The PR body says "Taking the lowest plane would have picked the wrong one", and the change fixes that only when the higher plane arrives first.
- Fix: let an
.underThePhoneor.atTheMetersFootcandidate replace a current ground that was chosen as.lowest(keep the chosen reason next togroundMeasured). Or state the order dependence under Risk and add a test for it. Either way, the choice between these belongs to the iOS owner.
- Scenario: the run 3 shape, two
-
The one-shot re-fit can lock onto a small, early
wallfragment.ios/HouseScanKit/Sources/HouseScanKit/Geometry/MeterTap.swift:125-135;ios/HouseScan/Runtime/ScanEngine.swift:416-424.refitaccepts any.wallplane of any size. After the first re-fit,meterPlaneSource = .detectedPlane, so no later re-fit happens.- Scenario: an accurate square-on estimated tap passes the gate. During the close-up, ARKit's first wall fragment appears (about 0.3 m across, near the meter board) with its normal 11° off, as early planes often are.
- The wall is turned 11° the wrong way. The export's line source becomes "detected", which narrows its error, and the later 6 m wall plane can't correct it.
- Fix: require a minimum outline size for a re-fit plane, for example a named-guess constant around 1 m along the wall. Or keep re-fitting to larger planes while the close-up lasts.
-
After a refusal, a lower plane can still replace the ground and un-meet a task (#62, A41-1-04).
ios/HouseScan/Runtime/ScanEngine.swift:389-402.- Only raises are blocked. A lower plane that replaces the ground still runs
coverage.updateWalland re-projects every row. Ground cells the homeowner was just told were done can go amber, which is the run 1 symptom whenever the change goes downward. - Please say under Risk that #62's "don't silently un-cover" is only partly addressed, or keep that part open as a follow-up rather than closing it with
Fixes #62.
- Only raises are blocked. A lower plane that replaces the ground still runs
Notes
- Check 3 of the tap gate will rarely fire on a device.
MeterTap.swift:83-85.raycastVerticalPlanetries.existingPlaneGeometryfirst along the same ray. If the line of sight crosses a detected plane inside its geometry, the hit is already.detectedPlane. So "behind a detected plane" can only fire in the 0.25 mcoverMarginband or in bounding-box notches.- In run 3 the reach was 1.6 m, under the 2 m limit, so the facing check (46° against 30°) is the only one that would have refused that tap.
- A bad estimated plane about 25° off at under 2 m passes all three checks, and only the close-up re-fit remains as a backstop. That is worth one line under Risk.
- The one refusal card gives no useful action for a far tap.
aimAtWallForMeternow servestooFarandturnedFromPhoneas well. A homeowner 3 m away is told to move the phone side to side, not to come closer, and one standing well to the side is not told to face the wall. The brief asked for a single card, so this is not a defect. It is worth counting in the U16 device check along with the 30° gate. - Re-fit ground edge.
ScanEngine.swift:421-423. If no plane qualifies for the re-fitted wall,groundMeasuredis carried over from the old wall, whose foot line was up to 1.1 m away. That old ground then becomes the baseline for the no-raise rule. This is minor. - Stale state.
meterTapCamerais not cleared inresetAll(ScanEngine.swift:1580) or inresetSpatialState. It is harmless today, because it is only read once a newmarkMeterhas set it again. It is cheap to clear alongsidemeterPlaneSource. - Scope.
- The
meterAnchorIDgate iningest(ScanEngine.swift:366) is outside "the plane-change branch only" that the brief allowed. It is justified (stale frames after a re-anchor) and explained in the body, and it doesn't change behavior when no re-fit happens. I checked every path that sets the engine's or LiveCapture's anchor id (markMeter,resetSpatialState,resetAll,restart), and they stay in step. - When U10 lands, its tap-time anchor transform must be reset on this re-fit too. Please mention that in the overlap notes.
- There are no
project.ymlorproject.pbxprojedits, no scene or packet schema change, and the brief's changes of design are all listed under Risk.
- The
- Public-repo rules. The diff, the tests (made-up geometry), the PR body and the commit message contain no names, meter numbers, addresses or private material. The numbers quoted all come from public #62, #69 and PR #60.
- PR body. It has
Fixes #62andFixes #69, the base, what was verified and what needs a device, the files touched, dependencies, the mid-walk re-fit follow-up and the attribution line. - CI.
gh pr checks 108: "iOS build" is skipping (drafts get no iOS run triggered by the PR), the size label passed, and CodeRabbit skipped the draft. Nothing in this PR has been compiled or tested yet. The firstmake iosrun is the real gate before this leaves draft.
Verdict: ready-with-notes
🤖 Generated with Claude Code
Marks already move with the anchor's correction (37729cc); the 4.1 build moved only the meter. This logs every applied correction with the total move and turn since the meter was anchored, so a device walk away and back can tell a corrected map from uncorrected drift (#73). The tracking is cleared on a spatial reset and on Start over. Tests pin that marks keep their anchor-local place and their s, height and out. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
AidenJohnston
left a comment
AidenJohnston
left a comment
There was a problem hiding this comment.
Adversarial review of 9c2a567 (head of t3/41-ground-and-meter-tap, base t3/ios-mvf), against #62, #69, the public run 1 and run 3 analyses, and the unit brief.
I mentally compiled every changed Swift file: the SourceFrame memberwise argument order at both LiveCapture call sites and in ReplayPlayer (meterAnchorID defaults to nil), SIMD4(SIMD3, w) and simd_float4x4 column init, the ARPlaneAnchor.Classification switch (same shape as groundEvidence), the exhaustive MeterPlaneSource switch, public access across the HouseScanKit boundary, nonisolated static describe passed as a function value from the @MainActor engine, and the Self.turned / local turned shadowing in MeterTapTests. I also traced every new test and the five existing GroundPlaneChoiceTests by hand against the new band (meter at 1.5 m means plane y in [-0.5, 0.6]). All of them should pass. Every call site of groundBelow, minimumDrop (none left), SourceFrame( and GroundPlaneEvidence( is updated. The branch merges cleanly into current t3/ios-mvf (4 commits behind, no file overlap).
Must fix
None found.
Should fix
-
The no-raise rule can lock in the wrong floor for the run 3 case the PR says it handles.
GroundPlaneChoice.swift:108andScanEngine.swift:391(current: groundMeasured ? wall.groundY : nil).
Scenario: in run 3 there were twofloorplanes, and the correct one was the higher. The public analysis gives -1.553 m, 7.0 x 3.6 m; the other was -1.806 m, 9.9 x 9.7 m. Say only the larger, lower plane has been detected when the meter is tapped, and it comes within 0.5 m of the foot line.markMeterthen takes it (.lowest) and setsgroundMeasured = true. When the correct plane under the phone arrives, it is 0.25 m higher, so line 108 filters it out before the containment preference runs. The ground stays 0.25 m low for the whole scan. The body's claim that containment picks the right plane in run 3 holds only if both planes exist at tap time, and nothing records detection order.
The rule is there to stop met coverage cells from being un-met, and no cells are kept before the walk:observeCloseUpViewonly adds the close-up view when the close-up step ends. Suggested fix: passcurrentonly once the phase is past.meterCloseUp, or waive the rule for an.underThePhonecandidate while in.findMeter/.meterCloseUp. Add aGroundPlaneTestscase where the lower plane is measured first and the higher plane under the phone arrives during the close-up. At minimum, state the order dependency in the Risk section and add it to the U16 device checks. -
Nothing tracks the rest of #69 once
Fixes #69closes it. This concerns the PR body. The issue's fix direction also asks to "export that disagreement so the server can widen the wall's error or refuse to place a spot", and to re-fit mid-walk. The PR defers both, correctly (U3 / #76 and aCoverageMapchange), but links no issue. Once this merges, #69 closes and the mid-walk re-fit has no tracker. A wall first detected after the close-up still produces run 3's result: the battery inside the house. Open a follow-up issue and link it, or switch to "Part of #69" for those parts.
Notes
ScanEngine.swift:366: the brief said to change only the plane-change branch ofingest, and U10 ownsrefreshMeterFromAnchor. The anchor-id gate is outside that branch. It is needed: without it, in-flight frames with the old anchor's pose read as a correction of about 1 m. It is documented, and the body stays unchanged. #119 (U10) is already stacked on this branch, so a change here needs a rebase of #119. The body says "doesn't depend on any open PR" (true), but should also say that #119 depends on it.MeterTap.swift:125-129: the refit accepts awallcrossing on either side of the tapped point, up to 2 m from the phone. Where a meter is not flush with the house wall (a pedestal, or a board on a garden wall), a tap on an estimated plane at the meter moves the meter back to the house wall behind it: 0.5 m to the meter plus 1 m more is still within reach. That's rare for Base's installs; consider capping how far behind the tapped point the crossing may lie, or listing it as a risk.MeterTap.swift:33and:83: the 0.25 m outline margin applies to small unclassified planes too. A detected porch post or downpipe plane about 0.2 m beside the sight line can refuse a valid estimated tap on the wall behind it. That's for the U16 refusal count.ScanEngine.swift:361-363: the re-fit runs only when the vertical-plane set changes. If a disagreeingwallplane was already stable at tap time, the re-fit waits for the next plane update. ARKit updates boundaries often, so the effect is small. CallingrefitWallToDetectedPlane()once at the end ofmarkMeterwould make it immediate.- The
aimAtWallForMetercopy now also covers thetooFar(> 2 m) refusal, and "move side to side" doesn't say to step closer. It recovers once ARKit detects the plane, because detected hits skip the distance check. That's a copy choice for the iOS owner, matching the brief's one-case decision. meterTapCameraisn't cleared inresetAllorresetSpatialState. That's harmless today, becausemarkMeteralways sets it beforegroundBelowcan run with a wall.- Scope: no
project.ymlorproject.pbxprojchanges, no packet orscene.jsonchanges, and no edits torefreshMeterFromAnchor,publishWall,showResultInCameraorCoverageMap.updateWall. The switch from the brief'sDetectedPlane/planestoWallPlaneEvidence, and keeping "lowest" as the last tie-break, are explained in the body and left to the iOS owner. That's fine. - Public-repo check: the diff, the body and the commit message contain no names, meter numbers or serials, addresses, emails,
private/content or media. The field numbers quoted (46 degrees, 1.1 m, 0.25 m) are all in public issues and analyses. - PR body: it has Fixes #62 and Fixes #69, the base, what was verified versus what is device-only (U16: no LiDAR, a table near the wall, low light), the files touched, dependencies and the attribution line.
- Checks:
iOS buildis skipping because the PR is a draft; drafts get no PR-triggered iOS run. Nothing here has compiled or run, so CI's first run after "ready for review" is the real compile and test gate. Label PR size passed. CodeRabbit skipped the draft.
Verdict: ready-with-notes
🤖 Generated with Claude Code
|
Verdict: ready-with-notes
🤖 Generated with Claude Code |
|
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. |
![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: 9c2a5676ff
ℹ️ 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 !frame.wallPlanes.isEmpty, frame.wallPlanes != wallPlanes { | ||
| wallPlanes = frame.wallPlanes | ||
| refitWallToDetectedPlane() |
There was a problem hiding this comment.
When an estimated tap is accepted after wallPlanes has already been populated, this block invokes the refit only if ARKit subsequently changes that array. A stable plane—for example, one whose outline is just outside the exact raycast geometry but inside coverMargin—therefore never corrects an accepted 10–30° orientation mismatch, and the walk can still be built on the wrong wall. Run the refit once after entering the meter close-up as well as on later plane updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@AidenJohnston Confirmed the cached-plane path at 9c2a567: findMeter caches the plane but cannot refit, and markMeter enters close-up without trying it again. An unchanged plane array then never refits an accepted estimated tap. Please attempt refit once on entering close-up as well as on later updates, with a cached-plane/estimated-hit case. This is conditional on estimated taps near detected-plane edges; no general P1 device failure was established.
| let live, let tapCamera = meterTapCamera, let wall = coverage?.wall, | ||
| let refit = MeterTap.refit(meter: wall.meter, outward: wall.outward, tapCamera: tapCamera, planes: wallPlanes) else { return } | ||
| let along = simd_normalize(simd_cross(-refit.outward, SIMD3(0, 1, 0))) | ||
| let choice = groundBelow(refit.meter, along: along, current: nil) |
There was a problem hiding this comment.
When the wall is refitted after ground has already been measured, passing current: nil disables maximumRaise. If another qualifying plane is substantially higher—for example, a raised surface that still leaves the meter 0.9–2.0 m above it—the refit can replace the measured ground with that plane and shift every exported height, despite the new no-raise guarantee. Pass the existing ground as current whenever groundMeasured is true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@AidenJohnston Confirmed the cap bypass at 9c2a567: measured ground y=0, meter y=1.5, a qualifying floor y=0.3 rejected by refineGround can still win the simultaneous wall-refit lookup with current:nil. Please preserve the measured-ground cap during refit, or define and test an explicit exception when changing walls invalidates that ground. This path runs during close-up, so it does not itself undo completed walk tasks. Send the corrective SHA through S3.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let flat = SIMD3(outward.x, 0, outward.z) | ||
| guard simd_length(flat) > 1e-3 else { return nil } | ||
| var nearest: (plane: WallPlaneEvidence, crossing: SightCrossing)? | ||
| for plane in planes where plane.kind == .wall { |
There was a problem hiding this comment.
LiveCapture labels them unclassified and this filter excludes them all. An estimated meter tap can pass the initial checks, but detecting the real wall during the close-up will never correct it, leaving subsequent scan measurements based on the estimated wall.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScanKit/Sources/HouseScanKit/Geometry/MeterTap.swift
Line: 125
Comment:
**Unclassified walls cannot refit** If a phone reports detected vertical planes without a wall classification, `LiveCapture` labels them unclassified and this filter excludes them all. An estimated meter tap can pass the initial checks, but detecting the real wall during the close-up will never correct it, leaving subsequent scan measurements based on the estimated wall.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| let measured = choice != nil || groundMeasured | ||
| guard setWall(meter: refit.meter, outward: refit.outward, groundY: choice?.plane.y ?? wall.groundY, groundMeasured: measured) else { return } |
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScan/Runtime/ScanEngine.swift
Line: 422-423
Comment:
**Refit keeps outdated close-up** If a wall plane arrives after the close-up photo is taken but while the homeowner is still reviewing the meter number, this refit replaces the wall without invalidating the saved photo, pending read, or credited camera view. The scan can then use a close-up taken against the old meter position for coverage on the new wall.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
@AidenJohnston The narrower reproducible source path is lost close-up coverage after confirmation. At beta 2db95af, chooseMeterNumber consumes closeUpCredit into coverage at Actions.swift:106-112/133-136, then finishCloseUp leaves the phase at meterCloseUp during its delayed transition. A plane update can still refit at ScanEngine.swift:539-545 and setWall replaces that credited map at1490. The accepted photo remains but its observation is not replayed. Please preserve/replay the accepted close-up view when refitting during that interval, with coverage for confirm then refit before wallWalk. Retaining the photo before confirmation is not itself shown wrong because its world-space camera is evaluated against the new map. Treat this as conditional P2 coverage loss, not a demonstrated stale-photo or OCR failure. No device reproduction or frequency estimate.
The base's anchor repair (0cf2125) replaced refreshMeterFromAnchor and refineGround with SpatialUpdate, and made SourceFrame.groundPlanes optional. This PR's pieces now sit on it: - SpatialUpdate.apply takes where the phone stood at the tap: the ground choice prefers the plane under it (GroundPlaneChoice.choose) and keeps the no-raise rule, and a correction moves the phone position with the wall, since it was recorded in the old frame. The log names the chosen plane. - The anchor-id gate now guards the anchor passed to SpatialUpdate. - A re-fit anchors the meter again without dropping the correction log (MeterAnchorTracking.anchorAgain), so the close-up photo is still corrected for moves made before the re-fit. - SourceFrame.wallPlanes is optional like groundPlanes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…the tap - markMeter tries the re-fit once on entering the close-up. Before, it ran only when the vertical planes changed, so a wall plane already known at the tap never corrected an accepted estimated hit turned 10 to 30 degrees from it (Codex, Sam and Greptile on #108). - MeterTap.refit skips a wall plane the line of sight crosses more than refitBehindTap (0.3 m, a guess) past the tapped point. A tap accepted in front of a detected plane is a meter standing proud of its wall; pulling it back to the wall behind a pedestal was wrong (Greptile). The field run's bad hit lay behind the real wall, which this doesn't limit. - meterTapCamera is cleared on resetAll, resetSpatialState and when a failed source is released, with the wall planes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The no-raise rule stops a higher plane from re-projecting coverage rows the homeowner was told were done (#62). Before the walk keeps a view there are no such rows, and the rule only made the choice depend on which plane ARKit found first: a lower floor reaching the foot line first locked out the floor under the phone, 0.25 m higher in the field run. SpatialUpdate.raiseLimit is the measured ground once coverage has kept a view, nil before. Every ground update uses it, and so does the re-fit's ground lookup (Codex and Sam on #108: the re-fit passed nil unconditionally; it now shares the one rule, which is nil during the close-up by definition). GroundUpdateTests: the floor under the phone raises the ground before the walk; after a kept view the same plane is refused. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size:XL
#108 now carries t3/ios-mvf's anchor repair, where SpatialUpdate applies the anchor correction and refreshMeterFromAnchor is gone. The drift log moves with it: - applySpatialUpdate writes the correction line once the correction has moved the wall and the marks; the step is how far the meter moved. - MeterAnchorTracking keeps both parts: the base's correction log (for poses captured earlier) and this PR's totals since anchored. A re-fit's anchorAgain starts the totals from the re-fitted pose, so the re-fit's own move is not counted as a correction, and keeps the log. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… on a missing line Review of #119 (Part of #73); the same change as beta's 1b3229a, on the SpatialUpdate structure: - The correction lines are .notice, so `log collect` and Console without "Include Info Messages" both show them, and they carry the capture clock (t) and the phone's distance from the meter, to line them up with the packet and the video. - One line when the frames stop carrying the meter's anchor, and one when they carry it again (MeterAnchorPresence). - A summary on every phase change once the meter is anchored, with zero corrections too, and always on sending the scan and opening the AR result: corrections since anchored, frames with and without the anchor, the latest frame's sighting. A re-fit starts the counts again with the totals. Test: AnchoredMarksTests.anchorPresenceReportsOnlyChanges. 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a6a159138
ℹ️ 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".
| } else if ground.measured { | ||
| ground.set(measured: false) | ||
| map.heightError = ground.guessError |
There was a problem hiding this comment.
When observed coverage exists and the only qualifying plane is more than maximumRaise above the measured ground, choose returns nil here and this branch clears ground.measured. On the next sampled frame, raiseLimit therefore returns nil, so the same high plane is accepted and raises the ground anyway. This occurs when the original floor anchor disappears or is replaced by a higher plane, and it shifts exported heights despite the no-raise guarantee; retain the prior raise bound or keep the measured ground when candidates were rejected solely by that bound.
Useful? React with 👍 / 👎.
| // No plane qualifies at the moved wall: the old ground stays for now, and the next frame's | ||
| // planes measure it again for the new wall or put it back to a guess (`SpatialUpdate`). | ||
| let measured = choice != nil || groundMeasured | ||
| guard setWall(meter: refit.meter, outward: refit.outward, groundY: choice?.plane.y ?? wall.groundY, groundMeasured: measured) else { return } |
There was a problem hiding this comment.
If a wall plane first triggers a refit during the auto-advance delay after chooseMeterNumber has called observeCloseUpView, the phase is still meterCloseUp, so this call replaces the populated CoverageMap with an empty one. The consumed closeUpCredit cannot be replayed afterward, and the walk starts without the accepted close-up's wall, ground, or depth evidence, potentially causing unnecessary missing-view requests. Rebuild the map while retaining that observation, or prevent this late refit window from discarding it.
Useful? React with 👍 / 👎.
|
Updated for review. Head is now Base merge
Tests: Review items
CIThe last run before this (36304552823) failed On Before pushing, I checked each commit on the Mac mini. HouseScanKit passed 450 tests, including the new ones, the xcodegen drift check passed, and the device build passed. A full Sam's review-clear is still needed. The device-only checks for U16 still stand: the 30° gate, far taps, and runs without LiDAR. 🤖 Generated with Claude Code |
Conflicts: SpatialUpdate.Outcome keeps both groundChoice and the base's `changed`; ingest re-fits to the wall planes before completing an upload snapshot, so a snapshot agrees with any re-fit made on the same frame. A re-fit goes through setWall, which already bumps spatialRevision. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ios: log meter-anchor corrections so marks can be checked against drift
|
@AidenJohnston Current #108 head |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@greptileai review Please review the current head |
| public static func raiseLimit(map: CoverageMap, ground: GroundEvidence) -> Float? { | ||
| ground.measured && !map.observedCameras.isEmpty ? map.wall.groundY : nil |
There was a problem hiding this comment.
During the walk, if the measured ground plane disappears or is reclassified while a plane more than 0.1 m higher remains, the higher plane is rejected and the ground becomes a guess. On the next frame, raiseLimit returns nil because the ground is no longer measured, so that same higher plane is accepted. The ground can rise after coverage has already been observed, changing reported heights and invalidating coverage the homeowner was told was complete.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScanKit/Sources/HouseScanKit/Geometry/SpatialUpdate.swift
Line: 51-52
Comment:
**Ground raise limit can reset**
During the walk, if the measured ground plane disappears or is reclassified while a plane more than 0.1 m higher remains, the higher plane is rejected and the ground becomes a guess. On the next frame, `raiseLimit` returns nil because the ground is no longer measured, so that same higher plane is accepted. The ground can rise after coverage has already been observed, changing reported heights and invalidating coverage the homeowner was told was complete.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Caretaker check at 5be6bd5: same defect as Sam's 4115100458 and Codex 4115136033, still open at this head. One frame without the measured ground clears measured (SpatialUpdate.swift:97-100), so on the next frame raiseLimit is nil (lines 51-52) and the higher plane is accepted (lines 85-94). Meter height then exports as 1.2 m instead of 1.5 m, with no ground uncertainty. The base already allowed this late rise; this PR's cap doesn't survive the frame without measured ground. Tracking it on Sam's request.
| guard simd_length(flat) > 1e-3 else { return nil } | ||
| let reach = simd_distance(meter, tapCamera) | ||
| var nearest: (plane: WallPlaneEvidence, crossing: SightCrossing)? | ||
| for plane in planes where plane.kind == .wall { |
There was a problem hiding this comment.
On a phone without plane classification, a detected vertical wall is reported as .unclassified, but this loop considers only .wall. If an estimated meter tap is accepted before the real wall is detected, detecting that wall during the close-up will not correct the meter or wall position. The scan can continue using the incorrect estimated wall.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScanKit/Sources/HouseScanKit/Geometry/MeterTap.swift
Line: 134
Comment:
**Unclassified walls cannot trigger refit**
On a phone without plane classification, a detected vertical wall is reported as `.unclassified`, but this loop considers only `.wall`. If an estimated meter tap is accepted before the real wall is detected, detecting that wall during the close-up will not correct the meter or wall position. The scan can continue using the incorrect estimated wall.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Caretaker check at 5be6bd5: not a defect for supported phones. The app targets iOS 26 (project.yml:11-12), so every supported iPhone has an A13 or later, and plane classification needs an A12, not LiDAR. Vertical-plane detection is on regardless of mesh support (LiveCapture.swift:75). Excluding .unclassified is deliberate, because unclassified planes can be meter boards, fences or posts (MeterTap.swift:126-128; Aiden, 5855939862), and MeterTapTests asserts it.
There was a problem hiding this comment.
That’s a fair correction. With the iOS 26 device floor, the supported phones have the hardware needed for ARKit plane classification, and vertical-plane detection is enabled independently of mesh support. More importantly, excluding .unclassified here is an intentional safety choice: a .none classification can represent a board, post, or fence, so treating it as the wall could create a worse refit. The existing test locks in that behavior. I’m withdrawing this finding; classified .wall planes remain the supported refit path.
This branch has not been deployed
2 participants
Fixes #62
Fixes #69
Base:
t3/ios-mvf. This PR doesn't depend on any open PR.What changed
In the build 4.1 field test, a phone without LiDAR built whole scans on the wrong reference twice. In run 1 the ground was taken from a folding table, so every height came out about 3 ft too high. In run 3 the meter tap landed on an ARKit estimated plane about 1.1 m behind the real wall and turned 46° from it, and nothing corrected it afterwards. This PR changes how the app picks the ground plane, checks meter taps on estimated planes before accepting them, and moves the wall onto a detected wall plane before the walk starts.
The brief for this work was written against older code.
t3/ios-mvfalready has a class filter for the ground (GroundPlaneChoice, from 37729cc): furniture is excluded,floorwins, then the lowest plane that reaches the wall's foot. This PR builds on that filter rather than adding a second chooser.Ground plane (#62)
GroundPlaneChoice.choosenow applies three more rules:camera.y - 1.4) withgroundMeasuredfalse.refineGroundaccepted any change over 1 cm, so a higher plane arriving late re-projected coverage and un-met a ground task the homeowner had just been told was done.floorplanes, and the other lay 0.25 m below it. Taking the lowest plane would have picked the wrong one.refineGroundandmarkMeternow log the chosen plane's ARKit id, its class and the reason it was chosen. Nothing about the ground is added to the packet orscene.json.Meter tap on an estimated plane (#69)
LiveCapturenow sends detected vertical planes with each sampled frame (WallPlaneEvidence). Before, the engine never received them.MeterTap.refusalrefuses a hit on an estimated plane in three cases:wallor unclassified plane that the phone's line of sight crosses first, within that plane's outline.Hits on detected planes are accepted as before. A hit in front of a detected plane is never refused, because a meter stands proud of its wall. On a refusal,
markMeterstays on the meter step, showsaimAtWallForMeterand logs the reason. That card used to say "Step a little closer to the wall", which in run 3 prompted the second, bad tap. It now says "Hold on, finding your wall" / "Move your phone slowly side to side, then aim at your meter again."Re-fit before the walk (#69)
ScanEngine.refitWallToDetectedPlaneruns whenever the vertical planes change, but only during the meter close-up and only when the meter was placed on an estimated plane.MeterTap.refitlooks at the line of sight from where the phone stood at the tap, through the meter. It takes the first detectedwallplane that line crosses within 2 m and within the plane's outline. If that plane disagrees with the tap by more than 0.2 m or 10°, the refit does the following:removeAnchor+addMeterAnchor);Frames already in flight still carry the old anchor's pose.
SourceFramenow carries the anchor's id, andingestpasses a frame torefreshMeterFromAnchoronly when that id matches the current anchor. Without this check, a stale frame would read as a 1 m "correction" of the new anchor.A re-fit during the walk is out of scope and left as a follow-up.
CoverageMap.updateWallcan't turn a wall it has already seen, so that needs a coverage method that rebuilds with a new direction and drops ends and corners. A wall found wrong after the close-up still stays wrong.Why
Both bugs corrupt the geometry every later step builds on, and without LiDAR the rest of the scan has nothing to catch the error. Every new threshold (0.9–2.0 m, 0.1 m raise, 2 m, 30°, 0.1 m behind, 0.25 m outline margin, 0.2 m / 10° re-fit) is a named constant documented as a guess. None of them is measured.
Verification
make iosis the first compile and test run.groundBelow,SourceFrameinitializers inLiveCaptureandReplayPlayer,meterAnchorID,detectedWallPlanes,meterTapCamera) and checked Swift 6 isolation.describeisnonisolatedbecause it is passed as a function value.GroundPlaneTests: a tabletop 0.5 m under the meter loses to the floor, even unclassified and under the phone; of two floors, the one under the phone beats a lower one; the meter's foot comes next; planes that put the meter 0.4 m or 2.5 m up are rejected; the no-raise rule holds; without classification every plane is a candidate.MeterTapTests: an estimated hit turned 46° and 1.1 m behind a detected wall is refused; one 2.1 m away is refused; square-on hits at 0.5 m, and on the meter's face in front of the wall, are accepted; detected-plane hits are accepted; door planes, and planes the line of sight passes beside, don't refuse. The re-fit flips just past 0.2 m and 10°, and a run-3-shaped tap is moved onto awallplane but not onto an unclassified one.GroundPlaneChoiceTestsinSpatialSourceTests.swiftshould still pass unchanged. I traced each case by hand.ScreenStatesUITestsdoesn't assert the old copy, so it isn't changed.Needs a device (U16 runbook). The ARKit class filter and the vertical planes exist only on a phone, and replays carry no planes. Run these checks on a phone without LiDAR, with a table near the wall, in low light:
floorplane and the heights look right.Risk
scene.json. That is a contract decision tied to An unwalked, tapped wall still gets a spot (here inside the house) with wall backing and cable route passing #76.ScanEngine.swift,ScanEngine+Actions.swift,LiveCapture.swiftorScanCopy.swift. None of them changesingest,refineGround,markMeter,groundBelow, the plane mapping insession(_:didUpdate:)or theaimAtWallForMetercopy, so conflicts should be textual only.Files touched:
ios/HouseScan/Runtime/LiveCapture.swiftios/HouseScan/Runtime/SourceFrame.swiftios/HouseScan/Runtime/ScanEngine.swiftios/HouseScan/Runtime/ScanEngine+Actions.swiftios/HouseScan/UI/Copy/ScanCopy.swiftios/HouseScanKit/Sources/HouseScanKit/Geometry/GroundPlaneChoice.swiftios/HouseScanKit/Sources/HouseScanKit/Geometry/MeterTap.swift(new)ios/HouseScanKit/Sources/HouseScanKit/Geometry/WallPlaneEvidence.swift(new)ios/HouseScanKit/Tests/HouseScanKitTests/GroundPlaneTests.swift(new)ios/HouseScanKit/Tests/HouseScanKitTests/MeterTapTests.swift(new)No project regeneration is needed:
HouseScan/is a synced folder and HouseScanKit is a SwiftPM package.Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit