refactor(bounds): fix getBounds to return a copy of model.bounds

A getter should return a copy and not a reference. getBoundsByReference can return the actual
model.bounds array.

BREAKING CHANGE: Mappers should overwrite computeBounds instead of getBounds

Author: finetjul

BREAKING_CHANGES.md

@@ -1,3 +1,8 @@
+## From 34.x to 35
+
+- **vtkMapper**: mappers should overwrite `computeBounds()` instead of `getBounds` that now calls `computeBounds()` before returning a copy of the mapper bounds.
+
+
 ## From 33.x to 34
 
 - **vtkMapper**: many properties have moved to `vtkVolumeProperty`. The full list of changed methods is: `getAnisotropy`, `getComputeNormalFromOpacity`, `getFilterMode`, `getFilterModeAsString`, `getGlobalIlluminationReach`, `getIpScalarRange`, `getIpScalarRangeByReference`, `getLAOKernelRadius`, `getLAOKernelSize`, `getLocalAmbientOcclusion`, `getPreferSizeOverAccuracy`, `getVolumetricScatteringBlending`, `setAnisotropy`, `setAverageIPScalarRange`, `setComputeNormalFromOpacity`, `setFilterMode`, `setFilterModeToNormalized`, `setFilterModeToOff`, `setFilterModeToRaw`, `setGlobalIlluminationReach`, `setIpScalarRange`, `setIpScalarRangeFrom`, `setLAOKernelRadius`, `setLAOKernelSize`, `setLocalAmbientOcclusion`, `setPreferSizeOverAccuracy`, `setVolumetricScatteringBlending`.

Sources/Common/DataModel/BoundingBox/index.d.ts

@@ -1,5 +1,5 @@
 import { mat4 } from 'gl-matrix';
-import { Bounds, Vector2, Vector3 } from '../../../types';
+import { Bounds, TypedArray, Vector2, Vector3 } from '../../../types';
 import vtkPoints from '../../Core/Points';
 import { Nullable } from '../../../types';
 
@@ -394,11 +394,16 @@ declare class BoundingBox {
   /**
    * Adds points to a bounding box.
    * @param {Bounds} bounds
-   * @param {number} x
+   * @param {number|Number[]|TypedArray} xOrPoint
    * @param {number} y
    * @param {number} z
    */
-  addPoint(bounds: Bounds, x: number, y: number, z: number): Bounds;
+  addPoint(
+    bounds: Bounds,
+    xOrPoint: number | number[] | TypedArray,
+    y?: number,
+    z?: number
+  ): Bounds;
 
   /**
    * Adds points to a bounding box.

Sources/Common/DataModel/BoundingBox/index.js

@@ -49,14 +49,23 @@ export function reset(bounds) {
   return setBounds(bounds, INIT_BOUNDS);
 }
 
-export function addPoint(bounds, x, y, z) {
+export function addPoint(bounds, xOrPoint, y, z) {
   const [xMin, xMax, yMin, yMax, zMin, zMax] = bounds;
-  bounds[0] = xMin < x ? xMin : x;
-  bounds[1] = xMax > x ? xMax : x;
-  bounds[2] = yMin < y ? yMin : y;
-  bounds[3] = yMax > y ? yMax : y;
-  bounds[4] = zMin < z ? zMin : z;
-  bounds[5] = zMax > z ? zMax : z;
+  if (arguments.length === 4) {
+    bounds[0] = xMin < xOrPoint ? xMin : xOrPoint;
+    bounds[1] = xMax > xOrPoint ? xMax : xOrPoint;
+    bounds[2] = yMin < y ? yMin : y;
+    bounds[3] = yMax > y ? yMax : y;
+    bounds[4] = zMin < z ? zMin : z;
+    bounds[5] = zMax > z ? zMax : z;
+  } else {
+    bounds[0] = xMin < xOrPoint[0] ? xMin : xOrPoint[0];
+    bounds[1] = xMax > xOrPoint[0] ? xMax : xOrPoint[0];
+    bounds[2] = yMin < xOrPoint[1] ? yMin : xOrPoint[1];
+    bounds[3] = yMax > xOrPoint[1] ? yMax : xOrPoint[1];
+    bounds[4] = zMin < xOrPoint[2] ? zMin : xOrPoint[2];
+    bounds[5] = zMax > xOrPoint[2] ? zMax : xOrPoint[2];
+  }
   return bounds;
 }
 

Sources/Common/DataModel/Box/index.js

@@ -120,7 +120,7 @@ function vtkBox(publicAPI, model) {
     vtkBoundingBox.setBounds(model.bbox, boundsArray);
   };
 
-  publicAPI.getBounds = () => model.bbox;
+  publicAPI.getBounds = () => [...model.bbox];
 
   publicAPI.evaluateFunction = (x, y, z) => {
     const point = Array.isArray(x) ? x : [x, y, z];

Sources/Proxy/Core/ViewProxy/index.js

@@ -353,10 +353,11 @@ function vtkViewProxy(publicAPI, model) {
 
   // --------------------------------------------------------------------------
 
-  publicAPI.setBackground = macro.chain(
-    model.renderer.setBackground,
-    updateAnnotationColor
-  );
+  publicAPI.setBackground = (...args) => {
+    const res = model.renderer.setBackground(...args);
+    updateAnnotationColor();
+    return res;
+  };
 
   // --------------------------------------------------------------------------
 

Sources/Rendering/Core/AbstractMapper3D/index.d.ts

@@ -15,11 +15,30 @@ export interface IAbstractMapper3DInitialValues
 export interface vtkAbstractMapper3D extends vtkAbstractMapper {
   /**
    * Get the bounds for this mapper as [xmin, xmax, ymin, ymax,zmin, zmax].
-   * @default 0
-   * @return {Bounds} The bounds for the mapper.
+   * Bounds are (re)computed if needed.
+   * @return {Bounds} A copy of the bounds for the mapper.
+   * @see getBoundsByReference
+   * @see computeBounds
    */
   getBounds(): Bounds;
 
+  /**
+   * Get the bounds for this mapper as [xmin, xmax, ymin, ymax,zmin, zmax].
+   * Bounds are (re)computed if needed.
+   * @return {Bounds} The bounds array of the mapper.
+   * @see getBounds
+   * @see computeBounds
+   */
+  getBoundsByReference(): Bounds;
+
+  /**
+   * Compute the bounds for this mapper.
+   * Must be implemented by sub-classes. Called by getBounds methods.
+   * @see getBoundsByReference
+   * @see getBounds
+   */
+  computeBounds(): void;
+
   /**
    * Get the center of this mapper’s data.
    * @return {Vector3} The center of the mapper's data.

Sources/Rendering/Core/AbstractMapper3D/index.js

@@ -1,28 +1,38 @@
 import macro from 'vtk.js/Sources/macros';
 import vtkAbstractMapper from 'vtk.js/Sources/Rendering/Core/AbstractMapper';
 import vtkBoundingBox from 'vtk.js/Sources/Common/DataModel/BoundingBox';
-import { createUninitializedBounds } from 'vtk.js/Sources/Common/Core/Math';
 
 // ----------------------------------------------------------------------------
 // vtkAbstractMapper methods
 // ----------------------------------------------------------------------------
 
 function vtkAbstractMapper3D(publicAPI, model) {
+  publicAPI.computeBounds = () => {
+    macro.vtkErrorMacro(`vtkAbstractMapper3D.computeBounds - NOT IMPLEMENTED`);
+  };
+
+  const superGetBounds = publicAPI.getBounds;
   publicAPI.getBounds = () => {
-    macro.vtkErrorMacro(`vtkAbstractMapper3D.getBounds - NOT IMPLEMENTED`);
-    return createUninitializedBounds();
+    publicAPI.computeBounds();
+    return superGetBounds();
+  };
+
+  const superGetBoundsByReference = publicAPI.getBoundsByReference;
+  publicAPI.getBoundsByReference = () => {
+    publicAPI.computeBounds();
+    return superGetBoundsByReference();
   };
 
   publicAPI.getCenter = () => {
-    const bounds = publicAPI.getBounds();
+    const bounds = publicAPI.getBoundsByReference();
     model.center = vtkBoundingBox.isValid(bounds)
       ? vtkBoundingBox.getCenter(bounds)
       : null;
     return model.center?.slice();
   };
 
   publicAPI.getLength = () => {
-    const bounds = publicAPI.getBounds();
+    const bounds = publicAPI.getBoundsByReference();
     return vtkBoundingBox.getDiagonalLength(bounds);
   };
 }
@@ -46,6 +56,7 @@ export function extend(publicAPI, model, initialValues = {}) {
   vtkAbstractMapper.extend(publicAPI, model, initialValues);
 
   macro.setGet(publicAPI, model, ['viewSpecificProperties']);
+  macro.getArray(publicAPI, model, ['bounds'], 6);
 
   vtkAbstractMapper3D(publicAPI, model);
 }

Sources/Rendering/Core/CubeAxesActor/index.js

@@ -788,7 +788,7 @@ function vtkCubeAxesActor(publicAPI, model) {
   // as it relies on the pixel size of the window. Every time the camera
   // changes the bounds change. This method simplifies by just expanding
   // the grid bounds by a user specified factor.
-  publicAPI.getBounds = () => {
+  publicAPI.computeBounds = () => {
     publicAPI.update();
     vtkBoundingBox.setBounds(model.bounds, model.gridActor.getBounds());
     vtkBoundingBox.scaleAboutCenter(
@@ -797,15 +797,15 @@ function vtkCubeAxesActor(publicAPI, model) {
       model.boundsScaleFactor,
       model.boundsScaleFactor
     );
-    return model.bounds;
   };
 
   // Make sure the grid share the actor property
-  const _setProp = macro.chain(
-    publicAPI.setProperty,
-    model.gridActor.setProperty
-  );
-  publicAPI.setProperty = (p) => _setProp(p)[0];
+  const superSetProp = publicAPI.setProperty;
+  publicAPI.setProperty = (p) => {
+    const res = superSetProp(p);
+    model.gridActor.setProperty(p);
+    return res;
+  };
 }
 
 // ----------------------------------------------------------------------------

Sources/Rendering/Core/Glyph3DMapper/index.js

@@ -66,22 +66,16 @@ function vtkGlyph3DMapper(publicAPI, model) {
     return idata.getPointData().getArray(model.scaleArray);
   };
 
-  publicAPI.getBounds = () => {
-    const idata = publicAPI.getInputData(0);
-    const gdata = publicAPI.getInputData(1);
-    if (!idata || !gdata) {
-      return vtkMath.createUninitializedBounds();
-    }
-
-    // first we build the arrays used for the glyphing
-    publicAPI.buildArrays();
-    return model.bounds;
-  };
+  publicAPI.computeBounds = () => publicAPI.buildArrays();
 
   publicAPI.buildArrays = () => {
     // if the mtgime requires it, rebuild
     const idata = publicAPI.getInputData(0);
     const gdata = publicAPI.getInputData(1);
+    if (!idata || !gdata) {
+      vtkBoundingBox.reset(model.bounds);
+      return;
+    }
     if (
       model.buildTime.getMTime() < gdata.getMTime() ||
       model.buildTime.getMTime() < idata.getMTime() ||
@@ -114,12 +108,7 @@ function vtkGlyph3DMapper(publicAPI, model) {
       // overall bounds while building the arrays
       const corners = [];
       vtkBoundingBox.getCorners(gbounds, corners);
-      model.bounds[0] = vtkBoundingBox.INIT_BOUNDS[0];
-      model.bounds[1] = vtkBoundingBox.INIT_BOUNDS[1];
-      model.bounds[2] = vtkBoundingBox.INIT_BOUNDS[2];
-      model.bounds[3] = vtkBoundingBox.INIT_BOUNDS[3];
-      model.bounds[4] = vtkBoundingBox.INIT_BOUNDS[4];
-      model.bounds[5] = vtkBoundingBox.INIT_BOUNDS[5];
+      vtkBoundingBox.reset(model.bounds);
 
       const tcorner = new Float64Array(3);
 
@@ -225,24 +214,7 @@ function vtkGlyph3DMapper(publicAPI, model) {
         // update bounds
         for (let p = 0; p < 8; ++p) {
           vec3.transformMat4(tcorner, corners[p], z);
-          if (tcorner[0] < model.bounds[0]) {
-            model.bounds[0] = tcorner[0];
-          }
-          if (tcorner[1] < model.bounds[2]) {
-            model.bounds[2] = tcorner[1];
-          }
-          if (tcorner[2] < model.bounds[4]) {
-            model.bounds[4] = tcorner[2];
-          }
-          if (tcorner[0] > model.bounds[1]) {
-            model.bounds[1] = tcorner[0];
-          }
-          if (tcorner[1] > model.bounds[3]) {
-            model.bounds[3] = tcorner[1];
-          }
-          if (tcorner[2] > model.bounds[5]) {
-            model.bounds[5] = tcorner[2];
-          }
+          vtkBoundingBox.addPoint(model.bounds, tcorner);
         }
 
         const n = new Float32Array(nbuff, i * 36, 9);
@@ -333,9 +305,6 @@ export function extend(publicAPI, model, initialValues = {}) {
   model.buildTime = {};
   macro.obj(model.buildTime, { mtime: 0 });
 
-  model.boundsTime = {};
-  macro.obj(model.boundsTime, { mtime: 0 });
-
   macro.setGet(publicAPI, model, [
     'orient',
     'orientationMode',

Sources/Rendering/Core/HardwareSelector/test/testHardwareSelectorGlyph.js

@@ -108,14 +108,16 @@ test('Test HardwareSelectorGlyph', (tapeContext) => {
   sel.setFieldAssociation(FieldAssociations.FIELD_ASSOCIATION_POINTS);
 
   return sel.selectAsync(renderer, 200, 200, 250, 300).then((res) => {
-    const allGood = res.length === 7 && res[0].getProperties().prop === actor;
-
-    tapeContext.ok(res.length === 7, 'Seven glyphs selected');
+    tapeContext.equal(res.length, 7, 'Seven glyphs selected');
     tapeContext.ok(
       res[0].getProperties().compositeID === 71,
       'glyph 71 was the first selected'
     );
-    tapeContext.ok(allGood, 'Correct prop was selected');
+    tapeContext.equal(
+      res[0].getProperties().prop,
+      actor,
+      'Correct prop was selected'
+    );
 
     gc.releaseResources();
   });