Skip to content

Commit

Permalink
Attribute allocation bug fixes (visgl#3342)
Browse files Browse the repository at this point in the history
  • Loading branch information
Pessimistress authored Jul 12, 2019
1 parent ea0e58d commit 4de8312
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 25 deletions.
30 changes: 8 additions & 22 deletions modules/core/src/lib/attribute.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import typedArrayManager from '../utils/typed-array-manager';
const DEFAULT_STATE = {
isExternalBuffer: false,
lastExternalBuffer: null,
allocatedValue: null,
needsUpdate: true,
needsRedraw: false,
updateRanges: range.FULL
Expand Down Expand Up @@ -86,9 +87,7 @@ export default class Attribute extends BaseAttribute {

delete() {
super.delete();
if (!this.userData.isExternalBuffer) {
typedArrayManager.release(this.value);
}
typedArrayManager.release(this.userData.allocatedValue);
}

needsUpdate() {
Expand All @@ -101,10 +100,6 @@ export default class Attribute extends BaseAttribute {
return needsRedraw;
}

getInstanceCount() {
return this.value !== null ? this.value.length / this.size : 0;
}

getUpdateTriggers() {
const {accessor} = this.userData;

Expand Down Expand Up @@ -178,15 +173,12 @@ export default class Attribute extends BaseAttribute {
return false;
}

// Do we need to reallocate the attribute's typed array?
const instanceCount = this.getInstanceCount();
const needsAlloc = instanceCount === 0 || instanceCount < numInstances;
if (needsAlloc && (state.update || state.accessor)) {
if (state.update) {
assert(Number.isFinite(numInstances));
// Allocate at least one element to ensure a valid buffer
const allocCount = Math.max(numInstances, 1);
const ArrayType = glArrayFromType(this.type || GL.FLOAT);
const oldValue = this.value;
const oldValue = state.allocatedValue;
const shouldCopy = state.updateRanges !== range.FULL;

this.constant = false;
Expand All @@ -200,15 +192,15 @@ export default class Attribute extends BaseAttribute {
if (this.buffer && this.buffer.byteLength < this.value.byteLength) {
this.buffer.reallocate(this.value.byteLength);

if (shouldCopy) {
if (shouldCopy && 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(oldValue);
}
}

this.setNeedsUpdate(true, {startRow: instanceCount});
state.allocatedValue = this.value;
return true;
}

Expand All @@ -226,15 +218,10 @@ export default class Attribute extends BaseAttribute {

let updated = true;
if (update) {
const allocatedValue = !noAlloc && this.value;
// Custom updater - typically for non-instanced layers
for (const [startRow, endRow] of updateRanges) {
update.call(context, this, {data, startRow, endRow, props, numInstances, bufferLayout});
}
if (allocatedValue && allocatedValue !== this.value) {
// The update function did not use the allocated value
typedArrayManager.release(allocatedValue);
}
if (this.constant || !this.buffer || this.buffer.byteLength < this.value.byteLength) {
// call base clas `update` method to upload value to GPU
this.update({
Expand Down Expand Up @@ -435,9 +422,8 @@ export default class Attribute extends BaseAttribute {
_validateAttributeUpdaters() {
const state = this.userData;

// Check that either 'accessor' or 'update' is a valid function
const hasUpdater =
state.noAlloc || typeof state.update === 'function' || typeof state.accessor === 'string';
// Check that 'update' is a valid function
const hasUpdater = state.noAlloc || typeof state.update === 'function';
if (!hasUpdater) {
throw new Error(`Attribute ${this.id} missing update or accessor`);
}
Expand Down
8 changes: 5 additions & 3 deletions modules/layers/src/bitmap-layer/bitmap-layer.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,18 @@ export default class BitmapLayer extends Layer {
positions: {
size: 3,
update: this.calculatePositions,
value: new Float32Array(12)
value: new Float32Array(12),
noAlloc: true
},
positions64xyLow: {
size: 3,
update: this.calculatePositions64xyLow,
value: new Float32Array(12)
value: new Float32Array(12),
noAlloc: true
}
});

this.setState({numInstances: 4}); // 4 corners
this.setState({numInstances: 1});
}

updateState({props, oldProps, changeFlags}) {
Expand Down
51 changes: 51 additions & 0 deletions test/modules/core/lib/attribute.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ test('Attribute#constructor', t => {
t.ok(attribute.allocate, 'Attribute.allocate function available');
t.ok(attribute.update, 'Attribute.update function available');

t.throws(() => new Attribute(gl, {size: 1}), 'Attribute missing update option');

t.end();
});

Expand Down Expand Up @@ -79,6 +81,55 @@ test('Attribute#getUpdateTriggers', t => {
t.end();
});

test('Attribute#allocate', t => {
const attributeNoAlloc = new Attribute(gl, {
id: 'positions',
size: 3,
accessor: 'a',
noAlloc: true
});

const attribute = new Attribute(gl, {
id: 'sizes',
update: attr => {
attr.constant = true;
attr.value = new Float32Array(2);
},
size: 2
});

const externalValue = new Float32Array(20).fill(1);

t.notOk(attributeNoAlloc.allocate(2), 'Should not allocate if noAlloc is set');

t.ok(attribute.allocate(2), 'allocate successful');
const allocatedValue = attribute.value;
t.ok(allocatedValue.length >= 4, 'allocated value is large enough');

t.ok(attribute.allocate(4), 'allocate successful');
t.is(attribute.value, allocatedValue, 'reused the same typed array');

attribute.setExternalBuffer(externalValue);
t.notOk(attributeNoAlloc.allocate(4), 'Should not allocate if external buffer is used');

attribute.setExternalBuffer(null);
t.ok(attribute.allocate(4), 'allocate successful');
t.is(attribute.value, allocatedValue, 'reused the same typed array');

attribute.setGenericValue([1, 1]);
t.notOk(attributeNoAlloc.allocate(4), 'Should not allocate if constant value is used');

attribute.setGenericValue(undefined);
t.ok(attribute.allocate(4), 'allocate successful');
t.is(attribute.value, allocatedValue, 'reused the same typed array');

t.ok(attribute.allocate(8), 'allocate successful');
t.not(attribute.value, allocatedValue, 'created new typed array');

attribute.delete();
t.end();
});

test('Attribute#shaderAttributes', t => {
const update = () => {};

Expand Down

0 comments on commit 4de8312

Please sign in to comment.