diff --git a/explorer.qmd b/explorer.qmd index ecec4607..c0b5c9e4 100644 --- a/explorer.qmd +++ b/explorer.qmd @@ -888,20 +888,26 @@ function hasFacetFilters() { // takes effect at neighborhood zoom") explains the cluster-mode honesty // gap: H3 summary parquets only carry `dominant_source`, so material / // context / object_type filters cannot affect cluster counts. Invariant: -// visible ⇔ (any facet active) ∧ (mode === 'cluster') -// Call sites that mutate either side of the conjunction MUST call this -// to keep DOM and state in agreement: +// visible ⇔ (any facet active) ∧ (mode === 'cluster') ∧ (heatmap off) +// Call sites that mutate any conjunct MUST call this to keep DOM and +// state in agreement: // - facetFilters cell (URL deep-link restore, #234 step 1) // - handleFacetFilterChange (user toggles a facet checkbox) // - enterPointMode / exitPointMode (mode transitions) -// `viewer` resolves late at call time per ojs reactive scoping, so this -// helper is safe to define before the viewer cell runs. +// - heatmapToggle change handler (#233 phase 3) +// The heatmap conjunct: when the heatmap overlay is on, it IS the +// filter-aware density view (dots are generated from filtered data, not +// pre-baked dominant_source), so the apology is false and must be hidden +// (#233). The toggle state is read straight off the DOM rather than via +// heatmapEnabled() so this helper stays reachable from cells that run +// before the viewer cell defines that function. function syncFacetNote() { const el = document.getElementById('facetNote'); if (!el) return; const active = hasFacetFilters(); const inCluster = (typeof viewer !== 'undefined') && viewer._globeState?.mode === 'cluster'; - el.style.display = (active && inCluster) ? 'block' : 'none'; + const heatOn = document.getElementById('heatmapToggle')?.checked === true; + el.style.display = (active && inCluster && !heatOn) ? 'block' : 'none'; } function escSql(value) { @@ -2562,11 +2568,31 @@ zoomWatcher = { } } + // Single source of truth for marker-layer visibility (#233 phase 3). + // + // Cluster dots (h3Points) and individual sample points (samplePoints) + // are mutually exclusive with the heatmap overlay. When the heatmap is + // on it IS the density view, so painting the cluster dots on top of it + // produces the dots-vs-hotspots disagreement RY flagged 2026-05-27 — + // two layers telling contradictory spatial stories at once. Rule: + // heatmap on ⇒ both marker collections hidden (heatmap stands alone) + // heatmap off ⇒ show whichever collection the altitude-driven mode + // calls for (cluster→h3Points, point→samplePoints) + // Aside from the one-time initializer (samplePoints starts hidden at + // creation), this is the ONLY place that writes `.show` on the two + // collections. Call it from every site that flips the mode or the + // heatmap toggle. + function applyLayerVisibility() { + const heat = heatmapEnabled(); + const mode = getMode(); + viewer.h3Points.show = !heat && mode === 'cluster'; + viewer.samplePoints.show = !heat && mode === 'point'; + } + // --- Mode transitions --- function enterPointMode(pushHistory) { setExplorerMode('point'); - viewer.h3Points.show = false; - viewer.samplePoints.show = true; + applyLayerVisibility(); if (pushHistory !== false) history.pushState(null, '', buildHash(viewer)); // #facetNote is only meaningful in cluster mode (#234 step 1). syncFacetNote(); @@ -2576,9 +2602,8 @@ zoomWatcher = { function exitPointMode(pushHistory) { setExplorerMode('cluster'); - viewer.samplePoints.show = false; viewer.samplePoints.removeAll(); - viewer.h3Points.show = true; + applyLayerVisibility(); if (pushHistory !== false) history.pushState(null, '', buildHash(viewer)); // Returning to cluster mode: surface the honesty note if any // facet filter is active (#234 step 1). @@ -2857,6 +2882,17 @@ zoomWatcher = { let heatmapReqId = 0; let heatmapDebounce = null; let heatmapLastKey = null; + // Last SUCCESSFULLY-rendered view + filter (#233 loop fix, 2026-05-28). + // refreshHeatmap() dedupes against these with a tolerance so that + // sub-meaningful viewport jitter does NOT schedule a re-render. Set on + // render success; cleared in clearHeatmap(), the render-error path, and + // refreshHeatmap()'s no-bounds path. DELIBERATELY NOT cleared on + // moveStart (unlike heatmapLastKey): retaining the last-success snapshot + // across a moveStart/moveEnd cycle is what lets terrain/inertia jitter + // be measured against the committed overlay and ignored, instead of + // re-arming the loop. See heatmapViewMeaningfullyChanged(). + let heatmapLastBounds = null; + let heatmapLastFilterHash = null; const HEATMAP_CANVAS_SIZE = 512; function heatmapEnabled() { @@ -2949,10 +2985,63 @@ zoomWatcher = { return `${bbox}:${heatmapFilterHash()}`; } + // Normalized center + span of a viewport rectangle, antimeridian-aware + // (mirrors renderHeatmap's `eastNorm` shift). Used by the tolerance + // dedupe below. + function heatmapViewMetrics(b) { + const eastNorm = b.west > b.east ? b.east + 360 : b.east; + return { + cLat: (b.north + b.south) / 2, + cLng: (b.west + eastNorm) / 2, + spanLat: Math.max(1e-9, b.north - b.south), + spanLng: Math.max(1e-9, eastNorm - b.west), + }; + } + + // Did the view change enough to warrant a fresh heatmap render? + // + // #233 loop fix (RY 2026-05-28): the heatmap re-renders on `moveEnd`, + // keyed off `computeViewRectangle`. But that rectangle is sensitive to + // tiny camera-height changes, and with Cesium World Terrain enabled the + // camera height keeps settling for a beat after a move (terrain-collision + // adjustment as tiles stream in) — plus inertial drift after mouse-up. + // Each tiny settle fired `moveEnd` → re-render → the render's frame + // nudged terrain again → another `moveEnd`: a self-sustaining refresh + // loop with no user input. Exact-key equality (`toFixed(4)`, ~11 m) let + // that jitter through. + // + // Fix: only re-render when the view center or span moved by more than a + // small fraction of the current span. Real pans/zooms blow past this; + // terrain-settle / inertial micro-jitter stays under it and is ignored. + const HEATMAP_VIEW_TOLERANCE = 0.02; // 2% of span + // Shortest angular distance between two longitudes (degrees), result in + // [0, 180]. Without this, a wrapped bbox near the antimeridian (cLng≈180 + // via the eastNorm +360 shift) compared to a settled non-wrapped bbox + // (cLng≈-177) would read as a ~357° "move" and wrongly pass the + // meaningful-change test — preserving the loop exactly at the dateline. + // Codex review 2026-05-28. + function lngDelta(a, b) { + let d = Math.abs(a - b) % 360; + return d > 180 ? 360 - d : d; + } + function heatmapViewMeaningfullyChanged(curr, prev) { + if (!prev) return true; + const a = heatmapViewMetrics(curr); + const b = heatmapViewMetrics(prev); + const tolLat = a.spanLat * HEATMAP_VIEW_TOLERANCE; + const tolLng = a.spanLng * HEATMAP_VIEW_TOLERANCE; + return Math.abs(a.cLat - b.cLat) > tolLat + || lngDelta(a.cLng, b.cLng) > tolLng + || Math.abs(a.spanLat - b.spanLat) > tolLat + || Math.abs(a.spanLng - b.spanLng) > tolLng; + } + function clearHeatmap() { ++heatmapReqId; clearTimeout(heatmapDebounce); heatmapLastKey = null; + heatmapLastBounds = null; + heatmapLastFilterHash = null; setHeatmapStatus(''); if (heatmapImageryLayer) { viewer.imageryLayers.remove(heatmapImageryLayer, true); @@ -3064,6 +3153,11 @@ zoomWatcher = { } heatmapImageryLayer = nextLayer; heatmapLastKey = key; // success-only — see refreshHeatmap() + // Snapshot the view + filter this render committed to, for the + // tolerance dedupe in refreshHeatmap() (#233 loop fix). Same + // success-only discipline as heatmapLastKey. + heatmapLastBounds = bounds; + heatmapLastFilterHash = heatmapFilterHash(); const refreshedAt = Date.now(); // With SQL pre-aggregation, every sample in the bbox is counted // into its pixel cell — no more arbitrary LIMIT cap. `capped` is @@ -3085,9 +3179,11 @@ zoomWatcher = { if (myReq !== heatmapReqId) return; console.warn('Heatmap refresh failed:', err); setHeatmapStatus('Heatmap unavailable for this view.'); - // Clear dedupe key so a retry on the same (viewport, filter) + // Clear dedupe state so a retry on the same (viewport, filter) // actually re-attempts the render. Codex round-1 review of #240. heatmapLastKey = null; + heatmapLastBounds = null; + heatmapLastFilterHash = null; // Codex round-2 review of #240: also remove the prior imagery // layer on failure. Without this, the user sees the OLD heatmap // density alongside the "Heatmap unavailable" status — a UI lie @@ -3123,6 +3219,8 @@ zoomWatcher = { ++heatmapReqId; clearTimeout(heatmapDebounce); heatmapLastKey = null; + heatmapLastBounds = null; + heatmapLastFilterHash = null; if (heatmapImageryLayer) { viewer.imageryLayers.remove(heatmapImageryLayer, true); heatmapImageryLayer = null; @@ -3138,13 +3236,41 @@ zoomWatcher = { return; } const key = heatmapKey(bounds); - if (key === heatmapLastKey) return; - // NOTE: heatmapLastKey is set ONLY after a successful layer swap in - // renderHeatmap(), and cleared on error/cancellation. Setting it here - // would let a moveStart-cancellation between the debounce schedule - // and the actual render leave the dedupe key set without a render - // having happened — the next moveEnd then early-returns and the - // overlay is wedged. Codex round-1 review of PR #240. + // Tolerance dedupe (#233 loop fix, RY 2026-05-28). Re-render only on + // a filter change OR a meaningful view change. Sub-meaningful + // viewport jitter (terrain-collision height settling, inertial + // micro-drift) keeps the same filter and stays within the view + // tolerance, so it is ignored here instead of driving a render that + // would nudge terrain and sustain the loop. The previous exact-key + // equality (`toFixed(4)`, ~11 m) let that jitter through. + const filterHash = heatmapFilterHash(); + if (filterHash === heatmapLastFilterHash + && !heatmapViewMeaningfullyChanged(bounds, heatmapLastBounds)) { + // View only jittered (terrain settle / inertia); the existing + // overlay is still correct. A moveStart may have flipped the + // status to "waiting for camera" — restore the truthful + // "rendered" line so it doesn't get stuck mid-loop. Gate on + // heatmapLastBounds (set only on a successful render) rather than + // a truthy count, so a valid ZERO-sample overlay also restores + // its status instead of sticking at "waiting" (Codex review). + if (heatmapLastBounds) { + const n = Number(viewer._heatmapOverlay?.lastPointCount || 0); + setHeatmapStatus(`Heatmap rendered from ${n.toLocaleString()} samples.`); + } + // Test-only signal: count tolerance skips so the regression test + // can PROVE the skip branch ran (a real moveEnd reached here), + // rather than relying on the vacuous "lastRefreshAt unchanged" + // assertion (Codex review 2026-05-28). + viewer._heatmapSkips = (viewer._heatmapSkips || 0) + 1; + return; + } + // NOTE: heatmapLast{Key,Bounds,FilterHash} are set ONLY after a + // successful layer swap in renderHeatmap(), and cleared on + // error/cancellation. Setting them here would let a moveStart- + // cancellation between the debounce schedule and the actual render + // leave the dedupe state set without a render having happened — the + // next moveEnd then early-returns and the overlay is wedged. Codex + // round-1 review of PR #240. clearTimeout(heatmapDebounce); const myReq = ++heatmapReqId; heatmapDebounce = setTimeout(() => { @@ -3158,8 +3284,16 @@ zoomWatcher = { } else { clearHeatmap(); } + // #233 phase 3: heatmap is mutually exclusive with the marker + // layers, so flip dot visibility and re-evaluate the facet apology + // note (which is meaningless once the heatmap shows filtered + // density directly). Both helpers read live state, so order vs the + // refresh/clear above does not matter. + applyLayerVisibility(); + syncFacetNote(); }); viewer._heatmapOverlay = { enabled: false, layer: null, lastRefreshAt: 0, lastPointCount: 0, lastKey: null }; + viewer._heatmapSkips = 0; // tolerance-dedupe skip counter (test signal) // --- Busy-flag depth counter (#173 review round 2) --- // diff --git a/tests/playwright/heatmap-overlay.spec.js b/tests/playwright/heatmap-overlay.spec.js index 70035256..d08c1047 100644 --- a/tests/playwright/heatmap-overlay.spec.js +++ b/tests/playwright/heatmap-overlay.spec.js @@ -55,11 +55,113 @@ test.describe('Heatmap overlay (#233 phase 1)', () => { }).toBeTruthy(); } + // Reads the live `.show` flags off the two marker collections so a test + // can assert mutual exclusion with the heatmap overlay (#233 phase 3). + async function markerVisibility(page) { + return await page.evaluate(() => { + return window._ojs.ojsConnector.mainModule.value('viewer').then((v) => ({ + clusterShown: v?.h3Points?.show === true, + pointShown: v?.samplePoints?.show === true, + mode: v?._globeState?.mode || null, + })); + }); + } + test('heatmap toggle exists', async ({ page }) => { await openExplorer(page); await expect(page.locator('#heatmapToggle')).toBeVisible(); }); + test('phase 3: enabling heatmap hides cluster dots; disabling restores them', async ({ page }) => { + // #233 phase 3: heatmap is mutually exclusive with the marker layers. + // The dots-vs-hotspots disagreement RY flagged 2026-05-27 came from + // both layers painting at once. At the Cyprus hash (alt=500km, well + // above ENTER_POINT_ALT=120km) the explorer is in cluster mode, so + // h3Points are the visible markers. + await openExplorer(page); + + const before = await markerVisibility(page); + expect(before.mode).toBe('cluster'); + expect(before.clusterShown).toBe(true); + + await enableHeatmap(page); + await expect.poll(async () => (await markerVisibility(page)).clusterShown, { + timeout: 30000, + intervals: [100, 250, 500], + }).toBe(false); + // Sample points stay hidden too (they are point-mode only). + expect((await markerVisibility(page)).pointShown).toBe(false); + + await page.locator('#heatmapToggle').uncheck(); + await expect.poll(async () => (await markerVisibility(page)).clusterShown, { + timeout: 30000, + intervals: [100, 250, 500], + }).toBe(true); + }); + + test('phase 3: in point mode, heatmap hides sample points; off restores them', async ({ page }) => { + // Codex review of #233 phase 3: the cluster test above covers the + // h3Points half; this covers the samplePoints half. Boot deep into a + // dense area (alt=8km < ENTER_POINT_ALT=120km) so boot enters point + // mode. Cold-cache point entry fetches the res8 + samples parquet, so + // allow generous time — mirrors the explorer-helper point-mode specs. + const POINT_HASH = '#v=1&lat=35&lng=33&alt=8000'; + await page.goto(explorerUrl(POINT_HASH), { + waitUntil: 'domcontentloaded', + timeout: 60000, + }); + await page.waitForSelector('#cesiumContainer', { timeout: 30000 }); + await page.waitForFunction(() => !!window._ojs?.ojsConnector?.mainModule, null, { timeout: 60000 }); + // Wait for boot to settle into point mode. + await expect.poll(async () => (await markerVisibility(page)).mode, { + timeout: 150000, + intervals: [500, 1000, 2000], + }).toBe('point'); + // Point mode, heatmap off: sample points shown, cluster dots hidden. + await expect.poll(async () => (await markerVisibility(page)).pointShown, { + timeout: 30000, + intervals: [250, 500, 1000], + }).toBe(true); + expect((await markerVisibility(page)).clusterShown).toBe(false); + + // Heatmap on: both marker collections hidden. + await enableHeatmap(page); + await expect.poll(async () => (await markerVisibility(page)).pointShown, { + timeout: 30000, + intervals: [100, 250, 500], + }).toBe(false); + expect((await markerVisibility(page)).clusterShown).toBe(false); + + // Heatmap off: sample points restored (still point mode). + await page.locator('#heatmapToggle').uncheck(); + await expect.poll(async () => (await markerVisibility(page)).pointShown, { + timeout: 30000, + intervals: [100, 250, 500], + }).toBe(true); + }); + + test('phase 3: heatmap on hides the #facetNote apology', async ({ page }) => { + // #233: with the heatmap on, the facet note ("filters apply at sample + // zoom level") is a lie — the heatmap shows filtered density directly. + // It must be hidden whenever a facet filter is active AND heatmap is on. + await openExplorer(page); + await waitForFacetCheckboxes(page); + + // Activate a material facet so the note would otherwise show in cluster + // mode (visible ⇔ active ∧ cluster ∧ heatmap-off). + await page.evaluate(() => { + const cb = document.querySelector('#materialFilterBody input[type="checkbox"]'); + if (cb) { cb.checked = true; cb.dispatchEvent(new Event('change', { bubbles: true })); } + }); + await expect(page.locator('#facetNote')).toBeVisible(); + + await enableHeatmap(page); + await expect(page.locator('#facetNote')).toBeHidden(); + + await page.locator('#heatmapToggle').uncheck(); + await expect(page.locator('#facetNote')).toBeVisible(); + }); + test('toggle on renders a visible heatmap layer', async ({ page }) => { await openExplorer(page); await enableHeatmap(page); @@ -134,6 +236,13 @@ test.describe('Heatmap overlay (#233 phase 1)', () => { }).toBeTruthy(); // Also assert the toggle DOM reflects the hydrated state. await expect(page.locator('#heatmapToggle')).toBeChecked(); + // #233 phase 3: a hydrated heatmap=1 boot must also hide the markers — + // the toggle's change handler (dispatched during boot) routes through + // applyLayerVisibility(). Cyprus is cluster mode, so h3Points hidden. + await expect.poll(async () => (await markerVisibility(page)).clusterShown, { + timeout: 30000, + intervals: [250, 500, 1000], + }).toBe(false); }); test('world view counts every sample (no LIMIT cap — phase 1.5)', async ({ page }) => { @@ -167,4 +276,68 @@ test.describe('Heatmap overlay (#233 phase 1)', () => { }); expect(cappedRaw).toBe(false); }); + + // Helper: shift the camera by a longitude delta (degrees), keeping lat/ + // height, and return the resulting padded-viewport span so the test can + // reason about the tolerance threshold. + async function nudgeLongitude(page, deltaDeg) { + await page.evaluate(async (d) => { + const Cesium = window.Cesium; + const v = await window._ojs.ojsConnector.mainModule.value('viewer'); + const cc = v.camera.positionCartographic; + v.camera.setView({ + destination: Cesium.Cartesian3.fromDegrees( + Cesium.Math.toDegrees(cc.longitude) + d, + Cesium.Math.toDegrees(cc.latitude), + cc.height), + }); + }, deltaDeg); + } + + test('loop fix: sub-threshold viewport jitter does NOT re-render; a real move does', async ({ page }) => { + // #233 loop fix (RY 2026-05-28): with Cesium World Terrain on, the + // camera height keeps settling for a beat after a move, firing moveEnd + // with a slightly-shifted computeViewRectangle. The old exact-key + // dedupe (toFixed(4)) let that jitter through → re-render → terrain + // nudge → moveEnd → … a self-sustaining refresh loop. The tolerance + // dedupe ignores view changes under 2% of span. This test simulates + // the two ends of that spectrum: a tiny nudge (jitter-class, must be + // ignored) and a large move (real, must re-render). + await openExplorer(page); + await enableHeatmap(page); + const first = await heatmapState(page); + expect(first.lastRefreshAt).toBeGreaterThan(0); + + const skipsAndLng = async () => page.evaluate(async () => { + const Cesium = window.Cesium; + const v = await window._ojs.ojsConnector.mainModule.value('viewer'); + return { skips: v._heatmapSkips || 0, lng: Cesium.Math.toDegrees(v.camera.positionCartographic.longitude) }; + }); + const before = await skipsAndLng(); + + // Tiny nudge (~0.01° on a multi-degree span ≪ 2% tolerance). Fires + // moveEnd but must NOT bump lastRefreshAt. Crucially we ALSO assert the + // skip counter incremented — that PROVES a real moveEnd reached the + // tolerance branch and was skipped, rather than the negative assertion + // passing vacuously because no moveEnd fired (Codex review 2026-05-28). + await nudgeLongitude(page, 0.01); + await expect.poll(async () => (await skipsAndLng()).skips, { + timeout: 10000, + intervals: [100, 250, 500], + }).toBeGreaterThan(before.skips); + const afterJitter = await skipsAndLng(); + // The camera really moved (so computeViewRectangle differed)… + expect(Math.abs(afterJitter.lng - before.lng)).toBeGreaterThan(0.005); + // …yet no re-render happened, and the overlay is still present. + const jitterState = await heatmapState(page); + expect(jitterState.lastRefreshAt).toBe(first.lastRefreshAt); + expect(jitterState.hasLayer).toBe(true); + + // Real move (~6°, far beyond tolerance) must trigger a fresh render. + await nudgeLongitude(page, 6); + await expect.poll(async () => (await heatmapState(page)).lastRefreshAt, { + timeout: 90000, + intervals: [250, 500, 1000], + }).toBeGreaterThan(first.lastRefreshAt); + }); });