Repository navigation
Mark an existing battery or another box on the wall (#27) - #52
AidenJohnston wants to merge 6 commits into
Conversation
|
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. |
|
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 |
![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: f0e97321ea
ℹ️ 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".
| let span = (ss.min() ?? 0)...(ss.max() ?? 0) | ||
| switch feature.kind { | ||
| case .door, .window: | ||
| case .door, .window, .battery, .elecBox: |
There was a problem hiding this comment.
When a homeowner follows the new prompt and taps the visible corners of a battery or electrical box from an oblique viewpoint, markFeaturePoint intersects each ray with the wall plane, not the equipment surface. Treating those intersections like door/window corners here produces a perspective-shifted span and height; an off-center camera can move the entire recorded box away from its true edge, allowing the server to evaluate clearance from the wrong location. These kinds need an equipment-surface/depth hit or prompts that identify points actually on the wall plane.
Useful? React with 👍 / 👎.
| /// How far an existing battery stands off the wall. Its corners are tapped on the wall plane, | ||
| /// which gives no depth, so this is Base's own battery, 22 in (the server's D): nominal, not | ||
| /// measured. A deeper footprint only holds a new battery farther away. | ||
| static let batteryDepth: Float = 0.5588 |
There was a problem hiding this comment.
For an older 3-by-3-foot unit or any non-Core battery deeper than 22 inches, this truncates the exported footprint to 0.5588 m even though docs/01-feature-map.md specifies battery dimensions per model. Near a corner or another wall, the server can consequently measure its required battery clearance from a fictitious front edge and approve a spot too close to the real unit; obtain the model/depth, measure it, or return an uncertain result rather than hardcoding one footprint in the client.
AGENTS.md reference: AGENTS.md:L20-L21
Useful? React with 👍 / 👎.
|
Please fill in this PR description before the review gate: what the two marks mean, how each reaches scene.json and the packet, test evidence, and what still needs a device check. The prior work split assigned #27 mark buttons to S3 after S2’s server |
|
I read f0e9732 against the engine and export. The shape is right: two corner taps like a window,
Also: please check the mark tray with eight chips at AX5 in the audited states, if |
AidenJohnston
left a comment
AidenJohnston
left a comment
There was a problem hiding this comment.
Challenger review. The contract side is right. battery and elec_box match the server schema, the vendored scene.schema.json is identical to server/schemas/scene.schema.json on t3/server, and battery carries the footprint that battery_clearance requires. elec_box goes through wall_equipment_above and the route detour. The "What do you see?" sheet gets both kinds through FeatureKind.allCases, which fixes the run-2 comment. No project files changed. One blocking problem:
-
blocking:
ios/HouseScan/Runtime/ScanEngine+Actions.swift:237and:277. Battery taps are intersected with the wall plane (wall.intersectWall(ray)). The prompt atScanCopy.swift:185asks for the battery's "bottom-left corner", and the visible corner is on the front face, up to 22 in off the wall. Projecting that corner back onto the wall scales each tap's offset from the phone byd / (d - 0.56). So both edges of the span move away from the phone. For a phone 2.5 m out and 1 m to the left of the battery, the left edge lands about 0.29 m (11 in) inside the real battery. That understates the 3 ft clearance on the side the homeowner is standing, by more than the 0.3 ft tap error. The same projection also putstoptoo low. Fix: for.battery, intersect each tap ray with the wall plane and with the plane atout = SceneExport.batteryDepth, then take the span and heights over both sets of hits. That is conservative for any battery depth up to 22 in. Alternatively, change the prompt to "Tap where its left side meets the wall", which the wall-plane hit measures correctly.elec_boxis shallow, so its error is small. -
nit: the PR body is empty. Say that it fixes #27 (to be closed by hand, since the base is
t3/ios-mvf), that the packet manifest leaves these marks out (ScanEngine+Packet.swift:137) and they travel only in scene.json, and list device checks: mark the installed battery from a few angles, then confirm the spot moves at least 3 ft from it.
|
Read-only review at |
Pre-CI checkThe queued iOS run for Fix: merge the current From reading the diff, I found nothing else that would fail CI. Every switch over |
|
Current-head CI on |
f0e9732 to
bc94dca
Compare
|
iOS CI on |
The tray gains Battery and Electrical box. Both are marked by two corners like a window. A battery exports as a scene object of type battery with a plan footprint out to a nominal 22 in, which the server's battery_clearance measures from; a disconnect, sub-panel, EV charger or solar box exports as elec_box, flush with the wall. The packet's manifest schema 1.1 has no mark kind for either, so they travel in its scene.json only. Co-authored-by: Cursor <cursoragent@cursor.com>
bc94dca to
68a66a8
Compare
|
@AidenJohnston I now own the carried review/CI loop. Source review at |
|
@AidenJohnston CI diagnosis needed after three failed attempts of run 36280888444 on unchanged |
|
@AidenJohnston The parent is assembling a test beta now. For #52 at |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let span = (ss.min() ?? 0)...(ss.max() ?? 0) | ||
| switch feature.kind { | ||
| case .door, .window: | ||
| case .door, .window, .battery, .elecBox: |
There was a problem hiding this comment.
When someone taps the visible corners of an installed battery or electrical box that stands out from the wall, these marks use the ray’s intersection with the wall behind the object. The two projected corners can be shifted or spread apart depending on the camera position, so the exported object can have the wrong span and produce incorrect placement clearances.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScan/Runtime/ScanEngine+Actions.swift
Line: 277
Comment:
**Protruding corners are misplaced**
When someone taps the visible corners of an installed battery or electrical box that stands out from the wall, these marks use the ray’s intersection with the wall behind the object. The two projected corners can be shifted or spread apart depending on the camera position, so the exported object can have the wrong span and produce incorrect placement clearances.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| footprint: kind == .battery | ||
| ? [plan(left, 0), plan(right, 0), plan(right, batteryDepth), plan(left, batteryDepth)] : nil, |
There was a problem hiding this comment.
If an existing battery is deeper than 22 inches, or an electrical box such as an EV charger protrudes from the wall, this export represents less space than the object occupies: batteries get a fixed-depth footprint, while electrical boxes get no footprint. Without a footprint, the scene treats the box as only a stretch of wall line, so the resulting clearance can be measured too close to the real equipment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScanKit/Sources/HouseScanKit/Scene/SceneExport.swift
Line: 443-444
Comment:
**Object depth can be understated**
If an existing battery is deeper than 22 inches, or an electrical box such as an EV charger protrudes from the wall, this export represents less space than the object occupies: batteries get a fixed-depth footprint, while electrical boxes get no footprint. Without a footprint, the scene treats the box as only a stretch of wall line, so the resulting clearance can be measured too close to the real equipment.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if feature.kind == .battery || feature.kind == .elecBox { | ||
| return .box(kind: feature.kind == .battery ? .battery : .elecBox, span: feature.span, bottom: bottom, top: top) |
There was a problem hiding this comment.
Two taps at the same or nearly the same position complete a battery or box mark, and this path exports its span without checking its size. That can send a zero-width or negligible-width obstacle while showing the mark as complete. Refusing collapsed box marks, as the export already does for collapsed ground lines, would avoid sending misleading geometry.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScan/Runtime/ScanEngine+Export.swift
Line: 40-41
Comment:
**Collapsed box marks are exported**
Two taps at the same or nearly the same position complete a battery or box mark, and this path exports its span without checking its size. That can send a zero-width or negligible-width obstacle while showing the mark as complete. Refusing collapsed box marks, as the export already does for collapsed ground lines, would avoid sending misleading geometry.
---
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 Confirmed at 68a66a8: two identical wall taps complete the mark in ScanEngine+Actions.swift:249-258, and SceneExport.swift:435-445 accepts equal span endpoints. A battery then exports a degenerate four-point footprint; an electrical box exports a zero-width span. Please reject a collapsed box with an explicit retap path and cover repeated taps plus export-time collapse. Do not silently drop the obstacle. This is an additional #52 fix request; the existing projection/depth findings remain open.
…t meets the wall, and send the answer to a person A mark's taps meet the wall's plane, so a box's front corner tapped from one side landed elsewhere on the wall, and the battery's footprint used Base's own 22 in depth for somebody else's unit. The prompts now ask for the back corners, where the box meets the wall, which the wall-plane hit measures. scene.json carries both kinds with no footprint and no depth. The server measures to their stretch of wall line and has no field for an unknown depth, so the app reads any answer to a scan with one as an installer's (ResultReading.answer, unmeasuredMarks), says why under the result, and the AR title no longer says the battery could go there. The tray chip reads "Box on the wall". 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. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@AidenJohnston Current-head run36314131622 failed compilation of the PR merge result ccabbfe, merging821cee303e6232f67c431b2c8d2d338060fb88f0 into29ebe846a3dac8c9e6a60ce3a96bc7cd3e4bff48. ScanEngine+Confirm.swift:338 switch kind lacks .battery and .elecBox. Please coordinate their spot-confirmation handling with S3 and add the explicit cases. This is a newer-base integration failure, not the same line in your standalone head. Codex current-head completion does not clear this build or the collapsed-span request. |
…ox, like a driveway and a fence 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: 98aa14ca24
ℹ️ 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".
| .foregroundStyle(.primary) | ||
| .accessibilityAddTraits(.isHeader) | ||
| Text("Gas meters, doors, windows, AC units, driveways and fences all change where a battery can go.") | ||
| Text("Gas meters, doors, windows, AC units, other batteries, boxes on the wall, driveways and fences all change where a battery can go.") |
There was a problem hiding this comment.
When a VoiceOver user reaches the live wall walk's action.markSomething button, its hard-coded hint in WallWalkScreen.swift still lists only gas meters, doors, windows, AC units, driveways, and fences. Because the hint describes the supported choices before the tray opens, it now omits the newly available battery and wall-box actions even though the visible review copy lists them; update that hint with the new kinds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@AidenJohnston Confirmed at 98aa14c: WallWalkScreen.swift:323 still lists the old kinds. Please include battery and electrical box in this hint when carrying the new marks. This is a VoiceOver copy gap; #163 remains the demo priority.
| case .battery: | ||
| return MarkedFeature(id: UUID(), kind: kind, span: -1.1 ... -0.35, bottom: 0, top: 1.0, out: nil, | ||
| points: [wall.world(s: -1.1, height: 0), wall.world(s: -0.35, height: 1.0)], opens: nil) |
There was a problem hiding this comment.
In -uiDemo, marking this new kind appends it to state.features, but sample and showResult() continue returning static ResultPresentation values whose unmeasuredMarks remains empty. Continuing through "Looks complete" can therefore show "A battery fits here" instead of requiring installer confirmation, so the demo and its UI tests do not exercise the production invariant added by this change; derive the demo result's unmeasuredMarks from state.features too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@AidenJohnston Confirmed at 98aa14c: DemoEngine.swift:806 appends the mark, but sample at520-525 and showResult at381-385 return static presentations without unmeasuredMarks. Please carry the marked kinds into the demo result and cover the pass-result path so a marked battery or electrical box still requires installer confirmation. This limits demo/test fidelity; it does not establish a production mapping failure or hold the current frozen candidate. #163 remains the demo priority.
# Conflicts: # ios/HouseScanKit/Sources/HouseScanKit/Scene/SceneExport.swift
…ey measure A gas meter or AC unit was one tap, exported as a 0.3 m square standing 0.3 m off the wall: a guessed width and depth. - Gas meter: two taps where it meets the wall, bottom-left then top-right, as a window or #52's box. Exported as a wall object with its span, bottom_ft and top_ft and no footprint, since a wall-plane tap says nothing about how far it stands out. The server measures to its stretch of wall line. - AC unit: two taps on the ground where its front meets it (a ray aimed at a unit meets the ground in front of the wall, #163). Exported with its span and a footprint from the wall line to that tapped front edge, so its depth is measured and the space behind it counts as taken. Height is not measured and is left out. - Refused and marked again: taps closer than 1 cm (SceneExport.minObjectSize), a single tap, and AC corners on two pieces of the wall. - The packet manifest (1.1) gives these kinds one point, so each mark carries the middle of its two corners; the packet's scene.json has the span and footprint. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Updated for review at Base merges
Review items
CI
Device check still needed: mark an installed battery by its back corners from a few angles, send the scan, and confirm the result says an installer will confirm. |
|
@greptileai review Please review the current head |
| answer = .installer | ||
| } | ||
| } | ||
| return unmeasuredMarks && answer != .oneMoreLook ? .installer : answer |
There was a problem hiding this comment.
.installer. The result card then has no button: its installer button requires a spot, while “Scan another wall” appears only for .notHere. Starting over is still available under Details, but the card no longer offers a next step.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScanKit/Sources/HouseScanKit/Scene/ResultReading.swift
Line: 83
Comment:
**Rejected scan loses primary action** When a scan with a battery or box mark is rejected without a spot, this override changes its answer to `.installer`. The result card then has no button: its installer button requires a spot, while “Scan another wall” appears only for `.notHere`. Starting over is still available under Details, but the card no longer offers a next step.
---
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 Caretaker check at 086ad94: confirmed, and new in this PR. A scan with a battery or box mark that comes back as a reject with no spot becomes .installer (ResultReading.swift:83). In ResultScreen.swift:267-277, the installer case renders a button only when a spot exists, and "Scan another wall" exists only for .notHere. The card therefore has no primary action, and Start over is hidden under the collapsed Details. The base returned .notHere here. Please give an installer answer without a spot the "Scan another wall" action while keeping the installer headline. Add a UI assertion for reject, no spot and a battery or box mark: the restart button is visible before Details is expanded, and tapping it starts a new scan.
| case .battery: | ||
| return MarkedFeature(id: UUID(), kind: kind, span: -1.1 ... -0.35, bottom: 0, top: 1.0, out: nil, | ||
| points: [wall.world(s: -1.1, height: 0), wall.world(s: -0.35, height: 1.0)], opens: nil) | ||
| case .elecBox: | ||
| return MarkedFeature(id: UUID(), kind: kind, span: 1.5...1.8, bottom: 1.1, top: 1.5, out: nil, | ||
| points: [wall.world(s: 1.5, height: 1.1), wall.world(s: 1.8, height: 1.5)], opens: nil) |
There was a problem hiding this comment.
unmeasuredMarks from them. After marking either one, a passing demo result can still say the battery could go there and omit the installer-check explanation. This makes the demo misleading and prevents it from exercising the new marked-result state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScan/UI/Preview/DemoEngine.swift
Line: 1003-1008
Comment:
**Demo marks bypass installer review** The demo can add these battery and box marks, but its sample result never sets `unmeasuredMarks` from them. After marking either one, a passing demo result can still say the battery could go there and omit the installer-check explanation. This makes the demo misleading and prevents it from exercising the new marked-result state.
---
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 086ad94: same defect as Codex 4115206629, which Sam confirmed in 4115220654 and scoped to demo and test fidelity (production fills unmeasuredMarks at ScanEngine+Export.swift:200). No fix at this head yet; tracking it on that thread.
This branch has not been deployed
2 participants
Fixes #27 (close it by hand: the base is
t3/ios-mvf).The review tray gains Battery and Box on the wall. A battery is one already installed; a box is a disconnect, sub-panel, EV charger or solar box. Each is marked with two taps where it meets the wall: bottom left, then top right.
What reaches the server
scene.jsongets an object of typebatteryorelec_boxwithspan_ft,bottom_ftandtop_ftfrom the two taps,source: tap, and no footprint. A tap lands on the wall's plane, so the prompts ask for the back corners, where the box meets the wall. A front corner tapped from one side would land somewhere else on the wall (SceneExportTests.anOffAxisBoxIsMarkedWhereItMeetsTheWall).scene.schema.jsonhas no field for an unknown depth, so the server measures its clearances to the box's stretch of wall line. That can understate a deep unit near a corner.ResultReading.answer,unmeasuredMarks). A view the camera can take still comes first. The AR title no longer says the battery could go there.scene.json.Tests
SceneExportTests.aBatteryOrBoxGoesOutWithNoFootprintandanOffAxisBoxIsMarkedWhereItMeetsTheWall;ResultReadingTests.aScanWithAnUnmeasuredMarkGoesToAPersonandaPassingAnswerToAScanWithABatteryMarkedGoesToAPerson;PacketWriterTestsfor the manifest skip.ScreenStatesUITests.testReviewMarksAnExistingBatteryAndABoxand thewallWalk-markingBoxstate.Still needed
batteryandelec_box.🤖 Generated with Claude Code