Handle offset in DataColumn (#3902)

visgl · Nov 20, 2019 · beb7293 · beb7293
1 parent 9ee3f45
commit beb7293
Show file tree

Hide file tree

Showing 8 changed files with 54 additions and 72 deletions.
diff --git a/modules/core/src/lib/attribute/attribute-transition-utils.js b/modules/core/src/lib/attribute/attribute-transition-utils.js
@@ -96,7 +96,7 @@ export function padBuffer({
   // its `size` and `elementOffset`?
   const precisionMultiplier = attribute.doublePrecision ? 2 : 1;
   const size = attribute.size * precisionMultiplier;
-  const offset = attribute.elementOffset * precisionMultiplier;
+  const byteOffset = attribute.byteOffset;
   const toStartIndices = attribute.startIndices;
   const hasStartIndices = fromStartIndices && toStartIndices;
   const toLength = getAttributeBufferLength(attribute, numInstances);
@@ -107,7 +107,9 @@ export function padBuffer({
     return;
   }
 
-  const toData = isConstant ? attribute.value : attribute.getBuffer().getData({});
+  const toData = isConstant
+    ? attribute.value
+    : attribute.getBuffer().getData({srcByteOffset: byteOffset});
   if (attribute.settings.normalized) {
     const getter = getData;
     getData = (value, chunk) => attribute._normalizeConstant(getter(value, chunk));
@@ -124,10 +126,13 @@ export function padBuffer({
     target: data,
     sourceStartIndices: fromStartIndices,
     targetStartIndices: toStartIndices,
-    offset,
     size,
     getData: getMissingData
   });
 
-  buffer.setData({data});
+  // TODO: support offset in buffer.setData?
+  if (buffer.byteLength < data.byteLength + byteOffset) {
+    buffer.reallocate(data.byteLength + byteOffset);
+  }
+  buffer.subData({data, offset: byteOffset});
 }
diff --git a/modules/core/src/lib/attribute/attribute.js b/modules/core/src/lib/attribute/attribute.js
@@ -1,5 +1,4 @@
 /* eslint-disable complexity */
-import {_Accessor as Accessor} from '@luma.gl/core';
 import DataColumn from './data-column';
 import assert from '../../utils/assert';
 import {createIterable} from '../../utils/iterable-utils';
@@ -13,7 +12,6 @@ export default class Attribute extends DataColumn {
 
     const {
       // deck.gl fields
-      offset = 0,
       transition = false,
       noAlloc = false,
       update = null,
@@ -27,7 +25,6 @@ export default class Attribute extends DataColumn {
       noAlloc,
       update: update || (accessor && this._standardAccessor),
       accessor,
-      elementOffset: offset / Accessor.getBytesPerElement(this.settings),
       transform
     });
 
@@ -223,7 +220,7 @@ export default class Attribute extends DataColumn {
 
   getVertexOffset(row, startIndices = this.startIndices) {
     const vertexIndex = startIndices ? startIndices[row] : row;
-    return this.settings.elementOffset + vertexIndex * this.size;
+    return vertexIndex * this.size;
   }
 
   getShaderAttributes() {

diff --git a/modules/core/src/lib/attribute/data-column.js b/modules/core/src/lib/attribute/data-column.js
@@ -10,7 +10,7 @@ import log from '../../utils/log';
 function addDoublePrecisionAttributes(id, baseAccessor, shaderAttributeOptions) {
   const doubleShaderAttributeDefs = {};
   const offset =
-    'offset' in shaderAttributeOptions ? shaderAttributeOptions.offset : baseAccessor.offset || 0;
+    'offset' in shaderAttributeOptions ? shaderAttributeOptions.offset : baseAccessor.offset;
   const stride =
     'stride' in shaderAttributeOptions ? shaderAttributeOptions.stride : baseAccessor.size * 4;
 
@@ -73,6 +73,7 @@ export default class DataColumn {
 
     this.value = null;
     this.settings = Object.assign({}, opts, {
+      offset: opts.offset || 0,
       defaultValue
     });
     this.state = {
@@ -99,6 +100,11 @@ export default class DataColumn {
     return this._buffer;
   }
 
+  get byteOffset() {
+    const {offset} = this.settings;
+    return this.doublePrecision && this.value instanceof Float64Array ? offset * 2 : offset;
+  }
+
   delete() {
     if (this._buffer) {
       this._buffer.delete();
@@ -187,13 +193,19 @@ export default class DataColumn {
       state.externalBuffer = null;
       state.constant = false;
       this.value = value;
+      const {buffer, byteOffset} = this;
 
       if (this.doublePrecision && value instanceof Float64Array) {
         value = toDoublePrecisionArray(value, this);
       }
-
-      this.buffer.setData(value);
-      opts.type = this.buffer.accessor.type;
+      // TODO: support offset in buffer.setData?
+      if (buffer.byteLength < value.byteLength + byteOffset) {
+        buffer.reallocate(value.byteLength + byteOffset);
+      }
+      // Hack: force Buffer to infer data type
+      buffer.setAccessor(null);
+      buffer.subData({data: value, offset: byteOffset});
+      opts.type = buffer.accessor.type;
     }
 
     state.bufferAccessor = {...this.settings, ...opts};
@@ -212,7 +224,7 @@ export default class DataColumn {
               endIndex: endOffset
             })
           : value.subarray(startOffset, endOffset),
-      offset: startOffset * value.BYTES_PER_ELEMENT
+      offset: startOffset * value.BYTES_PER_ELEMENT + this.byteOffset
     });
   }
 
@@ -221,29 +233,30 @@ export default class DataColumn {
     const oldValue = state.allocatedValue;
 
     // Allocate at least one element to ensure a valid buffer
-    this.value = typedArrayManager.allocate(oldValue, numInstances + 1, {
+    const value = typedArrayManager.allocate(oldValue, numInstances + 1, {
       size: this.size,
       type: this.defaultType,
       copy
     });
+    this.value = value;
+    const {buffer, byteOffset} = this;
 
-    if (this.buffer.byteLength < this.value.byteLength) {
-      this.buffer.reallocate(this.value.byteLength);
+    if (buffer.byteLength < value.byteLength + byteOffset) {
+      buffer.reallocate(value.byteLength + byteOffset);
 
       if (copy && oldValue) {
         // Upload the full existing attribute value to the GPU, so that updateBuffer
         // can choose to only update a partial range.
         // TODO - copy old buffer to new buffer on the GPU
-        this.buffer.subData({
+        buffer.subData({
           data:
-            oldValue instanceof Float64Array
-              ? toDoublePrecisionArray(oldValue, {size: this.size})
-              : oldValue
+            oldValue instanceof Float64Array ? toDoublePrecisionArray(oldValue, this) : oldValue,
+          offset: byteOffset
         });
       }
     }
 
-    state.allocatedValue = this.value;
+    state.allocatedValue = value;
     state.constant = false;
     state.externalBuffer = null;
     state.bufferAccessor = this.settings;

diff --git a/modules/core/src/utils/array-utils.js b/modules/core/src/utils/array-utils.js
@@ -61,15 +61,7 @@ function padArrayChunk({source, target, start = 0, end, getData}) {
  * @params {Array<Number>} [sourceStartIndices] - subdivision of the original data in [object0StartIndex, object1StartIndex, ...]
  * @params {Array<Number>} [targetStartIndices] - subdivision of the output data in [object0StartIndex, object1StartIndex, ...]
  */
-export function padArray({
-  source,
-  target,
-  size,
-  offset = 0,
-  getData,
-  sourceStartIndices,
-  targetStartIndices
-}) {
+export function padArray({source, target, size, getData, sourceStartIndices, targetStartIndices}) {
   if (!Array.isArray(targetStartIndices)) {
     // Flat arrays
     padArrayChunk({
@@ -81,15 +73,15 @@ export function padArray({
   }
 
   // Arrays have internal structure
-  let sourceIndex = offset;
-  let targetIndex = offset;
+  let sourceIndex = 0;
+  let targetIndex = 0;
   const getChunkData = getData && ((i, chunk) => getData(i + targetIndex, chunk));
 
   const n = Math.min(sourceStartIndices.length, targetStartIndices.length);
 
   for (let i = 1; i < n; i++) {
-    const nextSourceIndex = sourceStartIndices[i] * size + offset;
-    const nextTargetIndex = targetStartIndices[i] * size + offset;
+    const nextSourceIndex = sourceStartIndices[i] * size;
+    const nextTargetIndex = targetStartIndices[i] * size;
 
     padArrayChunk({
       source: source.subarray(sourceIndex, nextSourceIndex),

diff --git a/modules/layers/src/path-layer/path-layer.js b/modules/layers/src/path-layer/path-layer.js
@@ -63,8 +63,7 @@ export default class PathLayer extends Layer {
     attributeManager.addInstanced({
       positions: {
         size: 3,
-        // Hack - Attribute class needs this to properly apply partial update
-        // The first 3 numbers of the value is just padding
+        // Start filling buffer from 3 elements in
         offset: 12,
         type: GL.DOUBLE,
         fp64: this.use64bitPositions(),

diff --git a/modules/layers/src/path-layer/path-tesselator.js b/modules/layers/src/path-layer/path-tesselator.js
@@ -33,7 +33,7 @@ export default class PathTesselator extends Tesselator {
       getGeometry,
       positionFormat,
       attributes: {
-        positions: {size: 3, padding: 3, type: fp64 ? Float64Array : Float32Array},
+        positions: {size: 3, type: fp64 ? Float64Array : Float32Array},
         segmentTypes: {size: 1, type: Uint8ClampedArray}
       }
     });
@@ -92,9 +92,9 @@ export default class PathTesselator extends Tesselator {
     // segmentTypes     3  4  4  0  0  0  0  4  4
     for (let i = vertexStart, ptIndex = 0; ptIndex < geometrySize; i++, ptIndex++) {
       const p = this.getPointOnPath(path, ptIndex);
-      positions[i * 3 + 3] = p[0];
-      positions[i * 3 + 4] = p[1];
-      positions[i * 3 + 5] = p[2] || 0;
+      positions[i * 3] = p[0];
+      positions[i * 3 + 1] = p[1];
+      positions[i * 3 + 2] = p[2] || 0;
     }
   }
 

diff --git a/test/modules/core/lib/attribute/attribute.spec.js b/test/modules/core/lib/attribute/attribute.spec.js
@@ -710,7 +710,6 @@ test('Attribute#setExternalBuffer', t => {
   t.is(attribute.getAccessor().type, GL.FLOAT, 'attribute type is set correctly');
 
   t.ok(attribute.setExternalBuffer(value2), 'should set external buffer to typed array');
-  t.is(attribute.getBuffer().debugData.constructor.name, 'Uint8Array', 'external value is set');
   t.is(attribute.getAccessor().type, GL.UNSIGNED_BYTE, 'attribute type is set correctly');
 
   t.ok(attribute2.setExternalBuffer(value2), 'external value is set');
@@ -797,11 +796,6 @@ test('Attribute#doublePrecision', t0 => {
 
     attribute.setExternalBuffer(new Uint32Array([3, 4, 5, 4, 4, 5]));
     t.ok(attribute.value instanceof Uint32Array, 'Attribute is Uint32Array');
-    t.deepEqual(
-      attribute.buffer.debugData.slice(0, 6),
-      [3, 4, 5, 4, 4, 5],
-      'Attribute value is set'
-    );
     validateShaderAttributes(t, attribute, false);
 
     t.throws(
@@ -811,11 +805,6 @@ test('Attribute#doublePrecision', t0 => {
 
     attribute.setExternalBuffer(new Float64Array([3, 4, 5, 4, 4, 5]));
     t.ok(attribute.value instanceof Float64Array, 'Attribute is Float64Array');
-    t.deepEqual(
-      attribute.buffer.debugData.slice(0, 6),
-      [3, 4, 5, 0, 0, 0],
-      'Attribute value is set'
-    );
     validateShaderAttributes(t, attribute, true);
 
     const buffer = new Buffer(gl, 12);
@@ -850,11 +839,6 @@ test('Attribute#doublePrecision', t0 => {
 
     attribute.setExternalBuffer(new Uint32Array([3, 4, 5, 4, 4, 5]));
     t.ok(attribute.value instanceof Uint32Array, 'Attribute is Uint32Array');
-    t.deepEqual(
-      attribute.buffer.debugData.slice(0, 6),
-      [3, 4, 5, 4, 4, 5],
-      'Attribute value is set'
-    );
     validateShaderAttributes(t, attribute, false);
 
     t.throws(
@@ -864,11 +848,6 @@ test('Attribute#doublePrecision', t0 => {
 
     attribute.setExternalBuffer(new Float64Array([3, 4, 5, 4, 4, 5]));
     t.ok(attribute.value instanceof Float64Array, 'Attribute is Float64Array');
-    t.deepEqual(
-      attribute.buffer.debugData.slice(0, 6),
-      [3, 4, 5, 0, 0, 0],
-      'Attribute value is set'
-    );
     validateShaderAttributes(t, attribute, true);
 
     const buffer = new Buffer(gl, 12);

diff --git a/test/modules/layers/path-tesselator.spec.js b/test/modules/layers/path-tesselator.spec.js
@@ -84,13 +84,13 @@ test('PathTesselator#constructor', t => {
 
       t.ok(ArrayBuffer.isView(tesselator.get('positions')), 'PathTesselator.get positions');
       t.deepEquals(
-        tesselator.get('positions').slice(0, 12),
-        [0, 0, 0, 1, 1, 0, 2, 2, 0, 3, 3, 0],
+        tesselator.get('positions').slice(0, 9),
+        [1, 1, 0, 2, 2, 0, 3, 3, 0],
         'positions are filled'
       );
 
       t.deepEquals(
-        tesselator.get('positions').slice(24, 33),
+        tesselator.get('positions').slice(21, 30),
         [2, 2, 0, 3, 3, 0, 1, 1, 0],
         'positions is handling loop correctly'
       );
@@ -119,23 +119,20 @@ test('PathTesselator#partial update', t => {
   let positions = tesselator.get('positions').slice(0, 21);
   t.is(tesselator.instanceCount, 9, 'Initial instance count');
   t.deepEquals(
-    positions,
-    [0, 0, 0, 1, 1, 0, 2, 2, 0, 3, 3, 0, 1, 1, 0, 2, 2, 0, 3, 3, 0],
+    positions.slice(0, 18),
+    [1, 1, 0, 2, 2, 0, 3, 3, 0, 1, 1, 0, 2, 2, 0, 3, 3, 0],
     'positions'
   );
   t.deepEquals(Array.from(accessorCalled), ['A', 'B'], 'Accessor called on all data');
 
   sampleData[2] = {path: [[4, 4], [5, 5], [6, 6]], id: 'C'};
   accessorCalled.clear();
   tesselator.updatePartialGeometry({startRow: 2});
-  positions = tesselator.get('positions').slice(0, 39);
+  positions = tesselator.get('positions').slice(0, 36);
   t.is(tesselator.instanceCount, 12, 'Updated instance count');
   t.deepEquals(
     positions,
     [
-      0,
-      0,
-      0,
       1,
       1,
       0,
@@ -180,11 +177,11 @@ test('PathTesselator#partial update', t => {
   sampleData[0] = {path: [[6, 6], [5, 5], [4, 4]], id: 'A'};
   accessorCalled.clear();
   tesselator.updatePartialGeometry({startRow: 0, endRow: 1});
-  positions = tesselator.get('positions').slice(0, 30);
+  positions = tesselator.get('positions').slice(0, 27);
   t.is(tesselator.instanceCount, 12, 'Updated instance count');
   t.deepEquals(
     positions,
-    [0, 0, 0, 6, 6, 0, 5, 5, 0, 4, 4, 0, 1, 1, 0, 2, 2, 0, 3, 3, 0, 1, 1, 0, 2, 2, 0, 3, 3, 0],
+    [6, 6, 0, 5, 5, 0, 4, 4, 0, 1, 1, 0, 2, 2, 0, 3, 3, 0, 1, 1, 0, 2, 2, 0, 3, 3, 0],
     'positions'
   );
   t.deepEquals(Array.from(accessorCalled), ['A'], 'Accessor called only on partial data');