(test) Coverage for core changes

toddrun · toddrun · commit a417eb1fce96 · 2026-04-24T10:39:49.000-07:00
diff --git a/modules/arcgis/src/deck-renderer.ts b/modules/arcgis/src/deck-renderer.ts
@@ -103,16 +103,18 @@ export default function createDeckRenderer(DeckProps, RenderNode) {
     cancelInitialization: (() => void) | null = null;
     renderNode: any = null;
 
-    constructor(view: SceneView, props: DeckProps) {
-      this.view = view;
-      this.deck = new DeckProps(props);
+    constructor(viewOrProps: SceneView | ({view: SceneView} & DeckProps), props?: DeckProps) {
+      if (viewOrProps && typeof viewOrProps === 'object' && 'view' in viewOrProps) {
+        const {view, ...deckProps} = viewOrProps;
+        this.view = view;
+        this.deck = new DeckProps(deckProps);
+      } else {
+        this.view = viewOrProps;
+        this.deck = new DeckProps(props);
+      }
 
-      // Guard against concurrent initialization attempts (e.g. if render fires
-      // multiple times before the async init completes).
       let isInitializing = false;
 
-      // Alias outer `this` for use inside the RenderNode.createSubclass()
-      // callbacks below, where `this` refers to the RenderNode instance.
       // eslint-disable-next-line @typescript-eslint/no-this-alias
       const self = this;
       const DeckRenderNode = RenderNode.createSubclass({
@@ -122,15 +124,11 @@ export default function createDeckRenderer(DeckProps, RenderNode) {
         initialize() {},
 
         render(inputs: Array<{name: string}>) {
-          // `this` here is the RenderNode instance, typed loosely because
-          // ArcGIS's RenderNode.createSubclass() does not surface types.
           // biome-ignore lint/complexity/noThisInStatic: RenderNode render is an instance method
           // eslint-disable-next-line @typescript-eslint/no-this-alias, consistent-this
           const renderNode: any = this;
           const passthrough = inputs.find(i => i.name === 'composite-color');
 
-          // Lazy init: the first render() call is the earliest point at which
-          // `gl` is guaranteed to be available on the RenderNode.
           if (!self.resources && !isInitializing) {
             const gl = renderNode.gl;
             if (gl) {
@@ -164,59 +162,31 @@ export default function createDeckRenderer(DeckProps, RenderNode) {
             return passthrough;
           }
 
-          // bindRenderTarget() must be called BEFORE render() so that the
-          // FRAMEBUFFER_BINDING captured inside commons.render() points at the
-          // OUTPUT framebuffer rather than the composite-color INPUT.
           const output = renderNode.bindRenderTarget();
-
-          // Use gl drawing buffer rather than view.size, which can include
-          // non-GL chrome pixels.
           const gl = renderNode.gl as WebGL2RenderingContext;
 
           const dpr = window.devicePixelRatio || 1;
           const width = gl.drawingBufferWidth / dpr;
           const height = gl.drawingBufferHeight / dpr;
 
-          // deck.gl's 512-px tile convention: at zoom z / latitude φ,
-          // ground-meters-per-pixel = 78271.5 * cos(φ) / 2^z. ArcGIS's
-          // `view.scale` is related to ArcGIS's `view.resolution` by
-          // `resolution = scale / 3779.527559` (96 DPI), where resolution
-          // is *projected* Web-Mercator meters-per-pixel. Ground mpp at
-          // latitude is `resolution * cos(φ)` — the cos(φ) stretch is
-          // baked into both sides, so it cancels when equating deck ground
-          // mpp to ArcGIS ground mpp. The zoom formula is therefore:
-          //   zoom = log2(591657550.5 / view.scale) - 1
-          // (the `-1` converts the 256-px constant to deck's 512-px
-          // convention.) Earlier revisions included a `* cos(lat)` factor
-          // here; that was wrong — it was inadvertently matching
-          // *projected* Mercator mpp instead of ground mpp, and caused
-          // the deck layer to render ~26% smaller than the basemap at SF
-          // latitudes.
-          // deck.gl's `altitude` is the camera distance above the focal
-          // plane, measured in viewport-height units. In deck.gl,
-          // `altitude` couples camera distance with projection FOV:
-          //   verticalFOV = 2 * atan(0.5 / altitude)
-          // and the camera sits at slant distance = altitude from the
-          // focal point regardless of pitch.
-          //
-          // ArcGIS behaves differently: it uses a fixed 55° *diagonal*
-          // FOV and moves the camera closer to the focal point as tilt
-          // increases (keeping the focal point at screen center). Using
-          // the FOV-only formula (altitude = 0.5 / tan(verticalFOV/2))
-          // works at tilt=0 but diverges at high tilt because ArcGIS's
-          // actual camera is closer.
+          // Keep deck.gl's SceneView approximation anchored at the ArcGIS
+          // focal point every frame. Zoom comes from the actual horizontal
+          // ground meters-per-pixel at screen center (`toMap(center)` and
+          // `toMap(center + 1px)`), which stays stable under tilt in local
+          // SceneView. We keep the older `view.scale` formula only as a
+          // fallback when focal-point sampling is unavailable.
           //
-          // We match ArcGIS by deriving altitude from the *actual*
-          // slant distance between `camera.position` and the focal
-          // point on the ground, expressed in viewport-height units.
+          // deck.gl's `altitude` still couples camera distance and FOV, while
+          // ArcGIS keeps them separate. We therefore compute a slant-distance
+          // altitude from the live camera position and focal point, then only
+          // at high tilt blend toward the ArcGIS fixed-FOV altitude. That
+          // preserves the better mid-tilt camera match while reducing the
+          // remaining extreme-angle drift.
           const cameraPos = self.view.camera.position;
           const focalPoint = self.view.toMap({x: width / 2, y: height / 2} as any);
           const latitude = focalPoint ? focalPoint.latitude : self.view.center.latitude;
           const longitude = focalPoint ? focalPoint.longitude : self.view.center.longitude;
 
-          // Horizontal ground distance from camera to focal point (meters).
-          // Use a flat-earth approximation; viewingMode='local' in the
-          // SceneView makes this exact.
           const midLatRad = (((latitude + cameraPos.latitude) / 2) * Math.PI) / 180;
           const dLatM = (cameraPos.latitude - latitude) * METERS_PER_DEG_LAT;
           const dLngM =
@@ -226,8 +196,6 @@ export default function createDeckRenderer(DeckProps, RenderNode) {
 
           const zoom = getZoom(self.view, longitude, latitude, width, height);
 
-          // Viewport height in meters at the focal plane. ArcGIS's
-          // `view.resolution` is meters-per-pixel at the focal point.
           const viewportHeightM = height * self.view.resolution;
           const slantAltitude = slantM / viewportHeightM;
           const altitude = getAltitude(self.view.camera.tilt, slantAltitude, width, height);
@@ -277,8 +245,6 @@ export default function createDeckRenderer(DeckProps, RenderNode) {
       this.renderNode = new DeckRenderNode({view: this.view});
     }
 
-    // Kept for API compatibility. Resource initialization now happens lazily
-    // inside the RenderNode's render() callback on the first frame.
     // eslint-disable-next-line @typescript-eslint/no-unused-vars
     async setup(_context?: unknown) {}
 
diff --git a/test/apps/arcgis-i3s/app.js b/test/apps/arcgis-i3s/app.js
@@ -32,7 +32,8 @@ loadArcGISModules(['esri/Map', 'esri/views/SceneView', 'esri/views/3d/webgl/Rend
     viewingMode: 'local'
   });
 
-  const renderer = new DeckRenderer(sceneView, {
+  const renderer = new DeckRenderer({
+    view: sceneView,
     layers: [
       new Tile3DLayer({
         id: 'tile-3d-layer',
@@ -42,13 +43,4 @@ loadArcGISModules(['esri/Map', 'esri/views/SceneView', 'esri/views/3d/webgl/Rend
       })
     ]
   });
-
-  // DeckRenderer is an ArcGIS RenderNode; it self-registers with the
-  // SceneView via the view property and does not need to be added as a layer.
-  // eslint-disable-next-line no-unused-expressions
-  renderer;
-
-  // Debug hook
-  window.__sceneView = sceneView;
-  window.__deckRenderer = renderer;
 });
diff --git a/test/apps/arcgis/app.js b/test/apps/arcgis/app.js
@@ -68,10 +68,7 @@ loadArcGISModules(
     viewingMode: 'local'
   });
 
-  const renderer = new DeckRenderer(sceneView, {});
-
-  // DeckRenderer is an ArcGIS RenderNode; it registers itself with the
-  // SceneView via the view property and does not need to be added as a layer.
+  const renderer = new DeckRenderer({view: sceneView});
 
   /* global setInterval */
   setInterval(() => {
diff --git a/test/modules/core/shaderlib/project/project-functions.spec.ts b/test/modules/core/shaderlib/project/project-functions.spec.ts
@@ -149,17 +149,25 @@ test('project#projectPosition', () => {
   }
 });
 
-test('project#projectPosition rejects legacy numeric coordinate systems', () => {
+test('project#projectPosition normalizes legacy numeric coordinate systems', () => {
+  const numericResult = projectPosition([-122.46, 37.8, 1000], {
+    viewport: TEST_VIEWPORT,
+    coordinateSystem: 2 as never,
+    coordinateOrigin: TEST_COORDINATE_ORIGIN,
+    fromCoordinateSystem: 1 as never
+  });
+
+  const stringResult = projectPosition([-122.46, 37.8, 1000], {
+    viewport: TEST_VIEWPORT,
+    coordinateSystem: 'meter-offsets',
+    coordinateOrigin: TEST_COORDINATE_ORIGIN,
+    fromCoordinateSystem: 'lnglat'
+  });
+
   expect(
-    () =>
-      projectPosition([-122.46, 37.8, 1000], {
-        viewport: TEST_VIEWPORT,
-        coordinateSystem: 2 as never,
-        coordinateOrigin: TEST_COORDINATE_ORIGIN,
-        fromCoordinateSystem: 1 as never
-      }),
-    'Legacy numeric coordinate systems are rejected'
-  ).toThrow(/Invalid coordinateSystem/);
+    equals(numericResult, stringResult),
+    'Legacy numeric coordinate systems normalize to string equivalents'
+  ).toBeTruthy();
 
   const identityResult = projectPosition([0, 0, 0], {
     viewport: TEST_VIEWPORT,
diff --git a/test/modules/core/shaderlib/project/viewport-uniforms.spec.ts b/test/modules/core/shaderlib/project/viewport-uniforms.spec.ts
@@ -154,21 +154,32 @@ test('project#getUniforms', () => {
   ).toBeTruthy();
 });
 
-test('project#getUniforms rejects legacy numeric coordinate systems', () => {
-  expect(
-    () => project.getUniforms({viewport: TEST_VIEWPORTS.map, coordinateSystem: -1 as never}),
-    'Legacy DEFAULT is rejected'
-  ).toThrow(/Invalid coordinateSystem/);
+test('project#getUniforms normalizes legacy numeric coordinate systems', () => {
+  const numericDefault = project.getUniforms({
+    viewport: TEST_VIEWPORTS.map,
+    coordinateSystem: -1 as never
+  });
+  const stringDefault = project.getUniforms({
+    viewport: TEST_VIEWPORTS.map,
+    coordinateSystem: COORDINATE_SYSTEM.DEFAULT
+  });
 
-  expect(
-    () =>
-      project.getUniforms({
-        viewport: TEST_VIEWPORTS.mapHighZoom,
-        coordinateSystem: 2 as never,
-        coordinateOrigin: Object.freeze([-122.4, 37.7])
-      }),
-    'Legacy offsets are rejected'
-  ).toThrow(/Invalid coordinateSystem/);
+  expect(numericDefault, 'Legacy DEFAULT normalizes to current DEFAULT').toEqual(stringDefault);
+
+  const numericOffsets = project.getUniforms({
+    viewport: TEST_VIEWPORTS.mapHighZoom,
+    coordinateSystem: 2 as never,
+    coordinateOrigin: Object.freeze([-122.4, 37.7])
+  });
+  const stringOffsets = project.getUniforms({
+    viewport: TEST_VIEWPORTS.mapHighZoom,
+    coordinateSystem: COORDINATE_SYSTEM.METER_OFFSETS,
+    coordinateOrigin: Object.freeze([-122.4, 37.7])
+  });
+
+  expect(numericOffsets, 'Legacy offsets normalize to current meter-offsets behavior').toEqual(
+    stringOffsets
+  );
 });
 
 test('project#getUniforms normalizes IDENTITY to CARTESIAN', () => {
