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..7bb4383df 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 @@ -197,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 3d659c6ed..884e380dd 100644 --- a/ios/HouseScan/Runtime/LaunchOptions.swift +++ b/ios/HouseScan/Runtime/LaunchOptions.swift @@ -28,6 +28,10 @@ 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. +/// 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. @@ -42,6 +46,7 @@ struct LaunchOptions: Equatable { var autopilotSomethingThere = false var simulateAppStore = false var injectGroundRise: Float? + var failCloseUpSave = false init( arguments: [String] = ProcessInfo.processInfo.arguments, @@ -58,6 +63,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..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 } @@ -827,42 +831,55 @@ 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") + // 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(.noNumber) - return - } + closeUpCredit.photoChecked(view, passed: readout.photoPassedChecks) guard !readout.candidates.isEmpty else { retakeCloseUp(readout.retake ?? .noNumber) return 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..308af7c9f --- /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)) + + // 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") + + // 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) + } +}