From abbf69a5120df7d9a0d7697ddbccb87254280de5 Mon Sep 17 00:00:00 2001 From: Said Turksever Date: Fri, 2 Oct 2026 01:01:40 +0100 Subject: [PATCH] fix(street-level): camera height over Google 3D, cast only when it helps, gate token From the code review of #768: - view.js measured the camera's height above ground with globe.getHeight, but Google 3D hides the globe, so coverage picked its zoom from the ellipsoidal height. Over a high city (Denver, ~1,600 m) a camera 500 m up read 2,100 m: coverage stayed at z12/z13, whose tiles are too big to cast, and lines stayed draped on the roofs. With the globe hidden, the bare-earth height from the ground caster now answers; coverage re-picks its zoom when the surface mode changes. Verified in Denver: terrain mode, z14, cast lines. The core's surface check uses the same helper (and no longer subtracts the 2 m lift from the camera height). - castTile rebuilt a tile even when no line could be cast (terrain proxy down, tile too big); it now leaves such tiles as they are. - The feature gate read Street Level's share token as a literal `0`; it now takes it from LAYER_STATE_REGISTRY, since a rebase can reassign it. New view.test.mjs covers the shown-globe, hidden-globe and no-sample cases. Co-Authored-By: Claude Opus 5.5 --- scripts/format-scope.json | 1 + scripts/qa-street-level.mjs | 17 ++++--- src/layers/streetLevel/index.js | 21 ++++----- .../providers/mapillary/coverage.js | 16 +++++-- src/layers/streetLevel/view.js | 19 ++++++-- src/layers/streetLevel/view.test.mjs | 44 +++++++++++++++++++ 6 files changed, 96 insertions(+), 22 deletions(-) create mode 100644 src/layers/streetLevel/view.test.mjs diff --git a/scripts/format-scope.json b/scripts/format-scope.json index 432b7d274..dddf20ab3 100644 --- a/scripts/format-scope.json +++ b/scripts/format-scope.json @@ -901,6 +901,7 @@ "src/layers/streetLevel/pickRouter.test.mjs", "src/layers/streetLevel/uiState.test.mjs", "src/layers/streetLevel/groundCast.test.mjs", + "src/layers/streetLevel/view.test.mjs", "src/layers/streetLevel/providers/mapillary/provider.test.mjs", "src/layers/streetLevel/providers/mapillary/decode.test.mjs", "src/ui/panelDock.test.mjs", diff --git a/scripts/qa-street-level.mjs b/scripts/qa-street-level.mjs index 3de8e2df7..ecb40aa5c 100644 --- a/scripts/qa-street-level.mjs +++ b/scripts/qa-street-level.mjs @@ -9,6 +9,7 @@ */ import assert from 'node:assert/strict'; import { pathToFileURL } from 'node:url'; +import { LAYER_STATE_REGISTRY } from '../src/data/layerState.js'; /** Viewports every layout assertion runs at. */ export const VIEWPORTS = Object.freeze([ @@ -19,6 +20,11 @@ export const VIEWPORTS = Object.freeze([ /** Provider chips the panel must show, in order (one per registered provider). */ export const EXPECTED_PROVIDERS = Object.freeze(['mapillary']); +/** Street Level's share-link token, as the ledger assigned it. */ +const STREET_LEVEL_TOKEN = LAYER_STATE_REGISTRY.find( + (entry) => entry.id === 'street-level', +).token; + /** Expected right-rail order once the layout controller has run. */ export const RAIL_ORDER = Object.freeze([ 'pp-toggles', @@ -338,13 +344,14 @@ async function main() { assert.ok(after <= before, `${after} ≤ ${before}`); assert.equal((await ui()).filter.pano, 'pano'); // The click is a user params request, so the share link records it - // (Street Level's share token is `0`). + // under Street Level's share token, whatever the ledger assigned. await page.waitForFunction( - () => - /(^|_)0\.p\.p(_|$)/.test( - new URLSearchParams(location.hash.slice(1)).get('lo') || '', - ), + (option) => + (new URLSearchParams(location.hash.slice(1)).get('lo') || '') + .split('_') + .includes(option), { timeout: 10_000 }, + `${STREET_LEVEL_TOKEN}.p.p`, ); await page.click('[data-sl-pano="all"]'); await sleep(600); diff --git a/src/layers/streetLevel/index.js b/src/layers/streetLevel/index.js index 275dc5843..dba3f96ed 100644 --- a/src/layers/streetLevel/index.js +++ b/src/layers/streetLevel/index.js @@ -125,22 +125,23 @@ export function createStreetLevelLayer({ /** * Camera height above the bare earth under it, for the surface mode. The - * ground comes from the caster's coarse grid; a cold cell is fetched once - * and the mode checked again when it lands. + * ground comes from the caster's coarse grid; a cold cell near enough to + * matter is fetched once and the mode checked again when it lands. */ function cameraHeightForSurface() { const carto = state.viewer?.camera?.positionCartographic; if (!carto) return null; - const fallback = cameraHeightAboveGround(state.viewer); - if (!(fallback < SURFACE_WARM_BELOW_M)) return fallback; + const caster = parts.groundCaster; + const height = cameraHeightAboveGround(state.viewer, { + groundAt: caster.groundAt, + }); const lon = (carto.longitude * 180) / Math.PI; const lat = (carto.latitude * 180) / Math.PI; - const ground = parts.groundCaster.heightAt(lon, lat); - if (ground !== null) return carto.height - ground; - parts.groundCaster.prepare([[lon, lat]]).then((ready) => { - if (ready) scheduleSurfaceSync(); - }); - return fallback; + if (caster.groundAt(lon, lat) === null && height < SURFACE_WARM_BELOW_M) + caster.prepare([[lon, lat]]).then((ready) => { + if (ready) scheduleSurfaceSync(); + }); + return height; } /** diff --git a/src/layers/streetLevel/providers/mapillary/coverage.js b/src/layers/streetLevel/providers/mapillary/coverage.js index b0a3a9bf7..c9c23c730 100644 --- a/src/layers/streetLevel/providers/mapillary/coverage.js +++ b/src/layers/streetLevel/providers/mapillary/coverage.js @@ -172,9 +172,12 @@ export function createCoverage({ state, source }) { const lines = entry.sequenceList .filter((sequence) => passesImageryFilter(sequence, filter())) .map((sequence) => sequence.coordinates); - state.context.groundCaster.prepareLines(lines, { signal }).then(() => { + const caster = state.context.groundCaster; + caster.prepareLines(lines, { signal }).then(() => { const attached = [...state.coverage.tiles.values()].includes(entry); if (signal.aborted || !attached || !terrainMode()) return; + // Nothing new to cast (terrain proxy down, tile too big): keep it draped. + if (!lines.some((coords) => caster.castLine(coords))) return; // Keep the draped lines until the cast ones are built, so nothing blinks. const previous = entry.primitives; entry.primitives = []; @@ -393,7 +396,9 @@ export function createCoverage({ state, source }) { function refresh() { const viewer = state.viewer; if (!viewer || !state.context.isActive() || state.keyRequired) return; - const height = cameraHeightAboveGround(viewer); + const height = cameraHeightAboveGround(viewer, { + groundAt: state.context.groundCaster?.groundAt, + }); const sequenceZoom = coverageZoomForHeight(height); const overviewZoom = sequenceZoom ? null : overviewZoomForHeight(height); const zoom = sequenceZoom ?? overviewZoom; @@ -481,10 +486,15 @@ export function createCoverage({ state, source }) { notify(); } - /** Redraw loaded tiles draped or cast after the core's surface mode changes. */ + /** + * Redraw loaded tiles draped or cast after the core's surface mode changes, + * then re-pick the zoom: the bare-earth height under the camera that the + * change brings in can move it (a high city reads much closer to the street). + */ function setSurface() { if (!state.context.isActive()) return; rebuild(); + refresh(); } /** Look a sequence up across loaded tiles. */ diff --git a/src/layers/streetLevel/view.js b/src/layers/streetLevel/view.js index 56532cd94..4b114f777 100644 --- a/src/layers/streetLevel/view.js +++ b/src/layers/streetLevel/view.js @@ -1,13 +1,24 @@ import * as Cesium from 'cesium'; /** - * Camera height above the surface under the camera, in metres. Falls back to - * the ellipsoidal height when the globe has no height sample yet. + * Camera height above the surface under the camera, in metres. The globe's + * terrain answers when the globe is shown; Google 3D hides the globe, so + * there the bare-earth height from `groundAt(lon, lat)` answers instead + * (without it a camera over a city 1,600 m up reads 1,600 m too high). Falls + * back to the ellipsoidal height when neither has a sample yet. + * @param {object} viewer + * @param {{groundAt?: (lon: number, lat: number) => number|null}} [options] */ -export function cameraHeightAboveGround(viewer) { +export function cameraHeightAboveGround(viewer, { groundAt } = {}) { const carto = viewer?.camera?.positionCartographic; if (!carto) return null; - const ground = viewer.scene?.globe?.getHeight?.(carto); + const globe = viewer.scene?.globe; + let ground = globe?.show === false ? null : globe?.getHeight?.(carto); + if (!Number.isFinite(ground)) + ground = groundAt?.( + Cesium.Math.toDegrees(carto.longitude), + Cesium.Math.toDegrees(carto.latitude), + ); return carto.height - (Number.isFinite(ground) ? ground : 0); } diff --git a/src/layers/streetLevel/view.test.mjs b/src/layers/streetLevel/view.test.mjs new file mode 100644 index 000000000..c9c9997e6 --- /dev/null +++ b/src/layers/streetLevel/view.test.mjs @@ -0,0 +1,44 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { cameraHeightAboveGround } from './view.js'; + +const RAD = Math.PI / 180; +/** A viewer 300 m above a street 1,600 m up (Denver-like). */ +function viewer({ globeShown, globeHeight }) { + return { + camera: { + positionCartographic: { + longitude: -104.99 * RAD, + latitude: 39.74 * RAD, + height: 1900, + }, + }, + scene: { globe: { show: globeShown, getHeight: () => globeHeight } }, + }; +} + +test('a shown globe answers the ground under the camera', () => { + assert.equal( + cameraHeightAboveGround(viewer({ globeShown: true, globeHeight: 1600 })), + 300, + ); +}); + +test('a hidden globe (Google 3D) falls back to the bare-earth height', () => { + const calls = []; + const groundAt = (lon, lat) => { + calls.push([+lon.toFixed(2), +lat.toFixed(2)]); + return 1600; + }; + // A hidden globe's getHeight is ignored even when it returns a number. + const hidden = viewer({ globeShown: false, globeHeight: 0 }); + assert.equal(cameraHeightAboveGround(hidden, { groundAt }), 300); + assert.deepEqual(calls, [[-104.99, 39.74]]); +}); + +test('with no ground sample the ellipsoidal height is used', () => { + const hidden = viewer({ globeShown: false, globeHeight: 0 }); + assert.equal(cameraHeightAboveGround(hidden), 1900); + assert.equal(cameraHeightAboveGround(hidden, { groundAt: () => null }), 1900); + assert.equal(cameraHeightAboveGround({}), null); +});