Repository navigation
Draw unclean AR spots as outlines and face the closest spot in the result model - #146
Conversation
…lift) - ResultScene3D faces the camera at the closest spot tried when there is no spot, the same spot it circles, so a rejected candidate round a corner opens from its front rather than the meter wall's heading. - "See it on your wall" draws a spot that isn't a clean fit as the dashed outline of its footprint, in both the AR scene and the Canvas overlay, as the result card does, and no longer titles it "Your battery could go here" on a pass. - The footprint outline sits above every clearance zone drawn, not at a fixed 2 cm that a sixth zone reached. The mark choice, focus spot, zone stacking, outline lift and dash layout live in HouseScanKit's ResultMarkLayout with tests. 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 |
After the camera failed post-upload, the AR button was gated but the card's "Show me", the check lines' "Show me" and Details' "Capture it now" stayed on offer and opened a capture with no frames. ResultCardActions in HouseScanKit now picks the card's button and gates every capture offer on the source, and captureMissing refuses once the source is lost. Part of #110. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Resolves ResultScreen.swift: beta's spot-refused notice and start-over hint kept, with the card's sourceAvailable parameter. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
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: 6e8403fd18
ℹ️ 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".
| root.addChild(joint) | ||
| } | ||
| if let spot = result.spot { | ||
| if let spot = result.spot, ResultMarkLayout.spotMark(spotIsClean: result.spotIsClean) == .outline { |
There was a problem hiding this comment.
For an unclean spot, this branch moves the only spot mark to the ground, but ScanEngine.showResultInCamera and SpotDirection still test spotCenter(on:) at half the battery height. When that midpoint is inside the viewport while the ground footprint is below it, resultIsDrawn() suppresses the Canvas fallback and the chevron remains hidden, leaving the user with no visible indication of the spot. Use the footprint's ground-level center for the visibility and direction targets when rendering an outline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@AidenJohnston Independent nine-file review and lead source tracing confirm this at 6e8403f. Both renderers now put an unclean spot outline near ground level, while ResultARScreen.spotCenter:88-91 and ScanEngine:1488 retain half-battery-height focus. With that midpoint inside the clear band and the footprint below the screen or controls, neither the mark nor a downward chevron is visible. This affects Canvas on non-LiDAR too; restoring Canvas alone is insufficient. Please make the shared visibility/direction target follow the rendered mark, retaining midpoint targeting for clean batteries, and cover both projections. The clean resultAR demo does not exercise this case. Source verified only; no physical reproduction here.
There was a problem hiding this comment.
Fixed in 9a0e275. ResultMarkLayout.focusHeight(mark:batteryHeight:) (HouseScanKit) gives the height the AR view aims at: the battery middle for a clean spot, the ground for an outline. ResultPresentation.spotCenter(on:) now uses it with spotMark(spotIsClean:), so both the edge chevron (SpotDirection, Canvas and RealityKit paths alike) and ScanEngine.showResultInCamera's focus for resultIsDrawn() follow the rendered mark. Clean batteries keep the midpoint. ResultMarkLayoutTests.focusHeightFollowsTheMark covers the helper.
For a spot that isn't a clean fit both renderers draw the footprint outline on the ground, but the edge chevron (SpotDirection) and the AR scene's in-view check (showResultInCamera focus) still aimed at half the battery height. With that midpoint on screen and the outline below it, neither the mark nor the chevron showed. ResultMarkLayout.focusHeight now gives the height for the mark drawn (battery middle or the ground), and spotCenter uses it, so both checks follow the rendered mark. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
retake_request is the only final event when views or a member action are needed; otherwise verdict_ready. A no-meter or degraded run that still needs views stays needs_views, and the final-event check runs on those branches too. Co-authored-by: hss-cloud-agent <hss-cloud-agent@users.noreply.github.com>
Brings in all 127 commits of house-scanning-server main up to d43d657
("Emit one final event and keep result status with the outcome (#146)"),
with paths rewritten under backend/ by git filter-repo. The .github/
workflows stay out, as in the snapshot, because GitHub only runs workflows
from the repository root. Hunter's work email is mapped to his GitHub
no-reply address so this public repository doesn't publish it; GitHub
still attributes the commits to huntertcarver.
The resulting tree equals the snapshot commit 2d0911f exactly.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Part of #110.
The two original items on #110 (needsPerson checks under "Settles", rejected sweep extents) were fixed by #91 and are on
beta. This PR takes the follow-ups from SamGu-NRX's three comments on #110. The fourth bot item in the second comment (capturable missing evidence without a linked check) stays open; see Known gaps.What the old code did
ResultScene3D.orbitTargetalready circledspot ?? nearestSpot, butorbitHeadingstill readresult.spotonly. A reject whose closest spot lay round a corner opened facing the meter's piece of wall, not the candidate's.ResultARModel) and the Canvas fallback (BatteryOverlay). The AR title checked onlydecision == .pass && policyApproved, so a pass whose working-space check wasn't clean would have read "Your battery could go here".spatialResultAvailable == false), the result screen hid the AR button but kept offering the card's filled "Show me" (action.showMe), the check lines' "Show me" and Details' "Capture it now". Each opened a capture with no frames coming.captureMissingdidn't check the source either.What changed
orbitHeadingfaces the same spot the camera circles: the spot, else the closest spot tried, else the meter.spotIsCleanas well before it says "Your battery could go here". Otherwise it keeps "The spot an installer will check". The VoiceOver label says it is an outline.spotCenter(on:)now targets the outline's ground-level centre instead of half the battery height, via HouseScanKitResultMarkLayout.focusHeight. Both the edge chevron (SpotDirection, over Canvas and RealityKit) and the AR scene's in-view check (ScanEngine.showResultInCamerafocus forresultIsDrawn()) use it. A clean battery keeps the midpoint. Tested inResultMarkLayoutTests.focusHeightFollowsTheMark(9a0e275).ResultScene3Dinto a sharedFootprintOutline, which the AR model also uses.workingSpaceOutcomemoved ontoResultPresentation.ResultMarkLayout, tested inResultMarkLayoutTests. One behavior change in the dashes: an edge between 0.16 m and 0.2 m used to get two overlapping 0.1 m dashes. It now gets one solid line, which looks the same.ResultCardActionspicks the card's filled button from the answer, the spot and whether the camera is still up.offersView(capturable:sourceAvailable:)gates the card's "Show me", every check line's "Show me" and Details' "Capture it now". With the camera gone none of them shows, the same as the AR button. "Scan another wall" stays, because it starts a new camera session.ScanEngine.captureMissingalso returns early whenspatialResultAvailableis false. Tested inResultCardActionsTests.origin/betainto the branch. The merge was needed:git merge-treeof 0964ecc withorigin/betaconflicted inResultScreen.swift. The resolution keeps beta's spot-refused notice and its new start-over hint, and passes the card the newsourceAvailableparameter.None of this changes placement coordinates, outcomes or the answer the card leads with.
Known gaps
ResultReading.answer: the headline would become "One more look" for a view that isn't known to settle any check. That is a product decision, so this PR leaves it open.beta, and this head contains it. ItsResultARScreen.swiftedits (inSpotDirection) merged cleanly with the title, label andBatteryOverlayhunks here.resultARdemo state shows a clean spot, and no UI test drives the camera-failed result.Tests
~/hs-ci/ci-local.sh t3/110-result-details-followups g4-tf1at 9a0e275 (the head): the xcodegen drift check passed. HouseScanKitswift testwith-warnings-as-errorspassed: 526 tests in 77 suites, includingfocusHeightFollowsTheMark. The device build passed. UI tests are pending (waiting for the Mac mini's UI lock).~/hs-ci/ci-local.sh t3/110-result-details-followups g4-result-details-r2at 6e8403f: the xcodegen drift check passed. HouseScanKitswift testwith-warnings-as-errorspassed: 525 tests in 77 suites, includingResultCardActionsTestsandResultMarkLayoutTests. The device build passed. UI tests are pending. They're waiting for the Mac mini's UI lock, and they'll also run on GitHub CI once the PR is ready.g4-result-details-r1at b8b3c37 passed everything, including UI tests (24 run, 1 skipped, 0 failures). It skips the accessibility audit.git merge-tree --write-tree origin/beta HEADat 6e8403f: clean.origin/beta(6bc1df0) is an ancestor of the head.🤖 Generated with Claude Code