diff --git a/CHANGELOG.md b/CHANGELOG.md index f820938..e9d96c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +## 1.12.1 + +- Fix: destroy the encoding texture before recreating it +- Fix: reject `set()` calls if the instance was destroyed +- Fix: ensures unnecessary color and encoding texture updates are avoided +- Fix: color and encoding textures are destroyed upon calling `scatterplot.destroy()` +- Fix: prevent a minor memory leak in the newly added advanced exporter + ## 1.12.0 - Feat: add support for adjusting the anti-aliasing via `scatterplot.set({ antiAliasing: 1 })`. ([#175](https://github.com/flekschas/regl-scatterplot/issues/175)) diff --git a/src/constants.js b/src/constants.js index fe3e1fc..351f2b3 100644 --- a/src/constants.js +++ b/src/constants.js @@ -172,3 +172,6 @@ export const DEFAULT_PIXEL_ALIGNED = false; // Error messages export const ERROR_POINTS_NOT_DRAWN = 'Points have not been drawn'; +export const ERROR_INSTANCE_IS_DESTROYED = 'The instance was already destroyed'; +export const ERROR_IS_DRAWING = + 'Ignoring draw call as the previous draw call has not yet finished. To avoid this warning `await` the draw call.'; diff --git a/src/index.js b/src/index.js index 228e30d..b505567 100644 --- a/src/index.js +++ b/src/index.js @@ -99,6 +99,8 @@ import { DEFAULT_VIEW, DEFAULT_WIDTH, EASING_FNS, + ERROR_INSTANCE_IS_DESTROYED, + ERROR_IS_DRAWING, ERROR_POINTS_NOT_DRAWN, FLOAT_BYTES, KEYS, @@ -143,6 +145,8 @@ import { isPolygon, isPositiveNumber, isRect, + isSameElements, + isSameRgbas, isStrictlyPositiveNumber, isString, isValidBBox, @@ -319,6 +323,7 @@ const createScatterplot = ( lassoColor = toRgba(lassoColor, true); reticleColor = toRgba(reticleColor, true); + let isDrawing = false; let isDestroyed = false; let backgroundColorBrightness = rgbBrightness(backgroundColor); let camera; @@ -1231,6 +1236,11 @@ const createScatterplot = ( let tmpColors = isMultipleColors(newColors) ? newColors : [newColors]; tmpColors = tmpColors.map((color) => toRgba(color, true)); + if (isSameRgbas(prevColors, tmpColors)) { + // We don't need to update the color texture so we return early + return; + } + if (colorTex) { colorTex.destroy(); } @@ -1343,12 +1353,21 @@ const createScatterplot = ( }; const setPointSize = (newPointSize) => { + const oldPointSize = Array.isArray(pointSize) ? [...pointSize] : pointSize; + if (isConditionalArray(newPointSize, isPositiveNumber, { minLength: 1 })) { pointSize = [...newPointSize]; + } else if (isStrictlyPositiveNumber(+newPointSize)) { + pointSize = [+newPointSize]; } - if (isStrictlyPositiveNumber(+newPointSize)) { - pointSize = [+newPointSize]; + if (oldPointSize === pointSize || isSameElements(oldPointSize, pointSize)) { + // We don't need to update the encoding texture so we return early + return; + } + + if (encodingTex) { + encodingTex.destroy(); } minPointScale = MIN_POINT_SIZE / pointSize[0]; @@ -1401,12 +1420,21 @@ const createScatterplot = ( }; const setOpacity = (newOpacity) => { + const oldOpacity = Array.isArray(opacity) ? [...opacity] : opacity; + if (isConditionalArray(newOpacity, isPositiveNumber, { minLength: 1 })) { opacity = [...newOpacity]; + } else if (isStrictlyPositiveNumber(+newOpacity)) { + opacity = [+newOpacity]; } - if (isStrictlyPositiveNumber(+newOpacity)) { - opacity = [+newOpacity]; + if (oldOpacity === opacity || isSameElements(oldOpacity, opacity)) { + // We don't need to update the encoding texture so we return early + return; + } + + if (encodingTex) { + encodingTex.destroy(); } encodingTex = createEncodingTexture(); @@ -2397,144 +2425,149 @@ const createScatterplot = ( */ const publicDraw = (newPoints, options = {}) => { if (isDestroyed) { - return Promise.reject(new Error('The instance was already destroyed')); - } - return toArrayOrientedPoints(newPoints).then( - (newPointsArray) => - new Promise((resolve) => { - if (isDestroyed) { - // In the special case where the instance was destroyed after - // scatterplot.draw() was called but before toArrayOrientedPoints() - // resolved, we will _not_ reject the promise as this would be - // confusing. Instead we will immediately resolve and return. - resolve(); - return; - } + return Promise.reject(new Error(ERROR_INSTANCE_IS_DESTROYED)); + } + if (isDrawing) { + return Promise.reject(new Error(ERROR_IS_DRAWING)); + } + isDrawing = true; + return toArrayOrientedPoints(newPoints).then((newPointsArray) => + new Promise((resolve) => { + if (isDestroyed) { + // In the special case where the instance was destroyed after + // scatterplot.draw() was called but before toArrayOrientedPoints() + // resolved, we will _not_ reject the promise as this would be + // confusing. Instead we will immediately resolve and return. + resolve(); + return; + } - let pointsCached = false; + let pointsCached = false; - if ( - !options.preventFilterReset || - newPointsArray?.length !== numPoints - ) { - isPointsFiltered = false; - filteredPointsSet.clear(); - } + if ( + !options.preventFilterReset || + newPointsArray?.length !== numPoints + ) { + isPointsFiltered = false; + filteredPointsSet.clear(); + } - const drawPointConnections = - newPointsArray && - hasPointConnections(newPointsArray[0]) && - (showPointConnections || options.showPointConnectionsOnce); - - const { zDataType, wDataType } = options; - - new Promise((resolveDraw) => { - if (newPointsArray) { - if (options.transition) { - if (newPointsArray.length === numPoints) { - pointsCached = cachePoints(newPointsArray, { - z: zDataType, - w: wDataType, - }); - } else { - // biome-ignore lint/suspicious/noConsole: This is a legitimately useful warning - console.warn( - 'Cannot transition! The number of points between the previous and current draw call must be identical.', - ); - } + const drawPointConnections = + newPointsArray && + hasPointConnections(newPointsArray[0]) && + (showPointConnections || options.showPointConnectionsOnce); + + const { zDataType, wDataType } = options; + + new Promise((resolveDraw) => { + if (newPointsArray) { + if (options.transition) { + if (newPointsArray.length === numPoints) { + pointsCached = cachePoints(newPointsArray, { + z: zDataType, + w: wDataType, + }); + } else { + // biome-ignore lint/suspicious/noConsole: This is a legitimately useful warning + console.warn( + 'Cannot transition! The number of points between the previous and current draw call must be identical.', + ); } - - setPoints(newPointsArray, { - zDataType, - wDataType, - preventFilterReset: options.preventFilterReset, - spatialIndex: options.spatialIndex, - }).then(() => { - if (options.hover !== undefined) { - hover(options.hover, { preventEvent: true }); - } - - if (options.select !== undefined) { - select(options.select, { preventEvent: true }); - } - - if (options.filter !== undefined) { - filter(options.filter, { preventEvent: true }); - } - - if (drawPointConnections) { - setPointConnections(newPointsArray) - .then(() => { - pubSub.publish('pointConnectionsDraw'); - draw = true; - drawReticleOnce = options.showReticleOnce; - }) - .then(resolve); - } else { - resolveDraw(); - } - }); - } else { - resolveDraw(); } - }).then(() => { - if (options.transition && pointsCached) { + + setPoints(newPointsArray, { + zDataType, + wDataType, + preventFilterReset: options.preventFilterReset, + spatialIndex: options.spatialIndex, + }).then(() => { + if (options.hover !== undefined) { + hover(options.hover, { preventEvent: true }); + } + + if (options.select !== undefined) { + select(options.select, { preventEvent: true }); + } + + if (options.filter !== undefined) { + filter(options.filter, { preventEvent: true }); + } + if (drawPointConnections) { - Promise.all([ - new Promise((resolveTransition) => { - pubSub.subscribe( - 'transitionEnd', - () => { - // Point connects cannot be transitioned yet so we hide them during - // the transition. Hence, we need to make sure we call `draw()` once - // the transition has ended. - draw = true; - drawReticleOnce = options.showReticleOnce; - resolveTransition(); - }, - 1, - ); - }), - new Promise((resolveDraw) => { - pubSub.subscribe('pointConnectionsDraw', resolveDraw, 1); - }), - ]).then(resolve); - } else { - pubSub.subscribe( - 'transitionEnd', - () => { - // Point connects cannot be transitioned yet so we hide them during - // the transition. Hence, we need to make sure we call `draw()` once - // the transition has ended. + setPointConnections(newPointsArray) + .then(() => { + pubSub.publish('pointConnectionsDraw'); draw = true; drawReticleOnce = options.showReticleOnce; - resolve(); - }, - 1, - ); - } - startTransition({ - duration: options.transitionDuration, - easing: options.transitionEasing, - }); - } else { - if (drawPointConnections) { - Promise.all([ - new Promise((resolveDraw) => { - pubSub.subscribe('draw', resolveDraw, 1); - }), - new Promise((resolveDraw) => { - pubSub.subscribe('pointConnectionsDraw', resolveDraw, 1); - }), - ]).then(resolve); + }) + .then(() => resolve()); } else { - pubSub.subscribe('draw', resolve, 1); + resolveDraw(); } - draw = true; - drawReticleOnce = options.showReticleOnce; + }); + } else { + resolveDraw(); + } + }).then(() => { + if (options.transition && pointsCached) { + if (drawPointConnections) { + Promise.all([ + new Promise((resolveTransition) => { + pubSub.subscribe( + 'transitionEnd', + () => { + // Point connects cannot be transitioned yet so we hide them during + // the transition. Hence, we need to make sure we call `draw()` once + // the transition has ended. + draw = true; + drawReticleOnce = options.showReticleOnce; + resolveTransition(); + }, + 1, + ); + }), + new Promise((resolveDraw) => { + pubSub.subscribe('pointConnectionsDraw', resolveDraw, 1); + }), + ]).then(() => resolve()); + } else { + pubSub.subscribe( + 'transitionEnd', + () => { + // Point connects cannot be transitioned yet so we hide them during + // the transition. Hence, we need to make sure we call `draw()` once + // the transition has ended. + draw = true; + drawReticleOnce = options.showReticleOnce; + resolve(); + }, + 1, + ); } - }); - }), + startTransition({ + duration: options.transitionDuration, + easing: options.transitionEasing, + }); + } else { + if (drawPointConnections) { + Promise.all([ + new Promise((resolveDraw) => { + pubSub.subscribe('draw', resolveDraw, 1); + }), + new Promise((resolveDraw) => { + pubSub.subscribe('pointConnectionsDraw', resolveDraw, 1); + }), + ]).then(() => resolve()); + } else { + pubSub.subscribe('draw', () => resolve(), 1); + } + draw = true; + drawReticleOnce = options.showReticleOnce; + } + }); + }).finally(() => { + isDrawing = false; + }), ); }; @@ -2545,7 +2578,7 @@ const createScatterplot = ( */ const drawAnnotations = (newAnnotations) => { if (isDestroyed) { - return Promise.reject(new Error('The instance was already destroyed')); + return Promise.reject(new ERROR_INSTANCE_IS_DESTROYED()); } isAnnotationsDrawn = false; @@ -3590,6 +3623,10 @@ const createScatterplot = ( return isDestroyed; } + if (property === 'isDrawing') { + return isDrawing; + } + if (property === 'isPointsDrawn') { return isPointsDrawn; } @@ -3639,6 +3676,10 @@ const createScatterplot = ( /** @type {(properties: Partial) => Promise} */ const set = (properties = {}) => { + if (isDestroyed) { + return Promise.reject(new Error(ERROR_INSTANCE_IS_DESTROYED)); + } + checkDeprecations(properties); if ( @@ -3917,14 +3958,14 @@ const createScatterplot = ( // all calls async. return new Promise((resolve) => { window.requestAnimationFrame(() => { - if (!canvas) { + if (isDestroyed || !canvas) { // Instance was destroyed in between return; } updateViewAspectRatio(); camera.refresh(); renderer.refresh(); - draw = true; + redraw(); resolve(); }); }); @@ -4096,7 +4137,7 @@ const createScatterplot = ( renderer.refresh(); await new Promise((resolve) => { - pubSub.subscribe('draw', resolve); + pubSub.subscribe('draw', resolve, 1); redraw(); }); @@ -4114,7 +4155,7 @@ const createScatterplot = ( setAntiAliasing(currAntiAliasing); await new Promise((resolve) => { - pubSub.subscribe('draw', resolve); + pubSub.subscribe('draw', resolve, 1); redraw(); }); @@ -4368,6 +4409,12 @@ const createScatterplot = ( pointConnections.destroy(); reticleHLine.destroy(); reticleVLine.destroy(); + if (colorTex) { + colorTex.destroy(); + } + if (encodingTex) { + encodingTex.destroy(); + } if (!(initialProperties.renderer || renderer.isDestroyed)) { // Since the user did not pass in an externally created renderer we can // assume that the renderer is only used by this scatter plot instance. diff --git a/src/utils.js b/src/utils.js index abd0c60..333e02d 100644 --- a/src/utils.js +++ b/src/utils.js @@ -311,6 +311,23 @@ export const isMultipleColors = (color) => color.length > 0 && (Array.isArray(color[0]) || isString(color[0])); +/** + * Test if two arrays contain the same primitive values + */ +export const isSameElements = (a, b) => + Array.isArray(a) && Array.isArray(b) && a.every((value, i) => value === b[i]); + +/** + * Test if two arrays contain the same RGBA quadruples + */ +export const isSameRgbas = (a, b) => + Array.isArray(a) && + Array.isArray(b) && + a.every(([r1, g1, b1, a1], i) => { + const [r2, g2, b2, a2] = b[i]; + return r1 === r2 && g1 === g2 && b1 === b2 && a1 === a2; + }); + /** * Fast version of `Math.max`. Based on * https://jsperf.com/math-min-max-vs-ternary-vs-if/24 `Math.max` is not diff --git a/tests/events.test.js b/tests/events.test.js index 0e20c67..d267c46 100644 --- a/tests/events.test.js +++ b/tests/events.test.js @@ -23,7 +23,6 @@ import { createKeyboardEvent, wait, capitalize, - catchError, } from './utils'; test('init and destroy events', async () => { @@ -35,19 +34,17 @@ test('init and destroy events', async () => { }); const whenInit = new Promise((resolve) => { - scatterplot.subscribe('init', resolve, 1); + scatterplot.subscribe('init', () => resolve(true), 1); }); const whenDestroy = new Promise((resolve) => { - scatterplot.subscribe('destroy', resolve, 1); + scatterplot.subscribe('destroy', () => resolve(true), 1); }); - await whenInit; + await expect(whenInit).resolves.toBe(true); scatterplot.destroy(); - await whenDestroy; - - expect(true).toBe(true); + await expect(whenDestroy).resolves.toBe(true); }); test('throw an error when calling draw() after destroy()', async () => { diff --git a/tests/get-set.test.js b/tests/get-set.test.js index f507d14..b5bbfc9 100644 --- a/tests/get-set.test.js +++ b/tests/get-set.test.js @@ -22,6 +22,7 @@ import { DEFAULT_POINT_CONNECTION_SIZE, DEFAULT_POINT_CONNECTION_SIZE_ACTIVE, DEFAULT_IMAGE_LOAD_TIMEOUT, + ERROR_INSTANCE_IS_DESTROYED, IMAGE_LOAD_ERROR, } from '../src/constants'; @@ -648,8 +649,6 @@ test('set({ cameraIsFixed })', async () => { await nextAnimationFrame(); - console.log('1. camera distance', scatterplot.get('camera').distance[0]); - // We expect the distance to be less than one because we zoomed into the plot // via wheeling expect(scatterplot.get('camera').distance[0]).toBeLessThan(1); @@ -695,3 +694,47 @@ test('set({ cameraIsFixed })', async () => { scatterplot.destroy(); }); + +test('get("isDestroyed")', async () => { + const scatterplot = createScatterplot({ canvas: createCanvas() }); + + expect(scatterplot.get('isDestroyed')).toBe(false); + + scatterplot.destroy(); + + expect(scatterplot.get('isDestroyed')).toBe(true); +}); + +test('get("isDrawing")', async () => { + const canvas = createCanvas(); + const scatterplot = createScatterplot({ canvas }); + + expect(scatterplot.get('isDrawing')).toBe(false); + + const whenDrawn = scatterplot.draw([ + [-1, 1], + [1, 1], + [0, 0], + [-1, -1], + [1, -1], + ]); + + expect(scatterplot.get('isDrawing')).toBe(true); + + await expect(whenDrawn).resolves.toBe(undefined); + + expect(scatterplot.get('isDrawing')).toBe(false); + + scatterplot.destroy(); +}); + + +test('set() after destroy', async () => { + const scatterplot = createScatterplot({ canvas: createCanvas() }); + + scatterplot.destroy(); + + const whenSet = scatterplot.set({ mouseMode: 'lasso' }); + + await expect(whenSet).rejects.toThrow(ERROR_INSTANCE_IS_DESTROYED); +}); diff --git a/tests/methods.test.js b/tests/methods.test.js index b3e020f..a2ab51e 100644 --- a/tests/methods.test.js +++ b/tests/methods.test.js @@ -6,6 +6,8 @@ import createScatterplot from '../src'; import { DEFAULT_POINT_SCALE_MODE, + ERROR_INSTANCE_IS_DESTROYED, + ERROR_IS_DRAWING, ERROR_POINTS_NOT_DRAWN, } from '../src/constants'; @@ -124,6 +126,34 @@ test('draw() with preventFilterReset', async () => { scatterplot.destroy(); }); +test('draw() prematurely', async () => { + const canvas = createCanvas(); + const scatterplot = createScatterplot({ canvas }); + + const whenDrawn1 = scatterplot.draw([[-1, 1], [0, 0], [1, -1]]); + const whenDrawn2 = scatterplot.draw([[-1, 1], [0, 0], [1, -1]]); + await expect(whenDrawn2).rejects.toThrow(ERROR_IS_DRAWING); + + await expect(whenDrawn1).resolves.toBe(undefined); + + const whenDrawn3 = scatterplot.draw([[-1, 1], [0, 0], [1, -1]]); + + await expect(whenDrawn3).resolves.toBe(undefined); + + scatterplot.destroy(); +}); + +test('draw() after destroy', async () => { + const canvas = createCanvas(); + const scatterplot = createScatterplot({ canvas }); + + scatterplot.destroy(); + + const whenDrawn = scatterplot.draw([[-1, 1], [0, 0], [1, -1]]); + + await expect(whenDrawn).rejects.toThrow(ERROR_INSTANCE_IS_DESTROYED); +}); + test('select()', async () => { const scatterplot = createScatterplot({ canvas: createCanvas() }); diff --git a/tests/utils.js b/tests/utils.js index f37ec62..e547d70 100644 --- a/tests/utils.js +++ b/tests/utils.js @@ -57,14 +57,6 @@ export const wait = (milliSeconds) => export const capitalize = (s) => `${s[0].toUpperCase}${s.slice(1)}`; -export const catchError = (testFn) => async (t) => { - try { - await testFn(t); - } catch (e) { - t.fail(e.message); - } -}; - export const isSameElements = (a, b) => { if (a.length !== b.length) return false; const aSet = new Set(a);