From 0348628d13ec91fe8175f569963c25fde058ad66 Mon Sep 17 00:00:00 2001 From: SamGu-NRX Date: Sat, 3 Oct 2026 04:12:48 -0500 Subject: [PATCH 1/2] ios: a close-up the phone couldn't save says so, instead of "Hold still" A failed save of the meter close-up retook with `.blurry`, so the screen told the homeowner to hold still, which can't help a phone that can't write the photo (B-20). A photo that couldn't be read back for the meter number said "Couldn't read the number", blaming the shot the same way. `KeyframeStore.saveStill` now says what happened: saved, not written, or written in a world frame discarded meanwhile. Not written, and not read back, retake as the new `.photoNotSaved`: "Your phone couldn't save that photo. Hold on the meter to try again." It counts as a failed attempt, so "Can't get a clear shot" still follows on the next one. A shot from a discarded world frame is dropped without a retake: the discard already sent the flow back to finding the meter, and a new close-up starts its own count. No photo that failed to save is listed or accepted. `-failCloseUpSave` drops the photo before the real save on a replay, so CloseUpSaveFailureUITests reaches the message and the skip through the real engine. Co-Authored-By: Claude Opus 5.5 (1M context) --- ios/HouseScan/Contract/ScanContract.swift | 3 ++ ios/HouseScan/Runtime/Autopilot.swift | 2 +- ios/HouseScan/Runtime/KeyframeStore.swift | 45 +++++++++++++---- ios/HouseScan/Runtime/LaunchOptions.swift | 5 ++ ios/HouseScan/Runtime/ScanEngine.swift | 18 +++++-- ios/HouseScan/UI/Copy/ScanCopy.swift | 1 + .../CloseUpSaveFailureUITests.swift | 49 +++++++++++++++++++ 7 files changed, 108 insertions(+), 15 deletions(-) create mode 100644 ios/HouseScanUITests/CloseUpSaveFailureUITests.swift diff --git a/ios/HouseScan/Contract/ScanContract.swift b/ios/HouseScan/Contract/ScanContract.swift index 705c3e30e..08df68dff 100644 --- a/ios/HouseScan/Contract/ScanContract.swift +++ b/ios/HouseScan/Contract/ScanContract.swift @@ -443,6 +443,9 @@ enum CloseUpProblem: Equatable, Sendable { case numberTooSmall /// No number could be read at all: retake. case noNumber + /// The photo couldn't be written to the phone, or read back once written. Nothing about the + /// shot was wrong, so the words don't blame the homeowner's hands (B-20). + case photoNotSaved } /// One reading of the meter number from the close-up, for the homeowner to confirm. diff --git a/ios/HouseScan/Runtime/Autopilot.swift b/ios/HouseScan/Runtime/Autopilot.swift index edde51070..67612b64e 100644 --- a/ios/HouseScan/Runtime/Autopilot.swift +++ b/ios/HouseScan/Runtime/Autopilot.swift @@ -119,7 +119,7 @@ final class Autopilot { let state = engine.state guard state.phase == .meterCloseUp else { return .left } if case .choose(let candidates) = state.meterNumber, let first = candidates.first { return .choose(first) } - if case .aiming(_, let problem?) = state.closeUp, problem == .noNumber || problem == .numberTooSmall { return .retake(problem) } + if case .aiming(_, let problem?) = state.closeUp, [.noNumber, .numberTooSmall, .photoNotSaved].contains(problem) { return .retake(problem) } return nil } diff --git a/ios/HouseScan/Runtime/KeyframeStore.swift b/ios/HouseScan/Runtime/KeyframeStore.swift index b081627b1..a5aee22e8 100644 --- a/ios/HouseScan/Runtime/KeyframeStore.swift +++ b/ios/HouseScan/Runtime/KeyframeStore.swift @@ -162,26 +162,51 @@ final class KeyframeStore { case writeFailed = "write failed" } - /// Writes a still such as the meter close-up from `frame`. Returns false when there was - /// nothing to write. The still stands for its purpose only once accepted (`acceptStill`). - func saveStill(_ frame: SourceFrame, name: String) async -> Bool { + /// What became of a `saveStill`. The two failures need different words on screen: a photo + /// that couldn't be written is a phone problem the homeowner can retry or skip, while a + /// discarded world frame has already sent the flow back to finding the meter (B-20: both + /// used to read as a blurry photo). + enum StillSave: Equatable { + case saved + /// No image bytes to write, or the write failed (a full phone, say). Nothing new is + /// listed; a still saved earlier under the same name stays as it was. + case notWritten + /// Written, but in a world frame discarded meanwhile, so it isn't kept. + case worldDiscarded + } + + /// Writes a still such as the meter close-up from `frame`. The still stands for its purpose + /// only once accepted (`acceptStill`). + func saveStill(_ frame: SourceFrame, name: String) async -> StillSave { let directory = directory let id = Self.purpose(of: name) let startedIn = epoch - let written = await Task.detached(priority: .utility) { () -> StoredKeyframe? in - guard let data = Self.data(of: frame.jpeg), (try? data.write(to: directory.appending(path: name), options: .atomic)) != nil else { return nil } - return Self.stored(frame, id: id, jpeg: data, in: directory) + let written = await Task.detached(priority: .utility) { () -> Result in + guard let data = Self.data(of: frame.jpeg) else { return .failure(.noPhoto) } + do { + try data.write(to: directory.appending(path: name), options: .atomic) + } catch { + return .failure(.writeFailed) + } + return .success(Self.stored(frame, id: id, jpeg: data, in: directory)) }.value - guard let written else { return false } + let stored: StoredKeyframe + switch written { + case .success(let entry): + stored = entry + case .failure(let failure): + RuntimeLog.capture.error("still \(name, privacy: .public) not stored: \(failure.rawValue, privacy: .public)") + return .notWritten + } // Taken in a world frame that was discarded while the file was being written. The file is // left alone: a close-up in the new frame may already have written the same name, and // nothing reads a still that `stillFrames` doesn't list. guard startedIn == epoch else { RuntimeLog.capture.info("still \(name, privacy: .public) not kept: its world frame was discarded") - return false + return .worldDiscarded } - stillCatalog.save(written, purpose: id, fileName: name) - return true + stillCatalog.save(stored, purpose: id, fileName: name) + return .saved } /// The homeowner accepted the still saved as `name` (the meter number read from it was diff --git a/ios/HouseScan/Runtime/LaunchOptions.swift b/ios/HouseScan/Runtime/LaunchOptions.swift index 3d659c6ed..8c17e9fc6 100644 --- a/ios/HouseScan/Runtime/LaunchOptions.swift +++ b/ios/HouseScan/Runtime/LaunchOptions.swift @@ -28,6 +28,9 @@ import OSLog /// ground, as ARKit refining the ground would (`ScanEngine.injectGroundForTest`). Replays carry /// no plane evidence, so this is the only way a UI test reaches that path. Meters must be over /// the engine's 1 cm refine threshold and at most `GroundPlaneChoice.maximumRaise`. +/// - `-failCloseUpSave`: with `-replay`, every meter close-up's photo fails to save, as on a phone +/// with no space left, so a UI test reaches the save-failure retake and the skip after it. The +/// photo is dropped before `KeyframeStore.saveStill`, which then takes its own failure path. /// - `-simulateAppStore`: run as an App Store install would, so the developer options and practice /// meter are unavailable whatever the stored switch says (`DeveloperSettings`). It can only take /// the switch away, never offer it. @@ -42,6 +45,7 @@ struct LaunchOptions: Equatable { var autopilotSomethingThere = false var simulateAppStore = false var injectGroundRise: Float? + var failCloseUpSave = false init( arguments: [String] = ProcessInfo.processInfo.arguments, @@ -58,6 +62,7 @@ struct LaunchOptions: Equatable { autopilotCantGetThere = arguments.contains("-autopilotCantGetThere") autopilotSomethingThere = arguments.contains("-autopilotSomethingThere") simulateAppStore = arguments.contains("-simulateAppStore") + failCloseUpSave = arguments.contains("-failCloseUpSave") serverURL = (value(after: "-serverURL") ?? defaultServerURL).flatMap(Self.serverURL) sampleResult = arguments.contains("-sampleResult") if let gate = value(after: "-autopilotGate") { autopilotGate = URL(fileURLWithPath: gate, isDirectory: true) } diff --git a/ios/HouseScan/Runtime/ScanEngine.swift b/ios/HouseScan/Runtime/ScanEngine.swift index 388f6e516..04c9d4edc 100644 --- a/ios/HouseScan/Runtime/ScanEngine.swift +++ b/ios/HouseScan/Runtime/ScanEngine.swift @@ -827,13 +827,23 @@ final class ScanEngine { Task { // The task can start after a reset or after the flow left the close-up. guard scan == generation, state.phase == .meterCloseUp else { return } - let photo = await closeUpPhoto(frame) + var photo = await closeUpPhoto(frame) + if options.failCloseUpSave, replay != nil { photo.jpeg = .none } // Drawing a practice photo suspends: a reset meanwhile must not save into the new scan. guard scan == generation, state.phase == .meterCloseUp else { return } let saved = await store.saveStill(photo, name: "meter_close.jpg") guard scan == generation, state.phase == .meterCloseUp else { return } - if !saved { - retakeCloseUp(.blurry) + switch saved { + case .saved: + break + case .notWritten: + retakeCloseUp(.photoNotSaved) + return + case .worldDiscarded: + // The discard sent the flow back to finding the meter, so the phase guard above + // returns first. Past it, the meter was found again while this file was written: + // the shot belongs to the old frame, and the new close-up is left to take its own. + RuntimeLog.engine.info("close-up from a discarded world frame dropped") return } let thumbnail = await store.thumbnail(ofStill: "meter_close.jpg") @@ -860,7 +870,7 @@ final class ScanEngine { closeUpCredit.photoChecked(view, passed: readout?.photoPassedChecks == true) guard let readout else { RuntimeLog.engine.error("close-up photo could not be read back for the meter number") - retakeCloseUp(.noNumber) + retakeCloseUp(.photoNotSaved) return } guard !readout.candidates.isEmpty else { diff --git a/ios/HouseScan/UI/Copy/ScanCopy.swift b/ios/HouseScan/UI/Copy/ScanCopy.swift index 904b82fc8..b59b8b29d 100644 --- a/ios/HouseScan/UI/Copy/ScanCopy.swift +++ b/ios/HouseScan/UI/Copy/ScanCopy.swift @@ -260,6 +260,7 @@ enum ScanCopy { case .tracking: "Move slowly" case .numberTooSmall: "Move closer so the number looks bigger" case .noNumber: "Couldn't read the number. Hold still for another photo." + case .photoNotSaved: "Your phone couldn't save that photo. Hold on the meter to try again." } } diff --git a/ios/HouseScanUITests/CloseUpSaveFailureUITests.swift b/ios/HouseScanUITests/CloseUpSaveFailureUITests.swift new file mode 100644 index 000000000..e3d953752 --- /dev/null +++ b/ios/HouseScanUITests/CloseUpSaveFailureUITests.swift @@ -0,0 +1,49 @@ +import XCTest + +/// B-20: a meter close-up the phone couldn't save read "Hold still", as if the homeowner's hand +/// had blurred it, and retaking with a steadier hand could never help. `-failCloseUpSave` makes +/// every close-up's save fail on a replay, through the real engine and `KeyframeStore.saveStill`. +final class CloseUpSaveFailureUITests: XCTestCase { + static let notSaved = "Your phone couldn't save that photo. Hold on the meter to try again." + + override func setUp() { + continueAfterFailure = false + } + + @MainActor + func testAPhotoThatDidNotSaveSaysSoAndCanBeSkipped() throws { + let app = XCUIApplication() + app.launchArguments = ["-replay", FullFlowUITests.fixture, "-sampleResult", "-failCloseUpSave"] + app.launch() + let any = app.descendants(matching: .any) + + XCTAssertTrue(any["screen.onboarding"].waitForExistence(timeout: 30)) + if app.buttons["action.onboardingSkip"].waitForExistence(timeout: 5) { app.buttons["action.onboardingSkip"].tap() } + XCTAssertTrue(app.buttons["action.finishOnboarding"].waitForExistence(timeout: 10)) + app.buttons["action.finishOnboarding"].tap() + XCTAssertTrue(app.buttons["action.markMeter"].waitForExistence(timeout: 20)) + app.buttons["action.markMeter"].tap() + XCTAssertTrue(any["screen.meterCloseUp"].waitForExistence(timeout: 15)) + + // The reason stays up for `ScanEngine.retakeNotice` (2 s) before the gate's own problems + // can replace it, longer than the query's polling. + let saidNotSaved = any.matching(identifier: "closeUp.problem").matching(NSPredicate(format: "label == %@", Self.notSaved)).firstMatch + XCTAssertTrue(saidNotSaved.waitForExistence(timeout: 60), "a close-up that didn't save never said so") + snap(app, "meterCloseUp-photoNotSaved") + + // The save failure counted as one failed attempt; the next one (another failed save, or + // `CloseUpGate.failAfter` without a photo) brings the way out. + let skip = app.buttons["action.skipCloseUp"] + XCTAssertTrue(skip.waitForExistence(timeout: 90), "no way past a close-up that can't be saved") + skip.tap() + XCTAssertTrue(any["screen.wallWalk"].waitForExistence(timeout: 15), "skipping the unsaved close-up didn't lead on to the walk") + } + + @MainActor + private func snap(_ app: XCUIApplication, _ name: String) { + let shot = XCTAttachment(screenshot: app.screenshot()) + shot.name = name + shot.lifetime = .keepAlways + add(shot) + } +} From f44f603736dcf8a98e9bfbbd74b1786adcdf1aed Mon Sep 17 00:00:00 2001 From: SamGu-NRX Date: Sat, 3 Oct 2026 04:22:38 -0500 Subject: [PATCH 2/2] ios: a close-up is acknowledged only once it reads back; the hook holds its reason up Review of 0348628d: a photo that saved but couldn't be read back still flashed as a capture and counted, then asked for a retake. The close-up is now read back once, before the acknowledgment, and those bytes feed both the thumbnail and the meter reader, so `KeyframeStore.thumbnail(ofStill:)` goes. The replay is close to the meter for under a second, so after the 2 s notice the gate's "Move closer" replaced the save-failure reason before the UI test could be sure to see it. Under `-failCloseUpSave` only, the reason stays up 10 s. Co-Authored-By: Claude Opus 5.5 (1M context) --- ios/HouseScan/Runtime/KeyframeStore.swift | 8 ---- ios/HouseScan/Runtime/LaunchOptions.swift | 1 + ios/HouseScan/Runtime/ScanEngine.swift | 45 +++++++++++-------- .../CloseUpSaveFailureUITests.swift | 4 +- 4 files changed, 29 insertions(+), 29 deletions(-) diff --git a/ios/HouseScan/Runtime/KeyframeStore.swift b/ios/HouseScan/Runtime/KeyframeStore.swift index a5aee22e8..7bb4383df 100644 --- a/ios/HouseScan/Runtime/KeyframeStore.swift +++ b/ios/HouseScan/Runtime/KeyframeStore.swift @@ -222,14 +222,6 @@ final class KeyframeStore { stillCatalog.withdraw(Self.purpose(of: name)) } - func thumbnail(ofStill name: String) async -> CGImage? { - let url = directory.appending(path: name) - return await Task.detached(priority: .utility) { () -> CGImage? in - guard let data = try? Data(contentsOf: url) else { return nil } - return ImageWork.uprightThumbnail(jpeg: data) - }.value - } - /// Forgets keyframes and stills taken in a world frame that no longer exists (after a failed /// relocalization), the meter close-up included: export and the packet read only what /// `keyframes`, `stills` and `stillFrames` list, so a close-up skipped in the new frame exports diff --git a/ios/HouseScan/Runtime/LaunchOptions.swift b/ios/HouseScan/Runtime/LaunchOptions.swift index 8c17e9fc6..884e380dd 100644 --- a/ios/HouseScan/Runtime/LaunchOptions.swift +++ b/ios/HouseScan/Runtime/LaunchOptions.swift @@ -31,6 +31,7 @@ import OSLog /// - `-failCloseUpSave`: with `-replay`, every meter close-up's photo fails to save, as on a phone /// with no space left, so a UI test reaches the save-failure retake and the skip after it. The /// photo is dropped before `KeyframeStore.saveStill`, which then takes its own failure path. +/// The retake reason stays up 10 s instead of 2, so the test's query can't miss it. /// - `-simulateAppStore`: run as an App Store install would, so the developer options and practice /// meter are unavailable whatever the stored switch says (`DeveloperSettings`). It can only take /// the switch away, never offer it. diff --git a/ios/HouseScan/Runtime/ScanEngine.swift b/ios/HouseScan/Runtime/ScanEngine.swift index 04c9d4edc..cf1c55d35 100644 --- a/ios/HouseScan/Runtime/ScanEngine.swift +++ b/ios/HouseScan/Runtime/ScanEngine.swift @@ -756,7 +756,7 @@ final class ScanEngine { state.coaching = coaching(for: frame.tracking, skip: nil) // After a retake request the shutter waits long enough for the reason to be read (and, // for "move closer", acted on) before the hold can start again. - if let retake = closeUpRetake, screenTime - retake.since < Self.retakeNotice { + if let retake = closeUpRetake, screenTime - retake.since < currentRetakeNotice { state.closeUp = .aiming(hold: 0, problem: retake.problem) return } @@ -786,6 +786,10 @@ final class ScanEngine { /// Seconds a retake reason stays up before the next close-up can be taken. A guess to try /// on a phone, not measured: long enough to read one short line. private static let retakeNotice: Double = 2 + /// Under `-failCloseUpSave` the reason stays up 10 s, so the UI test's query can't miss it: + /// the replay is close to the meter for under a second, and after the 2 s notice the gate's + /// "Move closer" replaces it. + private var currentRetakeNotice: Double { options.failCloseUpSave && replay != nil ? 10 : Self.retakeNotice } private var screenTime: Double { Double(DispatchTime.now().uptimeNanoseconds) / 1_000_000_000 } @@ -846,33 +850,36 @@ final class ScanEngine { RuntimeLog.engine.info("close-up from a discarded world frame dropped") return } - let thumbnail = await store.thumbnail(ofStill: "meter_close.jpg") + // Read back once, before the capture is acknowledged: the thumbnail and the meter + // reader both use these bytes, and a photo that can't be read back wasn't kept. + let file = store.directory.appending(path: "meter_close.jpg") + let readBack = await Task.detached(priority: .userInitiated) { () -> (jpeg: Data, thumbnail: CGImage?)? in + guard let jpeg = try? Data(contentsOf: file) else { return nil } + return (jpeg, ImageWork.uprightThumbnail(jpeg: jpeg)) + }.value guard scan == generation, state.phase == .meterCloseUp else { return } - state.closeUp = .captured(thumbnail) + guard let readBack else { + RuntimeLog.engine.error("close-up photo could not be read back") + closeUpCredit.photoChecked(view, passed: false) + retakeCloseUp(.photoNotSaved) + return + } + state.closeUp = .captured(readBack.thumbnail) state.captureCount += 1 - state.lastCapture = CaptureEvent(id: state.captureCount, kind: .closeUp, thumbnail: thumbnail) - await readMeterNumber(scan: scan, view: view) + state.lastCapture = CaptureEvent(id: state.captureCount, kind: .closeUp, thumbnail: readBack.thumbnail) + await readMeterNumber(readBack.jpeg, scan: scan, view: view) } } - /// Reads the meter number from the saved close-up, off the main actor, then offers the - /// candidates for the homeowner to pick from (never filling one in) or asks for a retake. + /// Reads the meter number from the saved close-up's bytes, off the main actor, then offers + /// the candidates for the homeowner to pick from (never filling one in) or asks for a retake. /// `view` is the shot's, which coverage may take once the reader has passed its photo. - private func readMeterNumber(scan: Int, view: CloseUpView?) async { + private func readMeterNumber(_ jpeg: Data, scan: Int, view: CloseUpView?) async { state.meterNumber = .reading let reader = MeterNumberReaders.make() - let photo = store.directory.appending(path: "meter_close.jpg") - let readout = await Task.detached(priority: .userInitiated) { () -> MeterReadout? in - guard let jpeg = try? Data(contentsOf: photo) else { return nil } - return await reader.read(jpeg: jpeg) - }.value + let readout = await Task.detached(priority: .userInitiated) { await reader.read(jpeg: jpeg) }.value guard scan == generation, state.phase == .meterCloseUp, state.meterNumber == .reading else { return } - closeUpCredit.photoChecked(view, passed: readout?.photoPassedChecks == true) - guard let readout else { - RuntimeLog.engine.error("close-up photo could not be read back for the meter number") - retakeCloseUp(.photoNotSaved) - return - } + closeUpCredit.photoChecked(view, passed: readout.photoPassedChecks) guard !readout.candidates.isEmpty else { retakeCloseUp(readout.retake ?? .noNumber) return diff --git a/ios/HouseScanUITests/CloseUpSaveFailureUITests.swift b/ios/HouseScanUITests/CloseUpSaveFailureUITests.swift index e3d953752..308af7c9f 100644 --- a/ios/HouseScanUITests/CloseUpSaveFailureUITests.swift +++ b/ios/HouseScanUITests/CloseUpSaveFailureUITests.swift @@ -25,8 +25,8 @@ final class CloseUpSaveFailureUITests: XCTestCase { app.buttons["action.markMeter"].tap() XCTAssertTrue(any["screen.meterCloseUp"].waitForExistence(timeout: 15)) - // The reason stays up for `ScanEngine.retakeNotice` (2 s) before the gate's own problems - // can replace it, longer than the query's polling. + // Under `-failCloseUpSave` the reason stays up 10 s rather than 2 before the gate's own + // "Move closer" replaces it, so this query can't miss it. let saidNotSaved = any.matching(identifier: "closeUp.problem").matching(NSPredicate(format: "label == %@", Self.notSaved)).firstMatch XCTAssertTrue(saidNotSaved.waitForExistence(timeout: 60), "a close-up that didn't save never said so") snap(app, "meterCloseUp-photoNotSaved")