diff --git a/src/layers/streetLevel/cameraFollow.js b/src/layers/streetLevel/cameraFollow.js index dcb4ee7dd..03b1eaa2e 100644 --- a/src/layers/streetLevel/cameraFollow.js +++ b/src/layers/streetLevel/cameraFollow.js @@ -70,11 +70,26 @@ export function createCameraFollow({ state, parts }) { requestRender(); } + /** + * The framing flight this module started, while it is still the camera's + * current flight. Cesium cancels it (calling `cancel`) when any other + * flight starts, so a stale token never cancels someone else's flight. + * @type {object|null} + */ + let framing = null; + /** Frame the current image from a short distance behind it. */ function lookAtPosition() { const { position, bearing, altitude } = state.street; if (!position || !state.viewer) return; const ground = groundHeightAt(position.lon, position.lat, altitude); + // Owned before the call: starting it cancels the previous flight (ours + // included) synchronously, and a zero-length one completes at once. + const flight = {}; + framing = flight; + const release = () => { + if (framing === flight) framing = null; + }; state.viewer.camera.flyToBoundingSphere( new Cesium.BoundingSphere( Cesium.Cartesian3.fromDegrees(position.lon, position.lat, ground + 2), @@ -87,15 +102,32 @@ export function createCameraFollow({ state, parts }) { 140, ), duration: 1.6, + complete: release, + cancel: release, }, ); } + /** + * Stop the framing flight if it is still in the air (the image closed or + * the layer went off). A flight another feature started since is left be. + */ + function cancelFraming() { + if (!framing) return; + // Give up ownership first: cancelFlight delivers `cancel` synchronously. + framing = null; + state.viewer?.camera?.cancelFlight?.(); + } + function setFollow(enabled) { state.street.follow = enabled === true && state.street.followAvailable; - if (state.street.follow) followCamera(); + // The framing tween would keep overwriting the follow view until it lands. + if (state.street.follow) { + cancelFraming(); + followCamera(); + } state.notify?.(); } - return { followCamera, lookAtPosition, setFollow }; + return { followCamera, lookAtPosition, cancelFraming, setFollow }; } diff --git a/src/layers/streetLevel/cameraFollow.test.mjs b/src/layers/streetLevel/cameraFollow.test.mjs index ccc162a21..16a45c1e2 100644 --- a/src/layers/streetLevel/cameraFollow.test.mjs +++ b/src/layers/streetLevel/cameraFollow.test.mjs @@ -8,6 +8,40 @@ import { FOLLOW_EYE_HEIGHT_M } from './policy.js'; const heightOf = (cartesian) => Cesium.Cartographic.fromCartesian(cartesian).height; +/** + * A camera with Cesium's flight bookkeeping: starting a flight cancels the + * current one (calling its `cancel`), `cancelFlight` cancels whichever flight + * is current, whoever started it, and `land()` completes it. `cancelled` + * counts the flights `cancelFlight` stopped. + */ +function flightCamera(flights, views) { + let current = null; + function stop() { + const flight = current; + current = null; + flight?.options.cancel?.(); + return Boolean(flight); + } + const camera = { + cancelled: 0, + flyToBoundingSphere(sphere, options = {}) { + stop(); + current = { sphere, options }; + flights.push(current); + }, + cancelFlight() { + if (stop()) camera.cancelled++; + }, + land() { + const flight = current; + current = null; + flight?.options.complete?.(); + }, + setView: (view) => views.push(view), + }; + return camera; +} + /** * A viewer whose scene sample is `sampled` (the Austin case: −14,886 m before * the tiles under the photo had loaded) and whose bare earth is `dem`. @@ -31,11 +65,7 @@ function setup({ sampled, dem = null, globe = null, altitude = 149 }) { sampleHeight: () => sampled, globe: { getHeight: () => globe }, }, - camera: { - flyToBoundingSphere: (sphere, options) => - flights.push({ sphere, options }), - setView: (view) => views.push(view), - }, + camera: flightCamera(flights, views), }, }; const parts = { @@ -87,3 +117,44 @@ test('following stands the camera at eye height above the checked ground', () => 0.01, ); }); + +test('cancelFraming stops the framing flight while it is still ours (review IC8 P1)', () => { + const { follow, state } = setup({ sampled: 121, dem: 117 }); + const { camera } = state.viewer; + follow.lookAtPosition(); + follow.cancelFraming(); + assert.equal(camera.cancelled, 1, 'the flight toward the closed photo stops'); + follow.cancelFraming(); + assert.equal(camera.cancelled, 1, 'and only once'); + + // Re-framing (the next photo) supersedes the first flight, not the claim. + follow.lookAtPosition(); + follow.lookAtPosition(); + follow.cancelFraming(); + assert.equal(camera.cancelled, 2); +}); + +test('cancelFraming leaves a landed flight and a newer navigation flight alone (review IC8 P1)', () => { + const { follow, state, flights } = setup({ sampled: 121, dem: 117 }); + const { camera } = state.viewer; + follow.lookAtPosition(); + camera.land(); + follow.cancelFraming(); + assert.equal(camera.cancelled, 0, 'landed: nothing to stop'); + + follow.lookAtPosition(); + // A search result flies the globe elsewhere; Cesium cancels ours first. + camera.flyToBoundingSphere(null, {}); + assert.equal(flights.length, 3); + follow.cancelFraming(); + assert.equal(camera.cancelled, 0, 'the newer flight keeps going'); +}); + +test('turning FOLLOW on stops the framing flight so it cannot fight the follow view', () => { + const { follow, state, views } = setup({ sampled: 121, dem: 117 }); + const { camera } = state.viewer; + follow.lookAtPosition(); + follow.setFollow(true); + assert.equal(camera.cancelled, 1, 'the framing tween stops'); + assert.equal(views.length, 1, 'and the camera stands at the photo'); +}); diff --git a/src/layers/streetLevel/index.js b/src/layers/streetLevel/index.js index 8da2e370f..d4641916b 100644 --- a/src/layers/streetLevel/index.js +++ b/src/layers/streetLevel/index.js @@ -239,7 +239,21 @@ export function createStreetLevelLayer({ parts.credits.hide(state.viewer, entry.def); } + /** + * The nearest-image lookup in flight. Anything newer the user asks for (a + * later lookup, a picked image, closing, the layer going off) aborts it, + * so its late answer can never replace that choice. + * @type {AbortController|null} + */ + let nearestLookup = null; + + function abortNearest() { + nearestLookup?.abort(); + nearestLookup = null; + } + function openImage(providerId, imageId) { + abortNearest(); return parts.viewerHost.open(providerId, imageId); } @@ -358,6 +372,7 @@ export function createStreetLevelLayer({ disable() { state.enabled = false; + abortNearest(); unwatchSurface(); parts.viewerHost.unmount(); for (const entry of state.providers.values()) entry.instance.deactivate(); @@ -461,6 +476,10 @@ export function createStreetLevelLayer({ const view = point || viewCentre(state.viewer); if (!Number.isFinite(view?.lat) || !Number.isFinite(view?.lon)) return false; + abortNearest(); + const lookup = new AbortController(); + nearestLookup = lookup; + const { signal } = lookup; state.street.loading = true; state.street.error = null; notify(); @@ -468,20 +487,25 @@ export function createStreetLevelLayer({ for (const entry of activeEntries()) { let imageId = null; try { - imageId = await entry.instance.nearestImage({ - lat: view.lat, - lon: view.lon, - }); + imageId = await entry.instance.nearestImage( + { lat: view.lat, lon: view.lon }, + { signal }, + ); } catch (error) { + // Overtaken: whatever replaced it owns the panel, errors included. + if (signal.aborted) return false; // One provider failing (no key, offline) must not hide the others. lastError = error; continue; } + if (signal.aborted) return false; if (!state.enabled) break; // This provider was switched off while it looked: try the next. if (!imageId || !entry.on) continue; + nearestLookup = null; // answered: nothing left to abort return openImage(entry.def.id, imageId); } + if (nearestLookup === lookup) nearestLookup = null; // Switched off while it looked: nothing to open, nothing to report. if (state.enabled && activeEntries().length) state.street.error = @@ -493,6 +517,7 @@ export function createStreetLevelLayer({ }, /** Close the image and deselect it everywhere on the map. */ closeViewer() { + abortNearest(); parts.viewerHost.close(); parts.clearSequences(); }, diff --git a/src/layers/streetLevel/index.test.mjs b/src/layers/streetLevel/index.test.mjs index 63c1852ff..47429a929 100644 --- a/src/layers/streetLevel/index.test.mjs +++ b/src/layers/streetLevel/index.test.mjs @@ -3,6 +3,7 @@ import test from 'node:test'; import * as Cesium from 'cesium'; import { createStreetLevelLayer } from './index.js'; import { MAPILLARY_CREDIT_HTML } from './providers/mapillary/policy.js'; +import { createMapillaryProvider } from './providers/mapillary/index.js'; import { fakeStreetLevelProvider as fakeProvider } from '../../testSupport/streetLevelFakes.mjs'; /** @@ -11,6 +12,15 @@ import { fakeStreetLevelProvider as fakeProvider } from '../../testSupport/stree */ function fakeViewer() { const credits = []; + /** Cesium's flight bookkeeping: a new flight cancels the current one. */ + let flight = null; + const flights = { started: 0, cancelled: 0 }; + const stopFlight = () => { + const current = flight; + flight = null; + current?.cancel?.(); + return Boolean(current); + }; const canvas = Object.assign(new EventTarget(), { style: {}, // Keep Cesium's handler on the canvas; there is no real document here. @@ -19,11 +29,27 @@ function fakeViewer() { }); return { credits, - scene: { canvas, primitives: { add: (p) => p, remove() {} } }, + flights, + scene: { + canvas, + primitives: { add: (p) => p, remove() {} }, + // Enough of a scene for the position marker to clamp to the ground. + frameState: { mode: Cesium.SceneMode.SCENE3D }, + updateHeight: () => () => {}, + getHeight: () => undefined, + }, camera: { changed: new Cesium.Event(), moveStart: new Cesium.Event(), moveEnd: new Cesium.Event(), + flyToBoundingSphere(sphere, options = {}) { + stopFlight(); + flight = options; + flights.started++; + }, + cancelFlight() { + if (stopFlight()) flights.cancelled++; + }, }, creditDisplay: { addStaticCredit: (credit) => credits.push(credit), @@ -36,7 +62,11 @@ function fakeViewer() { /** A layer over `providers`, initialised and enabled on a stand-in viewer. */ async function enabledLayer(t, providers = [fakeProvider()]) { const saved = globalThis.document; - globalThis.document = new EventTarget(); + // A pose moves the position marker, whose glyph is drawn on a canvas. + const drawing = new Proxy({}, { get: () => () => ({ addColorStop() {} }) }); + globalThis.document = Object.assign(new EventTarget(), { + createElement: () => ({ getContext: () => drawing }), + }); const viewer = fakeViewer(); const layer = createStreetLevelLayer({ providers }); layer.init(viewer); @@ -53,10 +83,50 @@ const settle = () => new Promise((resolve) => setTimeout(resolve, 0)); function deferred() { let resolve; - const promise = new Promise((done) => { + let reject; + const promise = new Promise((done, fail) => { resolve = done; + reject = fail; }); - return { promise, resolve }; + return { promise, resolve, reject }; +} + +/** A provider whose viewer reports a pose for every image it opens. */ +function posingProvider() { + const provider = fakeProvider(); + const create = provider.create; + provider.create = (context) => { + const instance = create(context); + let emit = null; + instance.viewer.onPose = (listener) => { + emit = listener; + return () => { + emit = null; + }; + }; + instance.viewer.open = async (imageId) => { + provider.calls.open.push(imageId); + emit?.({ + providerId: 'mapillary', + imageId, + position: { lon: -121.49, lat: 38.58 }, + bearing: 90, + }); + }; + return instance; + }; + return provider; +} + +/** nearestImage lookups that answer when the test says, with their signals. */ +function slowLookups() { + const lookups = []; + const nearestImage = (point, { signal } = {}) => { + const answer = deferred(); + lookups.push({ point, signal, answer }); + return answer.promise; + }; + return { lookups, nearestImage }; } /** A map stack controller that can be switched between stacks. */ @@ -227,3 +297,125 @@ test('a provider withdraws only the error it reported (review P3)', async (t) => actions.reportError(null); assert.match(layer.getUIState().street.error, /Open the Street Level panel/); }); + +test('closing the photo stops the globe flying to it (review IC8 P1)', async (t) => { + const { layer, viewer } = await enabledLayer(t, [posingProvider()]); + layer.attachViewerHost({}); + assert.equal(await layer.openImage('mapillary', 'img1'), true); + assert.equal(viewer.flights.started, 1, 'the photo is framed'); + layer.closeViewer(); + assert.equal(viewer.flights.cancelled, 1); +}); + +test('switching the layer off or destroying it stops the framing flight (review IC8 P1)', async (t) => { + const { layer, viewer } = await enabledLayer(t, [posingProvider()]); + layer.attachViewerHost({}); + await layer.openImage('mapillary', 'img1'); + layer.disable(); + assert.equal(viewer.flights.cancelled, 1, 'layer off'); + + layer.enable(viewer); + await layer.openImage('mapillary', 'img2'); + layer.destroy(); + assert.equal(viewer.flights.cancelled, 2, 'destroyed'); +}); + +test('closing the photo leaves a newer navigation flight alone (review IC8 P1)', async (t) => { + const { layer, viewer } = await enabledLayer(t, [posingProvider()]); + layer.attachViewerHost({}); + await layer.openImage('mapillary', 'img1'); + assert.equal(viewer.flights.started, 1, 'the photo is being framed'); + // A search result flies the globe elsewhere before the framing lands. + viewer.camera.flyToBoundingSphere(null, {}); + layer.closeViewer(); + assert.equal(viewer.flights.cancelled, 0, 'the search flight keeps going'); +}); + +test('an older nearest lookup that answers late cannot replace a newer one (review IC8 P2)', async (t) => { + const { lookups, nearestImage } = slowLookups(); + const provider = fakeProvider({ nearestImage }); + const { layer } = await enabledLayer(t, [provider]); + layer.attachViewerHost({}); + const older = layer.openNearest({ lat: 38.58, lon: -121.49 }); + const newer = layer.openNearest({ lat: 38.59, lon: -121.48 }); + lookups[1].answer.resolve('newer'); + assert.equal(await newer, true); + lookups[0].answer.resolve('older'); + assert.equal(await older, false); + assert.deepEqual(provider.calls.open, ['newer']); + assert.equal(lookups[0].signal?.aborted, true, 'its request was aborted'); + assert.equal(lookups[1].signal?.aborted, false); +}); + +test('an image picked during a nearest lookup wins over its late answer (review IC8 P2)', async (t) => { + const { lookups, nearestImage } = slowLookups(); + const provider = fakeProvider({ nearestImage }); + const { layer } = await enabledLayer(t, [provider]); + layer.attachViewerHost({}); + const lookup = layer.openNearest({ lat: 38.58, lon: -121.49 }); + // A cone click goes through the provider's openImage action. + assert.equal(await provider.context().actions.openImage('picked'), true); + lookups[0].answer.resolve('nearest'); + assert.equal(await lookup, false); + assert.deepEqual(provider.calls.open, ['picked']); + assert.equal(lookups[0].signal?.aborted, true); +}); + +test('closing the viewer or switching the layer off retires a nearest lookup (review IC8 P2)', async (t) => { + const { lookups, nearestImage } = slowLookups(); + const provider = fakeProvider({ nearestImage }); + const { layer, viewer } = await enabledLayer(t, [provider]); + layer.attachViewerHost({}); + + const closed = layer.openNearest({ lat: 38.58, lon: -121.49 }); + layer.closeViewer(); + // The aborted fetch rejects, as fetch does; that is not the user's error. + lookups[0].answer.reject(new DOMException('aborted', 'AbortError')); + assert.equal(await closed, false); + let { street } = layer.getUIState(); + assert.deepEqual( + [street.open, street.loading, street.error], + [false, false, null], + ); + assert.equal(lookups[0].signal?.aborted, true, 'closing aborts the request'); + + const disabled = layer.openNearest({ lat: 38.58, lon: -121.49 }); + layer.disable(); + layer.enable(viewer); + lookups[1].answer.resolve('img1'); + assert.equal(await disabled, false, 'nor does it open once back on'); + assert.deepEqual(provider.calls.open, []); + ({ street } = layer.getUIState()); + assert.deepEqual( + [street.open, street.loading, street.error], + [false, false, null], + ); + assert.equal(lookups[1].signal?.aborted, true, 'layer off aborts it too'); +}); + +test('the Mapillary provider hands the lookup signal to its source (review IC8 P2)', async () => { + const requests = []; + const source = { + hasToken: () => true, + getStatus: async () => ({ configured: true }), + getTile: async () => new Uint8Array(0), + getSequenceImages: async () => [], + nearestImages: async (query, options) => { + requests.push(options); + return [{ id: 7, is_pano: false, captured_at: 10 }]; + }, + }; + const instance = createMapillaryProvider({ source }).create({ + services: {}, + getFilter: () => ({ pano: 'all', sinceMs: null }), + isActive: () => true, + notify() {}, + actions: { openImage() {}, reportError() {} }, + }); + const { signal } = new AbortController(); + assert.equal( + await instance.nearestImage({ lat: 1, lon: 2 }, { signal }), + '7', + ); + assert.equal(requests[0]?.signal, signal); +}); diff --git a/src/layers/streetLevel/providers/mapillary/index.js b/src/layers/streetLevel/providers/mapillary/index.js index da4edafe1..3a5b5aca6 100644 --- a/src/layers/streetLevel/providers/mapillary/index.js +++ b/src/layers/streetLevel/providers/mapillary/index.js @@ -180,14 +180,15 @@ export function createMapillaryProvider({ source }) { loading: state.sequence.loading, }), - /** Nearest image that passes the imagery filter, or null. */ - async nearestImage({ lat, lon }) { - const images = await source.nearestImages({ - lat, - lon, - radius: NEAREST_RADIUS_M, - limit: NEAREST_LIMIT, - }); + /** + * Nearest image that passes the imagery filter, or null. The core + * aborts `signal` once a newer request overtakes the lookup. + */ + async nearestImage({ lat, lon }, { signal } = {}) { + const images = await source.nearestImages( + { lat, lon, radius: NEAREST_RADIUS_M, limit: NEAREST_LIMIT }, + { signal }, + ); // The API returns the images in the radius in no particular order. const metres = (record) => { const [lon2, lat2] = (record.computed_geometry || record.geometry) diff --git a/src/layers/streetLevel/registry.js b/src/layers/streetLevel/registry.js index 0f26f8366..eaa4961db 100644 --- a/src/layers/streetLevel/registry.js +++ b/src/layers/streetLevel/registry.js @@ -35,7 +35,7 @@ * @property {(sequenceId: string) => Promise} [selectSequence] * @property {() => void} [clearSequence] * @property {() => {selectedId: string|null, images: number, loading: boolean}} [sequenceStats] - * @property {(point: {lat: number, lon: number}) => Promise} nearestImage + * @property {(point: {lat: number, lon: number}, options?: {signal?: AbortSignal}) => Promise} nearestImage An aborted lookup gives up (it may reject with an AbortError). * @property {ViewerAdapter} viewer * * @typedef {object} GroundCaster diff --git a/src/layers/streetLevel/viewerHost.js b/src/layers/streetLevel/viewerHost.js index 19cf80160..068318880 100644 --- a/src/layers/streetLevel/viewerHost.js +++ b/src/layers/streetLevel/viewerHost.js @@ -153,9 +153,13 @@ export function createViewerHost({ state, parts }) { return true; } - /** Close the image; the mounted adapter stays warm for the next open. */ + /** + * Close the image; the mounted adapter stays warm for the next open. The + * globe stops flying toward it too (unmount closes, so layer off does). + */ function close() { openSeq++; + parts.follow.cancelFraming(); active?.adapter.close(); Object.assign(state.street, freshStreet()); parts.marker.clear(); diff --git a/src/layers/streetLevel/viewerHost.test.mjs b/src/layers/streetLevel/viewerHost.test.mjs index f50f02f9c..371e94ae6 100644 --- a/src/layers/streetLevel/viewerHost.test.mjs +++ b/src/layers/streetLevel/viewerHost.test.mjs @@ -39,11 +39,16 @@ function harness(adapter) { }; const def = { id: 'mapillary', name: 'Mapillary', label: 'MAPILLARY' }; state.providers.set('mapillary', { def, instance: { viewer: adapter } }); + const framing = { started: 0, cancelled: 0 }; const parts = { marker: { set() {}, clear() {} }, - follow: { followCamera() {}, lookAtPosition() {} }, + follow: { + followCamera() {}, + lookAtPosition: () => framing.started++, + cancelFraming: () => framing.cancelled++, + }, }; - return { state, host: createViewerHost({ state, parts }) }; + return { state, framing, host: createViewerHost({ state, parts }) }; } /** @@ -318,3 +323,17 @@ test('closing the photo stops sequence playback in the hidden viewer (review P2- assert.equal(viewer.playback.stops, 1, 'playback stopped with the photo'); assert.equal(viewers.live, 1, 'the viewer itself stays warm'); }); + +test('closing the photo or switching the layer off stops its framing flight (review IC8 P1)', async () => { + const adapter = fakeAdapter(); + const { framing, host } = harness(adapter); + await host.open('mapillary', 'a'); + assert.equal(framing.started, 1, 'the open framed the photo'); + host.close(); + assert.equal(framing.cancelled, 1, 'closed: the globe stops flying to it'); + await host.open('mapillary', 'b'); + host.unmount('panoramax'); + assert.equal(framing.cancelled, 1, 'another provider leaves it alone'); + host.unmount(); + assert.equal(framing.cancelled, 2, 'layer off: likewise'); +});