Repository navigation
Split corner-crossing marks, share the ground entry cap, aim at one missing run - #154
Conversation
…issing run - Result view: a fence or driveway that crosses a corner is drawn in parts, one per wall piece, instead of wholly on its midpoint's piece. - The gap planner reads each band through SceneExport.observedBudget, which leaves one ground entry per corner as the export does, so a request can't settle locally on a reach the export joins down. Tested on a cornered chain at the cap. - A lagging band's aim picks one contiguous missing run: cells where the band is already done split the runs, so the aim can't land on done ground between two gaps. Fixes #129 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
|
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: 9d0a662778
ℹ️ 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".
| var runs: [[Int]] = [[]] | ||
| for index in indices { | ||
| if isDeferred(band, at: middleOf(index)) { | ||
| if isDeferred(band, at: middleOf(index)) || done(coverage.level(band, index)) { |
There was a problem hiding this comment.
When the newly separated run has exactly needed (three) cells and is bordered by .skipped cells, this selects its midpoint even though an aim task always checks a fixed 0.6 m window. With 0.1524 m cells, that window covers five cells; after the homeowner covers all three cells in the selected run, isSatisfied sees only 3/5 covered because the skipped boundary cells are not coverage, below the 0.8 threshold, so the instruction cannot complete without re-observing an area the homeowner already marked inaccessible and will instead stall or reappear. Keep the task span within the selected run or require a run wide enough for the fixed aim window.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@AidenJohnston Independent six-file review plus lead source tracing confirm the three-cell case at 9d0a662. Missing cells14-16 center at s=2.3622m; the fixed ±0.3m aim window overlaps13-17. If13 and17 are skipped, covering all three accessible cells reaches3/5=0.6, below0.8. The instruction can leave via the20-second stall timeout, so this is stalled completion rather than a permanent lock. Please keep the requested/settled span within the selected accessible run, or select a satisfiable window, and cover this three-cell case without counting skipped ground as observed. The four-cell changed test misses it. Shared export budget and split rendering had no additional concrete finding in this review; physical behavior is unverified.
There was a problem hiding this comment.
Fixed in 3ec22b2. laggingBand now skips any run whose aim can't be met: canSatisfy checks that the non-skipped cells in the fixed ±0.3 m stretch around the run's middle still reach aimSatisfied. Skipped cells are never treated as observed. New test threeMissingCellsBetweenSkippedOnesAreNotAskedFor covers this exact case: missing ground cells 14 to 16 centred at s = 2.3622, with 13 and 17 skipped. No ground aim is asked for there. As a control, with 13 and 17 open the same map still aims at 2.3622. Mac mini g5-tf1 at 3ec22b2: drift passed, and HouseScanKit swift test -warnings-as-errors passed (517 tests), as did the device build. UI tests are still queued. firstHole has the same pattern but predates this PR, so it is noted under Known gaps in the PR body and not changed here.
A run of exactly three missing cells between two skipped ones was centred on a 0.6 m stretch of five cells; skipped cells are never covered, so the aim could reach only 3/5 and ended by stalling. laggingBand now drops runs whose stretch can't reach aimSatisfied without the skipped cells. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2 participants
Fixes #129
Three follow-ups left over from #10. The first is display only; the other two are about guidance. None of them touches what the export claims.
1. Fence or driveway crossing a corner (result view)
Before:
ResultScene3D.buildDioramaput each marked feature on the wall piece under its midpoint. A fence or driveway whose two taps landed on different pieces was drawn whole on that one piece, so in the result it kept going straight past the corner.Now: fences and driveways are drawn in parts, one per piece they cover, the same way clearance zones already are. The first and last pieces extend past the drawn wall, so no part of a mark gets cut off at the ends. Doors, windows, gas meters and AC units still go on their midpoint's piece. The live camera overlay (
WallMarksOverlay) already draws these marks as lines between their measured world points, so it needed no change.2. Ground-gap settlement at the entry cap
Before:
GapPlanner.exportedalways coarsened every band toSceneExport.bandBudget(125 entries). The export gives the ground band one entry fewer per corner (bandBudget - corners). On a chain with corners at the cap, the export could join two ground reaches the planner still kept apart. The request then counted as met on the phone while the uploaded scene showed a smaller reach, and the server asked again.Now: the budget lives in one place,
SceneExport.observedBudget(_:corners:). The export and the planner both use it. The planner takes the corner count from the coverage map's chain (wall.segments.count - 1).Test:
CorneredCapSettlementTestsbuilds 125 ground spans with alternating reaches onChainExportTests.wall(2 corners) and exports them. For every cell away from the corner cuts, it checks that the planner's settlement agrees with the server-sideseen_toport reading the written scene. It also checks that the old full-share reading claimed at least one reach the export had joined down.3. Aim between disconnected missing runs
Before: in
GuidancePlanner.laggingBand, runs were split only by deferred stretches. Lagging cells on both sides of ground that was already done (covered or skipped) formed one run, and the aim went to its middle, which is on the done ground.laggingGroundAsksToAimDownasserted exactly that: s = 0.1524, in skipped ground.Now: a cell where the aimed band is already done also splits the runs. The aim goes to the nearest contiguous run that is long enough.
laggingGroundAsksToAimDownnow expects s = 0.6096 and checks that every cell in the requested stretch is still missing ground. The two hold/hysteresis tests that reused that aim now step to s = 0 instead of -0.5, so the new stretch is still in view (0.61 m away, inside the 1 m hold).A run is also dropped if its aim can't be met (review of this PR). An aim task checks a fixed 0.6 m stretch, and skipped cells never count as covered. So a run of exactly three missing cells between two skipped ones sits in the middle of a five-cell stretch and can reach only 3/5, below the 0.8 threshold. The request could only end through the stall timeout.
laggingBandnow keeps only runs where the stretch around the middle still reachesaimSatisfiedwithout the skipped cells (canSatisfy).threeMissingCellsBetweenSkippedOnesAreNotAskedForcovers that case: missing ground cells 14 to 16, centred at s = 2.3622, with 13 and 17 skipped. As a control, the same map with 13 and 17 open still aims at 2.3622.This PR doesn't edit any of the functions #135 changes (
WalkRefusals.swiftand the app runtime files).Known gaps
hiddenNearCamera("see behind") still aims at the midpoint of the first and last hidden cells near the camera. The ios: guided wall scan from the meter to a battery spot #10 review says it has the same pattern. Issue Follow up corner display and guidance settlement limits #129 lists only lagging-band aims, so it is left as is.firstHole(after both ends are marked) still picks the middle of the first long-enough run without the satisfiability check. That code predates this PR and the review didn't flag it.Tests run
~/hs-ci/ci-local.sh t3/129-corner-and-settlement-limits g5-guidance-limits-v2r1at 9d0a662: xcodegen drift passed.swift testfor HouseScanKit with-warnings-as-errorspassed: 516 tests in 76 suites, includingCorneredCapSettlementTestsandlaggingGroundAsksToAimDown. The device build succeeded (** BUILD SUCCEEDED **).GuidancePlannerTeststhat still expected the old midpoint aim (0.1524). Those are fixed in 9d0a662.g5-tf1at 3ec22b2 (the satisfiable-aim fix): xcodegen drift passed.swift testfor HouseScanKit with-warnings-as-errorspassed: 517 tests in 76 suites, includingthreeMissingCellsBetweenSkippedOnesAreNotAskedFor. The device build succeeded.🤖 Generated with Claude Code