Fix: flickery frames due to leaky event handling #36
Conversation
…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.
| @@ -168,12 +168,19 @@ export class RenderServer { | |||
| } | |||
| }; | |||
| this.clients.set(client, { | |||
| frame: renderFn(image, this.cache, hijack), | |||
There was a problem hiding this comment.
this was the bulk of the issue experienced by Skyler's work - renderFn() often returns a frame that is already finished - that means that event handlers (like the one that copies the webGL buffer to the screen) run before we even write this little object to our map of clients for the render server - this of course would be surprising for everyone!
| // 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) |
There was a problem hiding this comment.
the fix is to start the frame AFTER writing down important things like its private image buffer, size, etc.
| reportStatus({ status: 'cancelled' }); | ||
| }, | ||
| }; | ||
| doWorkOnQueue(interval, false) |
There was a problem hiding this comment.
here is where we do rendering work as fast as we can - by not waiting for our setInterval() function to do it for us. To help prevent confusing situations, I now pass false to the worker, indicating that all its lifecycle callbacks should happen later, on a promise
| @@ -225,6 +225,7 @@ export class AsyncDataCache<SemanticKey extends RecordKey, CacheKey extends Reco | |||
| workingSet: Record<SemanticKey, () => Promise<D>>, | |||
| use: (items: Record<SemanticKey, D>) => void, | |||
| toCacheKey: (semanticKey: SemanticKey) => CacheKey, | |||
| // TODO: consider removing taskFinished - it would be more simple to let the caller handle this in their use() function | |||
|
We can revisit this whole idea someday - but for now I think this is an ok way to go - its particularly nice because it should require no code changes for users of this package. |
What:
Skyler's integration prototype revealed some suspicious flickering when zooming around. This lead me to find a new instance of a bug I previously created and fixed in the BKP's RenderManager, in which the delivery of events to the system that manages frames was very confusing.
TLDR - a frame can finish before its even returned to the function trying to start it - this is nice for perf, but very confusing. This is still true, but the lifecycle events will be delivered in a more consistant order, and the helpful render-server now handles this situation better. Previously, it would start many frames all at once, because each frame was assumed to be the first.