From a4ac0b9b0d32695ebee017164b416652eca56d43 Mon Sep 17 00:00:00 2001 From: German Khokhlov Date: Thu, 15 Oct 2026 18:48:09 +0400 Subject: [PATCH] fix(globe): harden the Navara engine after review - maps: photoreal hides the base imagery layer instead of deleting it, and a new base layer goes in before the old one goes: deleting the last raster layer stops Navara 0.2.1 loading terrain at the current view, which left terrainHeight undefined under the tiles - Google 3D tiles open Navara's attribution card so the tiles' copyright lines are on screen, as Cesium shows them; onScreen credits do the same - setCameraInputs merges into the camera options set through the engine (new setCameraOptions extension) instead of resetting them - createNavaraGlobe disposes the view and canvas if the engine throws - the pin test checks the installed Navara version and catches bracketed private access - QA: draw-match checks at the nadir clamp, after a Navara flight and after a mouse drag; terrain under photoreal set right away; the copyright lines on screen; create/destroy/create leaves no canvas, frames, animation frames, workers or console errors (54/54) --- src/globe/navara/credits.js | 10 +++--- src/globe/navara/credits.test.mjs | 16 ++++++++- src/globe/navara/index.js | 9 +++--- src/globe/navara/input.js | 14 ++++++-- src/globe/navara/input.test.mjs | 21 ++++++++++++ src/globe/navara/internals.test.mjs | 19 +++++++++-- src/globe/navara/maps.js | 50 ++++++++++++++++++++++------- src/globe/navara/maps.test.mjs | 34 +++++++++++++++++++- 8 files changed, 148 insertions(+), 25 deletions(-) diff --git a/src/globe/navara/credits.js b/src/globe/navara/credits.js index 1b68cff96..411e100e6 100644 --- a/src/globe/navara/credits.js +++ b/src/globe/navara/credits.js @@ -1,9 +1,10 @@ // Static credits on Navara's attribution UI (view.attribution, the // AttributionPlugin). Navara matches entries by content and drops duplicates, // so one HTML string is shown once and counted here, staying until its last -// holder removes it. Navara has no separate on-screen strip: `onScreen` credits -// go into the same list; logos that must stay visible (Google) are sources -// with a `logo`, added by maps.js. +// holder removes it. Navara has no separate on-screen strip: an `onScreen` +// credit opens the attribution card (collapsed by default) so the list is on +// screen; logos that must stay visible (Google) are sources with a `logo`, +// added by maps.js. /** * @param {{ attribution?: { add: Function, remove: Function } }} view @@ -19,11 +20,12 @@ export function createNavaraCredits(view) { view.attribution?.remove([{ attributionHtml: html }]); }; const credits = { - addStatic(html) { + addStatic(html, { onScreen = false } = {}) { const text = String(html); const count = counts.get(text) ?? 0; counts.set(text, count + 1); if (count === 0) view.attribution?.add([{ attributionHtml: text }]); + if (onScreen) view.attribution?.show(); let removed = false; return () => { if (removed) return; diff --git a/src/globe/navara/credits.test.mjs b/src/globe/navara/credits.test.mjs index 1d6f3e7eb..b5cdbea60 100644 --- a/src/globe/navara/credits.test.mjs +++ b/src/globe/navara/credits.test.mjs @@ -4,8 +4,12 @@ import { createNavaraCredits } from './credits.js'; function fakeAttribution() { const shown = []; - return { + const attribution = { shown, + opened: 0, + show() { + attribution.opened++; + }, add(items) { for (const item of items) if (!shown.some((entry) => entry.attributionHtml === item.attributionHtml)) @@ -20,6 +24,7 @@ function fakeAttribution() { } }, }; + return attribution; } test('addStatic shows HTML in the attribution and the remover is idempotent', () => { @@ -55,3 +60,12 @@ test('destroy removes every credit; a view without attribution UI is a no-op', ( const remove = bare.credits.addStatic('A'); remove(); }); + +test('onScreen credits open the attribution list on screen; others do not', () => { + const attribution = fakeAttribution(); + const { credits } = createNavaraCredits({ attribution }); + credits.addStatic('Quiet'); + assert.equal(attribution.opened, 0); + credits.addStatic('Loud', { onScreen: true }); + assert.equal(attribution.opened, 1); +}); diff --git a/src/globe/navara/index.js b/src/globe/navara/index.js index 24c2d738f..35d45e0fc 100644 --- a/src/globe/navara/index.js +++ b/src/globe/navara/index.js @@ -42,6 +42,7 @@ export async function createNavaraGlobe({ container, effects, pixelRatio }) { const plan = navaraScenePlan(effects); const canvas = preservedCanvas(container); let view; + let engine; try { view = new ThreeView(navaraViewOptions({ container, canvas, pixelRatio })); const plugin = new DefaultPlugin(); @@ -53,15 +54,15 @@ export async function createNavaraGlobe({ container, effects, pixelRatio }) { for (const name of plan.lights) view.addLight({ [name]: {} }); for (const name of plan.effects) view.addEffect({ [name]: {} }); } + engine = createNavaraEngine(view, { + hooks: navaraHooks(view), + container, + }); } catch (error) { view?.dispose(); canvas.remove(); throw error; } - const engine = createNavaraEngine(view, { - hooks: navaraHooks(view), - container, - }); let destroyed = false; return { view, diff --git a/src/globe/navara/input.js b/src/globe/navara/input.js index 0bb41f1c3..92b1d4e38 100644 --- a/src/globe/navara/input.js +++ b/src/globe/navara/input.js @@ -32,6 +32,7 @@ export function createNavaraInput(view, { onCameraInput = () => {} } = {}) { let attached = false; let destroyed = false; let cameraInputs = true; + let cameraOptions = {}; let downAt = null; let last = null; @@ -108,11 +109,20 @@ export function createNavaraInput(view, { onCameraInput = () => {} } = {}) { }, setCameraInputs(enabled) { cameraInputs = Boolean(enabled); - view.camera.options = { + input.setCameraOptions({ enableSpin: cameraInputs, enableZoom: cameraInputs, enableTilt: cameraInputs, - }; + }); + }, + /** + * Navara-only extension: camera controller options (zoom distances, + * speeds, ...). Navara has no getter, so the engine keeps every option + * set through it and always sends the merged set. + */ + setCameraOptions(options) { + cameraOptions = { ...cameraOptions, ...options }; + view.camera.options = { ...cameraOptions }; }, }; return { diff --git a/src/globe/navara/input.test.mjs b/src/globe/navara/input.test.mjs index d58a1599d..9defd0d88 100644 --- a/src/globe/navara/input.test.mjs +++ b/src/globe/navara/input.test.mjs @@ -141,3 +141,24 @@ test('destroy removes every DOM listener and later subscriptions are inert', () assert.equal(typeof off, 'function'); assert.equal(canvas.listenerCount, 0); }); + +test('setCameraInputs merges into the camera options set through the engine', () => { + const { input, cameraOptions } = setup(); + input.setCameraOptions({ minimumZoomDistance: 50, zoomSpeed: 2 }); + input.setCameraInputs(false); + assert.deepEqual(cameraOptions.at(-1), { + minimumZoomDistance: 50, + zoomSpeed: 2, + enableSpin: false, + enableZoom: false, + enableTilt: false, + }); + input.setCameraOptions({ zoomSpeed: 3 }); + assert.deepEqual(cameraOptions.at(-1), { + minimumZoomDistance: 50, + zoomSpeed: 3, + enableSpin: false, + enableZoom: false, + enableTilt: false, + }); +}); diff --git a/src/globe/navara/internals.test.mjs b/src/globe/navara/internals.test.mjs index 1430ad365..0d74885a4 100644 --- a/src/globe/navara/internals.test.mjs +++ b/src/globe/navara/internals.test.mjs @@ -11,8 +11,23 @@ test('Navara internals are pinned: package.json and internals.js name the same e /NAVARA_INTERNALS_VERSION = '([^']+)'/, )?.[1]; assert.equal(declared, '0.2.1'); - for (const name of ['@navaramap/three', '@navaramap/three-default-plugin']) + for (const name of ['@navaramap/three', '@navaramap/three-default-plugin']) { assert.equal(pkg.dependencies[name], declared, name); + const installed = JSON.parse( + read(`../../../node_modules/${name}/package.json`), + ); + assert.equal(installed.version, declared, `installed ${name}`); + } +}); + +// `x._member`, `x?._member` and `x['_member']`. +const PRIVATE_ACCESS = /\.\s*_[A-Za-z]|\[\s*['"`]_[A-Za-z]/; + +test('the private-member pattern catches dotted and bracketed access', () => { + for (const code of ['view._isIdle', 'view?._scenes', "view['_isIdle']", 'v[ "_x" ]']) + assert.ok(PRIVATE_ACCESS.test(code), code); + for (const code of ['view.isIdle', 'a_b.c', "map['key']"]) + assert.ok(!PRIVATE_ACCESS.test(code), code); }); test('only internals.js reaches private Navara members', () => { @@ -20,7 +35,7 @@ test('only internals.js reaches private Navara members', () => { (name) => name.endsWith('.js') && name !== 'internals.js' && - /\.\s*_[A-Za-z]/.test(read(name).replace(/\/\/.*$/gm, '')), + PRIVATE_ACCESS.test(read(name).replace(/\/\/.*$/gm, '')), ); assert.deepEqual(offenders, []); }); diff --git a/src/globe/navara/maps.js b/src/globe/navara/maps.js index ec59027f1..f1f049988 100644 --- a/src/globe/navara/maps.js +++ b/src/globe/navara/maps.js @@ -11,9 +11,11 @@ // token), or none (the ellipsoid). Navara 0.2.1 panics on a quantized-mesh // URL without the {z}/{x}/{y}.terrain template (S2), so URLs always carry it. // - photoreal: Google 3D tiles with a browser key only in this phase (the -// server proxy of S3 comes later), with Google's logo kept on screen and the -// tiles' own copyright lines (creditLayerId). Showing them removes the base -// imagery, as MapSourceController does; clearing them does not restore it. +// server proxy of S3 comes later), with Google's logo and the tiles' own +// copyright lines (creditLayerId) on screen. Showing them hides the base +// imagery, as MapSourceController removes it; clearing them does not bring +// it back until setBaseImagery. Hidden, not deleted: deleting the last +// raster layer stops Navara 0.2.1 loading terrain at the current view. const OSM_URL = 'https://tile.openstreetmap.org/{z}/{x}/{y}.png'; const OSM_CREDIT = { @@ -68,24 +70,47 @@ export function createNavaraMaps( const generations = { imagery: 0, terrain: 0, photoreal: 0 }; let destroyed = false; - function clear(name) { - const slot = slots[name]; + function retire(slot) { if (!slot) return; - slots[name] = null; slot.layer.delete(); slot.source.delete(); - if (slot.credits.length) view.attribution?.remove(slot.credits); + if (slot.credits.length && !slot.hidden) + view.attribution?.remove(slot.credits); } + function clear(name) { + const slot = slots[name]; + slots[name] = null; + retire(slot); + } + + // The new layer goes in before the old one goes: deleting the last raster + // layer stops Navara 0.2.1 loading terrain at the current view. function show(name, key, sourceDesc, layerDesc, credits = []) { - clear(name); + const previous = slots[name]; const source = view.addSource(sourceDesc); const layer = view.addLayer({ ...layerDesc, source }); const shown = credits.map((credit) => typeof credit === 'function' ? credit(layer) : credit, ); + retire(previous); if (shown.length) view.attribution?.add(shown); - slots[name] = { key, source, layer, credits: shown }; + slots[name] = { key, source, layer, layerDesc, credits: shown }; + view.forceUpdate(); + } + + // Base imagery under photoreal is hidden rather than deleted (see show). + function setImageryShown(shown) { + const slot = slots.imagery; + if (!slot || slot.hidden === !shown) return; + slot.hidden = !shown; + slot.layer.update({ + ...slot.layerDesc, + source: slot.source, + raster: { show: shown }, + }); + if (slot.credits.length) + view.attribution?.[shown ? 'add' : 'remove'](slot.credits); view.forceUpdate(); } @@ -126,7 +151,7 @@ export function createNavaraMaps( const generation = ++generations.imagery; const { url, credit } = imageryFor(source); if (destroyed || generation !== generations.imagery) return; - if (slots.imagery?.key === url) return; + if (slots.imagery?.key === url) return setImageryShown(true); show('imagery', url, { type: 'raster-tile', url }, { type: 'raster' }, [ credit, ]); @@ -157,7 +182,7 @@ export function createNavaraMaps( ); if (destroyed || generation !== generations.photoreal) return; const url = `${GOOGLE_TILES_ROOT}?key=${encodeURIComponent(key)}`; - clear('imagery'); + setImageryShown(false); if (slots.photoreal?.key === url) return; show( 'photoreal', @@ -172,6 +197,9 @@ export function createNavaraMaps( }), ], ); + // Google's terms want its copyright lines on screen, as Cesium shows + // them; Navara lists them in its attribution card, collapsed by default. + view.attribution?.show(); }, photorealActive: () => slots.photoreal != null, }; diff --git a/src/globe/navara/maps.test.mjs b/src/globe/navara/maps.test.mjs index 3edec6405..1a06cf8b0 100644 --- a/src/globe/navara/maps.test.mjs +++ b/src/globe/navara/maps.test.mjs @@ -8,6 +8,8 @@ function fakeView() { layers: [], shown: [], frames: 0, + opened: 0, + rasterCounts: [], addSource(desc) { const source = { id: `source-${view.sources.length}`, @@ -24,14 +26,21 @@ function fakeView() { const layer = { id: `layer-${view.layers.length}`, desc, + update(next) { + layer.desc = next; + }, delete() { view.layers = view.layers.filter((entry) => entry !== layer); + view.rasterCounts.push(layersOf(view, 'raster').length); }, }; view.layers.push(layer); return layer; }, attribution: { + show() { + view.opened++; + }, add(items) { view.shown.push(...items); }, @@ -159,9 +168,14 @@ test('setPhotorealTiles shows Google 3D tiles with a browser key and the Google 'https://tile.googleapis.com/v1/3dtiles/root.json?key=browser%20key', ); assert.deepEqual(tiles.desc.model, { maxSse: 16 }); - assert.equal(layersOf(view, 'raster').length, 0, 'base imagery removed'); + // Hidden, not deleted: Navara 0.2.1 stops loading terrain at the current + // view when the last raster layer is deleted. + const [base] = layersOf(view, 'raster'); + assert.equal(base.desc.raster?.show, false, 'base imagery not hidden'); + assert.deepEqual(view.rasterCounts, []); const google = view.shown.find((item) => item.creditLayerId === tiles.id); assert.ok(google?.logo, 'no Google logo'); + assert.ok(view.opened > 0, 'the copyright lines are not opened on screen'); assert.equal(maps.photorealActive(), true); await maps.setPhotorealTiles(null); assert.equal(maps.photorealActive(), false); @@ -169,6 +183,24 @@ test('setPhotorealTiles shows Google 3D tiles with a browser key and the Google assert.ok(!view.shown.some((item) => item.creditLayerId)); }); +test('base imagery hidden by photoreal comes back; a new one replaces it without a raster gap', async () => { + const view = fakeView(); + const { maps } = createNavaraMaps(view); + await maps.setBaseImagery({ kind: 'osm' }); + await maps.setPhotorealTiles({ googleApiKey: 'k' }); + const [osm] = layersOf(view, 'raster'); + await maps.setPhotorealTiles(null); + await maps.setBaseImagery({ kind: 'osm' }); + assert.deepEqual(layersOf(view, 'raster'), [osm]); + assert.notEqual(osm.desc.raster?.show, false, 'OSM still hidden'); + view.rasterCounts.length = 0; + await maps.setBaseImagery({ kind: 'esri-imagery' }); + const rasters = layersOf(view, 'raster'); + assert.equal(rasters.length, 1); + assert.match(rasters[0].desc.source.desc.url, /World_Imagery/); + assert.deepEqual(view.rasterCounts, [1], 'a raster gap while replacing'); +}); + test('photoreal without a browser key rejects and changes nothing', async () => { const view = fakeView(); const { maps } = createNavaraMaps(view);