From 0e3f6759748c219206dbad797dac1fcf2714ff4b Mon Sep 17 00:00:00 2001 From: noah Date: Tue, 12 Nov 2024 11:07:39 -0800 Subject: [PATCH] Fix: flickery frames due to leaky event handling (#36) --- packages/scatterbrain/package.json | 4 +-- .../scatterbrain/src/abstract/async-frame.ts | 36 ++++++++----------- .../src/abstract/render-server.ts | 9 ++++- packages/scatterbrain/src/dataset-cache.ts | 1 + 4 files changed, 26 insertions(+), 24 deletions(-) diff --git a/packages/scatterbrain/package.json b/packages/scatterbrain/package.json index 4486ad7..07935cd 100644 --- a/packages/scatterbrain/package.json +++ b/packages/scatterbrain/package.json @@ -1,6 +1,6 @@ { "name": "@alleninstitute/vis-scatterbrain", - "version": "0.0.4", + "version": "0.0.5", "contributors": [ { "name": "Lane Sawyer", @@ -58,4 +58,4 @@ "lodash": "^4.17.21", "regl": "^2.1.0" } -} +} \ No newline at end of file diff --git a/packages/scatterbrain/src/abstract/async-frame.ts b/packages/scatterbrain/src/abstract/async-frame.ts index 2879831..a2e0eb3 100644 --- a/packages/scatterbrain/src/abstract/async-frame.ts +++ b/packages/scatterbrain/src/abstract/async-frame.ts @@ -92,7 +92,7 @@ export function beginFrame< renderItem(itemToRender, dataset, settings, maybe); } }; - const reportStatus = (event: AsyncFrameEvent, synchronous: boolean = false) => { + const reportStatus = (event: AsyncFrameEvent, synchronous: boolean) => { // we want to report our status, however the flow of events can be confusing - // our callers anticipate an asynchronous (long running) frame to be started, // but there are scenarios in which the whole thing is completely synchronous @@ -102,11 +102,11 @@ export function beginFrame< if (synchronous) { lifecycleCallback(event); } else { - Promise.resolve().then(() => lifecycleCallback(event)); + Promise.resolve().then(() => lifecycleCallback(event)) } }; - const doWorkOnQueue = (intervalId: number) => { + const doWorkOnQueue = (intervalId: number, synchronous: boolean = false) => { // try our best to cleanup if something goes awry const startWorkTime = performance.now(); const cleanupOnError = (err: unknown) => { @@ -117,7 +117,7 @@ export function beginFrame< abort.abort(err); clearInterval(intervalId); // pass the error somewhere better: - reportStatus({ status: 'error', error: err }, true); + reportStatus({ status: 'error', error: err }, synchronous); }; while (mutableCache.getNumPendingTasks() < Math.max(maximumInflightAsyncTasks, 1)) { // We know there are items in the queue because of the check above, so we assert the type exist @@ -131,7 +131,7 @@ export function beginFrame< requestsForItem(itemToRender, dataset, settings, abort.signal), partial(fancy, itemToRender), toCacheKey, - () => reportStatus({ status: 'progress', dataset, renderedItems: [itemToRender] }, true) + () => reportStatus({ status: 'progress', dataset, renderedItems: [itemToRender] }, synchronous) ); if (result !== undefined) { // put this cancel callback in a list where we can invoke if something goes wrong @@ -152,30 +152,24 @@ export function beginFrame< if (mutableCache.getNumPendingTasks() < 1) { // we do want to wait for that last in-flight task to actually finish though: clearInterval(intervalId); - reportStatus({ status: 'finished' }); + reportStatus({ status: 'finished' }, synchronous); } return; } }; - const interval = setInterval(() => doWorkOnQueue(interval), queueProcessingIntervalMS); - // do some work right now... + reportStatus({ status: 'begin' }, true) + const interval = setInterval(() => doWorkOnQueue(interval), queueProcessingIntervalMS); if (queue.length > 0) { - reportStatus({ status: 'begin' }, true); - doWorkOnQueue(interval); - // return a function to allow our caller to cancel the frame - guaranteed that no settings/data will be - // touched/referenced after cancellation, unless the author of render() did some super weird bad things - return { - cancelFrame: (reason?: string) => { - taskCancelCallbacks.forEach((cancelMe) => cancelMe()); - abort.abort(new DOMException(reason, 'AbortError')); - clearInterval(interval); - reportStatus({ status: 'cancelled' }); - }, - }; + doWorkOnQueue(interval, false) } return { - cancelFrame: () => {}, + cancelFrame: (reason?: string) => { + taskCancelCallbacks.forEach((cancelMe) => cancelMe()); + abort.abort(new DOMException(reason, 'AbortError')); + clearInterval(interval); + reportStatus({ status: 'cancelled' }, true); + } }; } diff --git a/packages/scatterbrain/src/abstract/render-server.ts b/packages/scatterbrain/src/abstract/render-server.ts index 0f1acda..664b538 100644 --- a/packages/scatterbrain/src/abstract/render-server.ts +++ b/packages/scatterbrain/src/abstract/render-server.ts @@ -168,12 +168,19 @@ export class RenderServer { } }; this.clients.set(client, { - frame: renderFn(image, this.cache, hijack), + frame: null, image, copyBuffer, resolution, updateRequested: null, }); + // this is worded rather awkwardly, because sometimes the frameLifecycle object returned by renderFn() represents + // a frame that is already finished! + // this is a good thing for performance, but potentially confusing - so we do our book-keeping before we actually start rendering: + const aboutToStart = this.clients.get(client); // this is the record we just put into the clients map - TS just wants to be sure it really exists: + if (aboutToStart) { + aboutToStart.frame = renderFn(image, this.cache, hijack) + } } } } diff --git a/packages/scatterbrain/src/dataset-cache.ts b/packages/scatterbrain/src/dataset-cache.ts index 6cab4c8..8ff6cbb 100644 --- a/packages/scatterbrain/src/dataset-cache.ts +++ b/packages/scatterbrain/src/dataset-cache.ts @@ -225,6 +225,7 @@ export class AsyncDataCache Promise>, use: (items: Record) => void, toCacheKey: (semanticKey: SemanticKey) => CacheKey, + // TODO: consider removing taskFinished - it would be more simple to let the caller handle this in their use() function taskFinished?: () => void ): cancelFn | undefined { const keys: SemanticKey[] = Object.keys(workingSet) as SemanticKey[];