Skip to content

fix(ios): refuse object taps that meet the wall under the floor or far along it - #151

Merged
AidenJohnston merged 2 commits into
betafrom
t3/140-object-tap-reach
Sep 27, 2026
Merged

AidenJohnston merged 2 commits into
betafrom
t3/140-object-tap-reach

Conversation

@AidenJohnston

@AidenJohnston AidenJohnston commented Sep 27, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #140

What the old code did

markFeaturePoint (ios/HouseScan/Runtime/ScanEngine+Actions.swift) sent every wall-mounted kind (AC unit, gas meter, door, window, box) straight to wall.intersectWall(ray) and appended whatever hit came back. intersectWall never bounds a hit's height, and the outermost wall pieces run on forever, so a tap aimed down at the ground met the wall's plane tens of meters under the floor. In the reported run two AC taps were pinned 105 ft and 276 ft from the meter, far below the floor, and exported. Only ground taps (driveway, fence) and wall-end taps had a reach or sanity check.

What changed

  • New ObjectTap.refusal(_:camera:wall:reach:groundError:) in HouseScanKit (Geometry/ObjectTap.swift). It refuses a wall hit that:
    • lies more than 0.15 m (plus the guessed ground's heightError) below the wall's ground. From a phone above the ground this also covers every ray that crosses the ground before the wall.
    • lies farther along the wall from the phone than the coverage map's maxDistance, the same bound wall-end taps already use.
  • markFeaturePoint calls it for non-ground kinds and answers a refused tap with the existing .noSurface refusal ("Nothing to pin there. Aim at the wall or the ground and try again."), logging the reason.
  • The 0.15 m slack is a tap-plausibility tolerance, not a clearance number, so it sits in code with a note that it's a guess.

Tests

ObjectTapTests uses synthetic fixtures (1d6adb6, cherry-picked from Sam's 8d9bfd7 after the Codex P1 review), not field-derived coordinates:

  • Points far under the floor and far along the wall (e.g. [-20,-10,0], [-40,-25,5] from the meter) are refused.
  • Points just above and just under the ground near the meter ([0.25,-1.4,0], [-1,-1.55,0]) are accepted, both as raw points and as rays cast from a phone through them.
  • A downward ray that meets the ground first is refused, even with a 0.3 m ground error.
  • Ground error widens the slack.
  • A grazing ray meeting the wall 30 m along is refused, and one meeting it 5 m along is accepted.
  • Reach is measured from the phone, not the meter.

Commands run

Mac mini ~/hs-ci/ci-local.sh t3/140-object-tap-reach g1-ac-reach-r1 on fd50357:

  • xcodegen drift: passed
  • swift test HouseScanKit (warnings as errors): passed, "Test run with 501 tests in 73 suites passed"
  • device build: passed
  • UI tests: still queued behind the Mac mini's UI lock. They're pending on GitHub CI once this PR is ready.

Known gaps

  • Not tried on a phone. The fix is checked against the issue's logged points, not with physical taps.
  • The 0.15 m below-ground slack is a judgement call, not a measured value.
  • A hit above the wall's top isn't refused, because the wall's height isn't bounded here either. The reported failures were all below the floor or far along the wall.

🤖 Generated with Claude Code

…r along it

markFeaturePoint took any wall-plane hit for an AC unit, gas meter, door,
window or box. A tap aimed down at the ground met the wall's plane tens of
meters under the floor and far along the wall, and was pinned and exported
(#140). ObjectTap.refusal in HouseScanKit now rejects a hit more than 0.15 m
(plus the guessed ground's error) below the ground, or farther along the wall
from the phone than the coverage map's maxDistance, the bound wall-end taps
already use. The engine answers such a tap with "Nothing to pin there".

Fixes #140

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a0f6d562-eec4-4205-b08d-55d13ffbe306

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the size:M 30-99 effective changed lines. label Sep 27, 2026
@AidenJohnston
AidenJohnston marked this pull request as ready for review September 27, 2026 09:23
@codemetrics-ai

Copy link
Copy Markdown

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fd50357db9

ℹ️ 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".

Comment on lines +30 to +33
// 105 ft left, 28 m below the meter.
(SIMD3<Float>(-31.89, -28.04, 0), false),
// 70 m below the meter.
(SIMD3<Float>(-77.78, -70.34, 6.58), false),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove real capture coordinates from tracked tests

These vectors are copied from a real field run's manifest, as the file states on lines 6-8, so committing them exposes measurements from a real home in this public repository. Replace every field-derived coordinate and associated exact measurement with synthetic cases that exercise the same refusal boundaries. The repository requires real captures and dataset material to remain in ignored directories.

AGENTS.md reference: AGENTS.md:L30-L32

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AidenJohnston Confirmed at fd50357: ObjectTapTests explicitly copies field-manifest values, and ObjectTap.swift comments also repeat exact field measurements. Please replace the field-derived vectors and associated exact measurements/comments with clearly synthetic fixtures, retaining coverage for below-floor hits, grazing rays, ground-error slack and camera-relative reach. Keep the runtime guard. The reported capture was an office hallway, so the review's home attribution is unsupported; the real-capture provenance still warrants this cleanup under AGENTS.md. Send the corrective SHA and validation evidence here for the parent's next cut. Root is holding this fixture change before promotion; beta6bc and unrelated testing remain unchanged. Use a normal follow-up commit, with no history rewrite.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AidenJohnston Coordination update: root is replacing the field-derived test vectors and exact-measurement prose with synthetic fixtures in its isolated beta assembly, preserving the guard behavior and your branch. Root will supply a fixture-only commit after ObjectTapTests. Please reuse that coming commit, or provide your equivalent corrective SHA if already prepared. No history rewrite or parallel implementation on your branch.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AidenJohnston Fixture-only correction is available for reuse: 8d9bfd7 on codex/beta-qa-object-taps-20260927. I inspected both changed files: field-derived test coordinates, pose values, labels and exact-measurement prose are replaced with synthetic cases; ObjectTap runtime guard is unchanged. Root reports all six ObjectTapTests passed with warnings-as-errors. Please reuse this commit on #151 or provide your equivalent SHA. Its parent4482ab1cb0bf1352311479778e9313ca39befe22 is the app/UI revision in already-started36310019572; that run is not evidence of tests at8d9. No new CI requested solely for this fixture/prose delta.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1d6adb6: cherry-picked Sam's 8d9bfd7, so ObjectTapTests uses synthetic fixtures and ObjectTap.swift no longer repeats field measurements. PR body updated to match.

@AidenJohnston
AidenJohnston merged commit e7af970 into beta Sep 27, 2026
4 of 5 checks passed
@SamGu-NRX

Copy link
Copy Markdown
Owner

@AidenJohnston Beta assembly coordination from root: root will preserve your beta history, now reported at e7af970 with #135/#149/#151, and reconcile it with the synthetic-fixture correction. The following cut is planned to include corrected #146 at9a0e275 and #152 atb2d80e8 plus the explicit live-fog-beta6836 handoff. Please coordinate further beta pushes through root while this batch is being tested so the team can land one usable build. Root reports4482 package/build passed with full UI still running. Main PRs remain open. This requests coordination only, no further source changes or duplicate tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 effective changed lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants