Fix annotation height culling review findings (#1277)

- Restrict the conservative frustum test to height sampling only: an
  endpoint outside it still projects at its cached or zero height, so an
  arrow with one off-frustum endpoint keeps its visible connector and label.
- Test a settled height as the actual point; reject an unknown height
  before sampling only when both ends of the accepted -430..9000 m range
  fail the same constraint (behind camera, or below horizon), which the
  convexity of each rejection region makes a proof for the whole range.
- Queue keys refused by an exhausted budget and reserve the next pass's
  budget for the oldest of them, so anchors beyond the first 32 are served
  under sparse passes while keeping 32 samples per pass and 1 s retries.
- Add regressions with a real frustum, a low-angle authored camera, and
  33 persistent points under sparse passes; consolidate CURRENT-STATE.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

docs/CURRENT-STATE.md

@@ -9,7 +9,15 @@ broader suffix variants. Follow-ups retain boundary provenance, and cancelled
 lookups preserve the previous analyst cohort and boundary source. Navigation
 search keeps a successful geocode, with no search or view-based region recovery.
 
-Screen annotations reject hidden records and anchors wholly behind the camera or globe horizon before surface sampling. Unresolved visible heights share a 32-sample projection-pass budget, retry at most once per second per anchor on rendered frames, and retain no extra timer or listener. Unsettled points continue retrying as late terrain arrives; behind-camera, horizon and conservative frustum culls run before sampling. Validated heights remain settled in the bounded cache.
+Screen annotations skip hidden records and, before surface sampling, reject an
+unsettled anchor only when its whole accepted height range (-430 to 9000 m) is
+behind the camera or below the globe horizon; a settled height is tested as the
+actual point. Anchors outside the conservative frustum still project at their
+cached or zero height, so a partly visible arrow keeps its connector, but do
+not sample. Unresolved visible heights share a 32-sample projection-pass budget,
+served oldest-refused first, retry at most once per second per anchor on
+rendered frames, and retain no extra timer or listener. Validated heights remain
+settled in the bounded cache.
 
 Terrain height requests accept at most 64 points, matching the browser service's
 existing batch size and one bounded upstream chunk. Larger requests return HTTP

src/annotations/screenAnnotationRenderer.js

@@ -83,9 +83,20 @@ export function createScreenAnnotationRenderer(
   const heightCache = new Map();
   const HEIGHT_CACHE_SOFT = 600; // target size; over this we evict keys not used this frame
   const HEIGHT_CACHE_HARD = 8000; // absolute ceiling (one big ring + others) — never exceeded
+  // Accepted surface-height range; samples outside it are treated as misses.
+  const MIN_SURFACE_HEIGHT = -430;
+  const MAX_SURFACE_HEIGHT = 9000;
+  const HEIGHT_SAMPLE_BUDGET = 32; // clampToHeight calls per projection pass
+  const HEIGHT_RETRY_MS = 1000; // minimum interval between misses per anchor
   let cullingVolume = null;
   const heightBounds = new Cesium.BoundingSphere(new Cesium.Cartesian3(), 9500);
-  let heightBudget = 32;
+  // Fair sampling: keys refused by an exhausted budget queue in arrival order,
+  // and each pass reserves budget for the oldest queued keys first, so a fixed
+  // iteration order cannot starve anchors beyond the first budget's worth.
+  let heightQueue = new Set(); // keys refused in earlier passes, oldest first
+  let heightReserved = new Set(); // queued keys this pass serves first
+  let heightRefused = new Set(); // keys refused during this pass
+  let heightBudget = HEIGHT_SAMPLE_BUDGET; // unreserved slots left this pass
   let projGen = 0; // bumped each projection frame; entries used this frame are "hot"
   function trimHeightCache() {
     if (heightCache.size <= HEIGHT_CACHE_SOFT) return;
@@ -113,10 +124,20 @@ export function createScreenAnnotationRenderer(
     return PALETTE[anno.color] || PALETTE.primary;
   }
 
+  function clearHeights() {
+    heightCache.clear();
+    heightQueue.clear();
+  }
+
+  function heightKey(lon, lat) {
+    return `${lon.toFixed(5)},${lat.toFixed(5)}`;
+  }
+
   // Best-effort surface height under a coordinate (clamps onto the 3D tiles).
   // Returns 0 until the tile loads, then caches the validated height.
-  function groundHeight(lon, lat) {
-    const key = `${lon.toFixed(5)},${lat.toFixed(5)}`;
+  // canSample=false returns the cached height (or 0) without surface sampling.
+  function groundHeight(lon, lat, canSample = true) {
+    const key = heightKey(lon, lat);
     const cached = heightCache.get(key);
     if (cached && cached.settled) {
       // Mark hot for this frame + bump to most-recently-used so heights for live
@@ -127,12 +148,19 @@ export function createScreenAnnotationRenderer(
       return cached.h;
     }
     const now = Date.now();
-    if (cached && now < cached.nextAttempt) {
+    if (cached && (!canSample || now < cached.nextAttempt)) {
       cached.gen = projGen;
       return cached.h;
     }
-    if (heightBudget <= 0) return cached?.h || 0;
+    if (!canSample) return 0;
+    if (heightReserved.has(key)) {
+      heightReserved.delete(key);
+    } else if (heightBudget > 0) {
       heightBudget--;
+    } else {
+      heightRefused.add(key);
+      return cached?.h || 0;
+    }
     try {
       if (
         scene.clampToHeightSupported &&
@@ -143,7 +171,11 @@ export function createScreenAnnotationRenderer(
         );
         if (c) {
           const h = Cesium.Cartographic.fromCartesian(c).height;
-          if (Number.isFinite(h) && h > -430 && h < 9000) {
+          if (
+            Number.isFinite(h) &&
+            h > MIN_SURFACE_HEIGHT &&
+            h < MAX_SURFACE_HEIGHT
+          ) {
             heightCache.set(key, { h, settled: true, gen: projGen });
             trimHeightCache();
             return h;
@@ -158,7 +190,7 @@ export function createScreenAnnotationRenderer(
       h: fallback,
       settled: false,
       gen: projGen,
-      nextAttempt: now + 1000,
+      nextAttempt: now + HEIGHT_RETRY_MS,
     });
     trimHeightCache();
     return fallback;
@@ -325,7 +357,7 @@ export function createScreenAnnotationRenderer(
       } catch {
         /* never inserted */
       }
-      if (records.size === 0) heightCache.clear();
+      if (records.size === 0) clearHeights();
       throw error;
     }
   }
@@ -377,29 +409,30 @@ export function createScreenAnnotationRenderer(
     // Anchor at the real surface height (cached + validated), so marks sit on
     // the ground/building instead of at sea level. The settle-once cache keeps
     // it stable (no per-frame jitter that earlier broke nearby projections).
-    // Cheap conservative endpoint tests precede GPU-backed surface sampling.
-    // Testing the accepted height range avoids culling an elevated roof solely
-    // because its sea-level anchor lies just beyond the horizon.
-    const visibleAt = (height) => {
-      const point = Cesium.Cartesian3.fromDegrees(lon, lat, height);
-      Cesium.Cartesian3.subtract(point, scene.camera.positionWC, scratchDir);
-      return (
-        Cesium.Cartesian3.dot(scratchDir, scene.camera.directionWC) > 0 &&
-        occluder.isPointVisible(point)
-      );
-    };
-    Cesium.Cartesian3.fromDegrees(lon, lat, 0, undefined, heightBounds.center);
-    if (
-      cullingVolume?.computeVisibility(heightBounds) ===
-      Cesium.Intersect.OUTSIDE
-    )
-      return null;
-    if (!visibleAt(0) && !visibleAt(9000)) return null;
-    const world = Cesium.Cartesian3.fromDegrees(
+    // A settled height is tested as the actual point below. An unknown height
+    // is rejected before GPU-backed surface sampling only when the whole
+    // accepted height range is proven hidden by one constraint.
+    let height;
+    if (heightCache.get(heightKey(lon, lat))?.settled) {
+      height = groundHeight(lon, lat);
+    } else {
+      if (heightRangeHidden(lon, lat)) return null;
+      // The frustum restricts sampling only; an endpoint outside it still
+      // projects (at its cached or zero height) so a partly visible arrow,
+      // route or ring keeps its on-screen connector and label.
+      Cesium.Cartesian3.fromDegrees(
         lon,
         lat,
-      groundHeight(lon, lat),
+        0,
+        undefined,
+        heightBounds.center,
       );
+      const inFrustum =
+        cullingVolume?.computeVisibility(heightBounds) !==
+        Cesium.Intersect.OUTSIDE;
+      height = groundHeight(lon, lat, inFrustum);
+    }
+    const world = Cesium.Cartesian3.fromDegrees(lon, lat, height);
     // Reject points behind the camera (they produce wrapped/extreme coords).
     Cesium.Cartesian3.subtract(world, scene.camera.positionWC, scratchDir);
     if (Cesium.Cartesian3.dot(scratchDir, scene.camera.directionWC) <= 0)
@@ -419,6 +452,21 @@ export function createScreenAnnotationRenderer(
     return { x: win.x, y: win.y };
   }
 
+  // Points along one surface normal are linear in height, and each rejection
+  // region (behind the camera plane; the occluder's cone beyond the horizon
+  // plane) is convex. So when both height-range endpoints fail the SAME test,
+  // every intermediate height fails it too. Mixed failures prove nothing.
+  function heightRangeHidden(lon, lat) {
+    const low = Cesium.Cartesian3.fromDegrees(lon, lat, MIN_SURFACE_HEIGHT);
+    const high = Cesium.Cartesian3.fromDegrees(lon, lat, MAX_SURFACE_HEIGHT);
+    const behind = (point) => {
+      Cesium.Cartesian3.subtract(point, scene.camera.positionWC, scratchDir);
+      return Cesium.Cartesian3.dot(scratchDir, scene.camera.directionWC) <= 0;
+    };
+    if (behind(low) && behind(high)) return true;
+    return !occluder.isPointVisible(low) && !occluder.isPointVisible(high);
+  }
+
   // Tracked-entity z-order: the tracked aircraft is a Cesium billboard in the
   // CANVAS, which sits BELOW this SVG overlay (z-90, HTML over canvas). So a screen mark
   // could hide it. After layout, any mark whose ACTUAL projected bounding box (rings,
@@ -500,7 +548,14 @@ export function createScreenAnnotationRenderer(
 
   function projectAll() {
     if (!records.size) return;
-    heightBudget = 32;
+    // Serve the oldest refused keys first; the rest of the budget is open.
+    heightReserved = new Set();
+    for (const key of heightQueue) {
+      if (heightReserved.size >= HEIGHT_SAMPLE_BUDGET) break;
+      heightReserved.add(key);
+    }
+    heightBudget = HEIGHT_SAMPLE_BUDGET - heightReserved.size;
+    heightRefused = new Set();
     cullingVolume =
       scene.camera.frustum?.computeCullingVolume?.(
         scene.camera.positionWC,
@@ -653,6 +708,13 @@ export function createScreenAnnotationRenderer(
         }
       }
     }
+    // Still-refused queued keys keep their place; new refusals join the back.
+    // Keys no longer requesting a sample (settled, culled, removed) drop out.
+    const nextQueue = new Set();
+    for (const key of heightQueue)
+      if (heightRefused.has(key)) nextQueue.add(key);
+    for (const key of heightRefused) nextQueue.add(key);
+    heightQueue = nextQueue;
 
     // Keep clustered callouts readable.
     const placed = [];
@@ -733,7 +795,7 @@ export function createScreenAnnotationRenderer(
     }, 360);
     records.delete(anno.id);
     // Board emptied → drop accumulated height samples (natural reset point).
-    if (records.size === 0) heightCache.clear();
+    if (records.size === 0) clearHeights();
   }
 
   function sync() {
@@ -753,7 +815,7 @@ export function createScreenAnnotationRenderer(
       /* gone */
     }
     records.clear();
-    heightCache.clear();
+    clearHeights();
   }
 
   return { add, update, remove, sync, destroy };

src/annotations/screenAnnotationRenderer.test.mjs

@@ -424,3 +424,189 @@ test('hidden and back-facing annotations do not sample; visible unresolved heigh
   assert.equal(camera.moveEnd.numberOfListeners, 0);
   assert.equal(postRender.numberOfListeners, 0);
 });
+
+function sampledLon(world) {
+  const carto = Cesium.Cartographic.fromCartesian(world);
+  return Number(Cesium.Math.toDegrees(carto.longitude).toFixed(5));
+}
+
+function installDom(t) {
+  const previous = globalThis.document;
+  const previousRaf = globalThis.requestAnimationFrame;
+  globalThis.document = fakeDocument();
+  globalThis.requestAnimationFrame = (fn) => {
+    fn();
+    return 1;
+  };
+  t.after(() => {
+    globalThis.document = previous;
+    globalThis.requestAnimationFrame = previousRaf;
+  });
+}
+
+function squareFrustum() {
+  const frustum = new Cesium.PerspectiveFrustum();
+  frustum.fov = Cesium.Math.toRadians(60);
+  frustum.aspectRatio = 1;
+  frustum.near = 1;
+  frustum.far = 1e8;
+  return frustum;
+}
+
+test('an arrow with one endpoint outside a real frustum keeps its visible connector', (t) => {
+  installDom(t);
+  t.mock.method(Cesium.SceneTransforms, 'worldToWindowCoordinates', (_scene, world) =>
+    sampledLon(world) > 0.1 ? { x: 1100, y: 400 } : { x: 400, y: 400 },
+  );
+  // 5 km straight down over (0, 0): east is +Y, north is +Z in ECEF here.
+  const camera = {
+    positionWC: Cesium.Cartesian3.fromDegrees(0, 0, 5000),
+    directionWC: new Cesium.Cartesian3(-1, 0, 0),
+    upWC: new Cesium.Cartesian3(0, 0, 1),
+    positionCartographic: { height: 5000 },
+    frustum: squareFrustum(),
+  };
+  const sampled = [];
+  const postRender = new Cesium.Event();
+  const scene = {
+    camera,
+    canvas: { width: 800, height: 800 },
+    postRender,
+    clampToHeightSupported: true,
+    clampToHeight(world) {
+      sampled.push(sampledLon(world));
+      return world;
+    },
+  };
+  // The 0.2-degree endpoint (~22 km east) is well beyond the side plane plus
+  // the 9.5 km height-bound radius, yet in front of the camera and the horizon.
+  const far = Cesium.Cartesian3.fromDegrees(0.2, 0, 0);
+  const volume = camera.frustum.computeCullingVolume(
+    camera.positionWC,
+    camera.directionWC,
+    camera.upWC,
+  );
+  assert.equal(
+    volume.computeVisibility(new Cesium.BoundingSphere(far, 9500)),
+    Cesium.Intersect.OUTSIDE,
+  );
+  const renderer = createScreenAnnotationRenderer({ scene, camera });
+  renderer.add({
+    id: 'partial-arrow',
+    type: 'arrow',
+    label: 'Ridge',
+    anchor: { lon: 0, lat: 0 },
+    to: { lon: 0.2, lat: 0 },
+  });
+  postRender.raiseEvent();
+  const { group } = findAnnotationGroup(globalThis.document);
+  assert.equal(group.style.display, '', 'the arrow stays drawn');
+  const path = group.children.find((child) => child.classList.contains('gev-anno-arrow'));
+  assert.match(path.getAttribute('d'), /^M 400\.0 400\.0 Q .* 1100\.0 400\.0$/);
+  assert.deepEqual([...new Set(sampled)], [0], 'only the in-frustum endpoint samples');
+  renderer.destroy();
+});
+
+test('a low-angle cached roof stays visible when range endpoints fail different tests', (t) => {
+  installDom(t);
+  t.mock.method(Cesium.SceneTransforms, 'worldToWindowCoordinates', () => ({ x: 400, y: 380 }));
+  // Camera 1 m above the ellipsoid, heading east, pitched 25 degrees down.
+  const pitch = Cesium.Math.toRadians(25);
+  const camera = {
+    positionWC: Cesium.Cartesian3.fromDegrees(0, 0, 1),
+    directionWC: new Cesium.Cartesian3(-Math.sin(pitch), Math.cos(pitch), 0),
+    upWC: new Cesium.Cartesian3(Math.cos(pitch), Math.sin(pitch), 0),
+    positionCartographic: { height: 1 },
+    frustum: squareFrustum(),
+  };
+  const roofLon = 4000 / 111319.49;
+  let samples = 0;
+  const postRender = new Cesium.Event();
+  const scene = {
+    camera,
+    canvas: { width: 800, height: 800 },
+    postRender,
+    clampToHeightSupported: true,
+    clampToHeight() {
+      samples++;
+      return Cesium.Cartesian3.fromDegrees(roofLon, 0, 20);
+    },
+  };
+  // Sanity: height 0 is beyond the horizon and 9000 m is behind the camera,
+  // while the 20 m roof itself is in front and above the horizon.
+  const occluder = new Cesium.EllipsoidalOccluder(Cesium.Ellipsoid.WGS84, camera.positionWC);
+  const ahead = (h) => {
+    const p = Cesium.Cartesian3.fromDegrees(roofLon, 0, h);
+    const v = Cesium.Cartesian3.subtract(p, camera.positionWC, new Cesium.Cartesian3());
+    return Cesium.Cartesian3.dot(v, camera.directionWC) > 0;
+  };
+  assert.equal(occluder.isPointVisible(Cesium.Cartesian3.fromDegrees(roofLon, 0, 0)), false);
+  assert.equal(ahead(9000), false);
+  assert.equal(ahead(20), true);
+  assert.equal(occluder.isPointVisible(Cesium.Cartesian3.fromDegrees(roofLon, 0, 20)), true);
+
+  const renderer = createScreenAnnotationRenderer({ scene, camera });
+  renderer.add({ id: 'roof', type: 'label', label: 'Roof', anchor: { lon: roofLon, lat: 0 } });
+  const { group } = findAnnotationGroup(globalThis.document);
+  assert.equal(samples, 1, 'the unknown height is sampled, not culled');
+  assert.equal(group.style.display, '');
+  for (let i = 0; i < 5; i++) postRender.raiseEvent();
+  assert.equal(samples, 1, 'the settled height is reused');
+  assert.equal(group.style.display, '', 'the cached roof point is tested and shown');
+  renderer.destroy();
+});
+
+test('height retries beyond the first budget are served under sparse passes', (t) => {
+  installDom(t);
+  t.mock.timers.enable({ apis: ['setTimeout', 'Date'], now: 1000 });
+  t.mock.method(Cesium.SceneTransforms, 'worldToWindowCoordinates', () => ({ x: 100, y: 100 }));
+  const camera = {
+    positionWC: Cesium.Cartesian3.fromDegrees(0, 0, 100000),
+    directionWC: new Cesium.Cartesian3(-1, 0, 0),
+    positionCartographic: { height: 100000 },
+  };
+  const lons = Array.from({ length: 33 }, (_, i) => Number(((i + 1) / 1000).toFixed(3)));
+  const last = lons.at(-1);
+  let tilesReady = false;
+  let pass = [];
+  const postRender = new Cesium.Event();
+  const scene = {
+    camera,
+    canvas: { width: 800, height: 600 },
+    postRender,
+    clampToHeightSupported: true,
+    clampToHeight(world) {
+      const lon = sampledLon(world);
+      pass.push(lon);
+      return tilesReady && lon === last ? Cesium.Cartesian3.fromDegrees(lon, 0, 12) : undefined;
+    },
+  };
+  const renderer = createScreenAnnotationRenderer({ scene, camera });
+  for (const lon of lons) {
+    renderer.add({ id: `p${lon}`, type: 'label', label: 'P', anchor: { lon, lat: 0 } });
+  }
+  assert.ok(pass.includes(last), 'every point gets its initial miss');
+  tilesReady = true; // tiles under the final point arrive after its miss
+  const lastSamples = [];
+  for (let i = 0; i < 4; i++) {
+    t.mock.timers.tick(1000);
+    pass = [];
+    postRender.raiseEvent();
+    assert.ok(pass.length <= 32, `pass ${i} keeps the 32-sample budget`);
+    lastSamples.push(pass.filter((lon) => lon === last).length);
+  }
+  assert.ok(lastSamples.slice(0, 2).includes(1), 'the 33rd point is served within two passes');
+  assert.equal(lastSamples.at(-1), 0, 'and then stays settled');
+  // Points refused in one pass are served first in the next: every point is
+  // retried within any two consecutive sparse passes.
+  pass = [];
+  t.mock.timers.tick(1000);
+  postRender.raiseEvent();
+  const firstPass = pass;
+  pass = [];
+  t.mock.timers.tick(1000);
+  postRender.raiseEvent();
+  const served = new Set([...firstPass, ...pass]);
+  for (const lon of lons.slice(0, -1)) assert.ok(served.has(lon), `${lon} retried`);
+  renderer.destroy();
+});