From d12bc77cb689bb7324c01cf36d881081e41273ba Mon Sep 17 00:00:00 2001 From: noah Date: Mon, 11 Nov 2024 15:17:26 -0800 Subject: [PATCH 1/2] update to v.0.0.5 - async frames would report frame lifecycle events in a confusing way, due to the asynchronous nature of the events vs. the desire to synchronously render whenever possible, it was common for frames to finish before even being returned from the begin() function, leading to confusing behavior. --- packages/scatterbrain/package.json | 4 +-- .../scatterbrain/src/abstract/async-frame.ts | 36 ++++++++----------- .../src/abstract/render-server.ts | 11 ++++-- packages/scatterbrain/src/dataset-cache.ts | 1 + 4 files changed, 27 insertions(+), 25 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..0917199 100644 --- a/packages/scatterbrain/src/abstract/render-server.ts +++ b/packages/scatterbrain/src/abstract/render-server.ts @@ -2,7 +2,7 @@ import { AsyncDataCache } from '../dataset-cache'; import type { ReglCacheEntry } from './types'; import { Vec2, type vec2 } from '@alleninstitute/vis-geometry'; import REGL from 'regl'; -import { type AsyncFrameEvent, type RenderCallback } from './async-frame'; +import { beginFrame, type AsyncFrameEvent, type RenderCallback } from './async-frame'; import { type FrameLifecycle } from '../render-queue'; function destroyer(item: ReglCacheEntry) { @@ -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[]; From 56ee21d9a3c57d187f81c3a9bdd6d9ec7ea8b7dd Mon Sep 17 00:00:00 2001 From: noah Date: Mon, 11 Nov 2024 15:34:11 -0800 Subject: [PATCH 2/2] delete unused import --- packages/scatterbrain/src/abstract/render-server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/scatterbrain/src/abstract/render-server.ts b/packages/scatterbrain/src/abstract/render-server.ts index 0917199..664b538 100644 --- a/packages/scatterbrain/src/abstract/render-server.ts +++ b/packages/scatterbrain/src/abstract/render-server.ts @@ -2,7 +2,7 @@ import { AsyncDataCache } from '../dataset-cache'; import type { ReglCacheEntry } from './types'; import { Vec2, type vec2 } from '@alleninstitute/vis-geometry'; import REGL from 'regl'; -import { beginFrame, type AsyncFrameEvent, type RenderCallback } from './async-frame'; +import { type AsyncFrameEvent, type RenderCallback } from './async-frame'; import { type FrameLifecycle } from '../render-queue'; function destroyer(item: ReglCacheEntry) {