Address reviewer feedback

Author: mzschwartz5

packages/engine/Source/Scene/BufferPointCollection.js

@@ -98,11 +98,7 @@ class BufferPointCollection extends BufferPrimitiveCollection {
    * @ignore
    */
   _cloneEmpty(capacity = Frozen.EMPTY_OBJECT) {
-    return new BufferPointCollection({
-      primitiveCountMax: capacity.primitiveCountMax ?? this.primitiveCountMax,
-      positionDatatype: this.positionDatatype,
-      positionNormalized: this.positionNormalized,
-    });
+    return new BufferPointCollection(this._cloneEmptyBaseArgs(capacity));
   }
 
   /////////////////////////////////////////////////////////////////////////////

packages/engine/Source/Scene/BufferPolygonCollection.js

@@ -252,12 +252,9 @@ class BufferPolygonCollection extends BufferPrimitiveCollection {
    */
   _cloneEmpty(capacity = Frozen.EMPTY_OBJECT) {
     return new BufferPolygonCollection({
-      primitiveCountMax: capacity.primitiveCountMax ?? this.primitiveCountMax,
-      vertexCountMax: capacity.vertexCountMax ?? this.vertexCountMax,
+      ...this._cloneEmptyBaseArgs(capacity),
       holeCountMax: capacity.holeCountMax ?? this.holeCountMax,
       triangleCountMax: capacity.triangleCountMax ?? this.triangleCountMax,
-      positionDatatype: this.positionDatatype,
-      positionNormalized: this.positionNormalized,
     });
   }
 

packages/engine/Source/Scene/BufferPolylineCollection.js

@@ -81,12 +81,7 @@ class BufferPolylineCollection extends BufferPrimitiveCollection {
    * @ignore
    */
   _cloneEmpty(capacity = Frozen.EMPTY_OBJECT) {
-    return new BufferPolylineCollection({
-      primitiveCountMax: capacity.primitiveCountMax ?? this.primitiveCountMax,
-      vertexCountMax: capacity.vertexCountMax ?? this.vertexCountMax,
-      positionDatatype: this.positionDatatype,
-      positionNormalized: this.positionNormalized,
-    });
+    return new BufferPolylineCollection(this._cloneEmptyBaseArgs(capacity));
   }
 
   /////////////////////////////////////////////////////////////////////////////

packages/engine/Source/Scene/BufferPrimitiveCollection.js

@@ -467,9 +467,14 @@ class BufferPrimitiveCollection {
     result._primitiveCount = collection._primitiveCount;
     result._positionCount = collection._positionCount;
 
-    // Unset PickIds.
+    // Copy per-primitive pick objects and unset each GPU PickId.
+    // PickIds are regenerated for the result collection on next render.
+    const srcPickObjects = collection._pickObjects;
+    const dstPickObjects = result._pickObjects;
+    dstPickObjects.length = 0;
     const primitive = new PrimitiveClass();
     for (let i = 0, il = result.primitiveCount; i < il; i++) {
+      dstPickObjects[i] = srcPickObjects[i];
       result.get(i, primitive)._pickId = 0;
     }
 
@@ -485,11 +490,18 @@ class BufferPrimitiveCollection {
    * Returns a copy of this collection resized to the given capacities, with all
    * primitives copied in. The new collection must be large enough to hold every primitive.
    *
+   * <p>Collection-level state (model matrix, blend option, picking, bounding-volume
+   * mode, etc.) is carried over, but GPU resources are not: the source collection
+   * retains ownership of its renderer resources, so the caller is responsible for
+   * calling {@link BufferPrimitiveCollection#destroy} on the source once it is no
+   * longer needed.</p>
+   *
    * @example
-   * const grown = collection.toCapacity({
+   * const grown = collection.withCapacity({
    *   primitiveCountMax: collection.primitiveCountMax * 2,
    *   vertexCountMax: collection.vertexCountMax * 2,
    * });
+   * collection.destroy(); // release the source collection's GPU resources
    *
    * @param {BufferPrimitiveCapacity} [capacity] Capacities for the new collection.
    * @returns {BufferPrimitiveCollection<T>}
@@ -500,6 +512,35 @@ class BufferPrimitiveCollection {
     return result;
   }
 
+  /**
+   * Base constructor arguments that carry over collection-level state to an
+   * empty copy created by {@link BufferPrimitiveCollection#withCapacity}.
+   * Subclasses should spread the result into their constructor arguments,
+   * adding any type-specific capacities. Omitted capacities are inherited from
+   * this collection.
+   *
+   * @param {BufferPrimitiveCapacity} capacity
+   * @returns {object}
+   * @protected
+   * @ignore
+   */
+  _cloneEmptyBaseArgs(capacity) {
+    return {
+      primitiveCountMax: capacity.primitiveCountMax ?? this.primitiveCountMax,
+      vertexCountMax: capacity.vertexCountMax ?? this.vertexCountMax,
+      positionDatatype: this.positionDatatype,
+      positionNormalized: this.positionNormalized,
+      modelMatrix: this._modelMatrix,
+      show: this.show,
+      debugShowBoundingVolume: this.debugShowBoundingVolume,
+      blendOption: this._blendOption,
+      allowPicking: this._allowPicking,
+      boundingVolume: this._boundingVolumeAutoUpdate
+        ? undefined
+        : this._boundingVolume,
+    };
+  }
+
   /**
    * Returns an empty collection with the same structure as this one, sized to the
    * given capacity. Omitted capacities are inherited from this collection.

packages/engine/Source/Scene/ClippingPolygonCollection.js

@@ -23,7 +23,6 @@ import ComputeCommand from "../Renderer/ComputeCommand.js";
 import PolygonSignedDistanceFS from "../Shaders/PolygonSignedDistanceFS.js";
 import Pass from "../Renderer/Pass.js";
 import BufferPolygonCollection from "./BufferPolygonCollection.js";
-import BufferPrimitiveCollection from "./BufferPrimitiveCollection.js";
 import BufferPolygon from "./BufferPolygon.js";
 
 // Reused flyweight for reading/writing individual BufferPolygons.
@@ -84,19 +83,17 @@ function ClippingPolygonCollection(options) {
    */
   this._polygons = [];
 
-  // Add each ClippingPolygon object.
   const polygons = options.polygons;
   let numVertices = 0;
   if (defined(polygons)) {
     const polygonsLength = polygons.length;
     for (let i = 0; i < polygonsLength; ++i) {
-      this._polygons.push({ clippingPolygon: polygons[i], bufferIndex: -1 });
       numVertices += polygons[i].length;
     }
   }
 
-  // Note: update uses this as a heuristic for tracking changes to the collections. Leave it as 0 for now so that
-  // the first update loop always runs.
+  // Note: update uses this as a sentinel for tracking changes to the collections. Leave it as 0 for now so that
+  // the first update loop always runs, even though we already know the value (numVertices).
   this._totalPositions = 0;
 
   // For now: this is a write-through mirror of the polygons array. In upcoming work,
@@ -105,15 +102,9 @@ function ClippingPolygonCollection(options) {
   this._bufferPolygonCollection = new BufferPolygonCollection({
     // We just need it as a data structure, set show to false to prevent unnecessary render buffer allocations.
     show: false,
-    // Preallocate double the number of polygons
-    primitiveCountMax:
-      polygons?.length > 0
-        ? 2 * polygons.length
-        : BufferPrimitiveCollection.DEFAULT_CAPACITY,
-    vertexCountMax:
-      numVertices > 0
-        ? 2 * numVertices
-        : BufferPrimitiveCollection.DEFAULT_CAPACITY,
+    // Preallocate double the initial data.
+    primitiveCountMax: 2 * (polygons?.length ?? 0),
+    vertexCountMax: 2 * numVertices,
     // ClippingPolygonCollection does not support holes currently (when this changes, update accordingly)
     holeCountMax: 0,
     // This may be fine to stay as 0: we do not need the triangulation for Vector-based clipping.
@@ -123,8 +114,7 @@ function ClippingPolygonCollection(options) {
   if (defined(polygons)) {
     for (let i = 0; i < polygons.length; ++i) {
       const positions = polygons[i].positions;
-      this._polygons[i].bufferIndex =
-        this._bufferPolygonCollection.primitiveCount;
+      const bufferIndex = this._bufferPolygonCollection.primitiveCount;
       this._bufferPolygonCollection.add(
         {
           positions: Cartesian3.packArray(
@@ -134,6 +124,10 @@ function ClippingPolygonCollection(options) {
         },
         bufferPolygonScratch,
       );
+      this._polygons.push({
+        clippingPolygon: polygons[i],
+        bufferIndex: bufferIndex,
+      });
     }
   }
 
@@ -356,6 +350,8 @@ function reserveBufferCapacity(collection, addedVertexCount) {
     primitiveCountMax: Math.max(2 * buffer.primitiveCountMax, neededPrimitives),
     vertexCountMax: Math.max(2 * buffer.vertexCountMax, neededVertices),
   });
+
+  buffer.destroy();
   collection._bufferPolygonCollection = grown;
   return grown;
 }

packages/engine/Specs/Scene/BufferPolygonCollectionSpec.js

@@ -4,6 +4,7 @@ import {
   Color,
   ComponentDatatype,
   Matrix4,
+  BlendOption,
   BufferPolygon,
   BufferPolygonCollection,
   BufferPolygonMaterial,
@@ -322,6 +323,45 @@ describe("Scene/BufferPolygonCollection", () => {
     expect(inherited.triangleCountMax).toBe(4);
   });
 
+  it("withCapacity transfers collection state", () => {
+    const modelMatrix = Matrix4.fromTranslation(new Cartesian3(1, 2, 3));
+    const boundingVolume = new BoundingSphere(new Cartesian3(4, 5, 6), 7);
+    const pickObject = { id: "picked" };
+
+    const src = new BufferPolygonCollection({
+      primitiveCountMax: 2,
+      vertexCountMax: 6,
+      holeCountMax: 1,
+      triangleCountMax: 4,
+      modelMatrix: modelMatrix,
+      blendOption: BlendOption.OPAQUE,
+      allowPicking: true,
+      boundingVolume: boundingVolume,
+      debugShowBoundingVolume: true,
+    });
+
+    const polygon = new BufferPolygon();
+    src.add(
+      { positions: createBoxPositions(3), pickObject: pickObject },
+      polygon,
+    );
+
+    const dst = src.withCapacity({ primitiveCountMax: 4 });
+
+    // Constructor-only collection state is carried over.
+    expect(dst.modelMatrix).toEqual(modelMatrix);
+    expect(dst._blendOption).toBe(BlendOption.OPAQUE);
+    expect(dst._allowPicking).toBe(true);
+    expect(dst.debugShowBoundingVolume).toBe(true);
+
+    // Manual bounding-volume mode and value are preserved.
+    expect(dst._boundingVolumeAutoUpdate).toBe(false);
+    expect(dst.boundingVolume).toEqual(boundingVolume);
+
+    // Per-primitive pick objects are copied.
+    expect(dst._pickObjects[0]).toBe(pickObject);
+  });
+
   it("sort", () => {
     const collection = new BufferPolygonCollection({
       primitiveCountMax: 3,