search

atlas-viewer / atlas

Main repository for Atlas viewer
https://atlas-viewer-storybook.netlify.app/
MIT License
4 stars 3 forks source link

Canvas opacity not updating with slow loading images #49

Open danieltbrennan opened 2 months ago

danieltbrennan commented 2 months ago

Noting a bug that we've encountered when working with an image service that is a bit slower to load than is typical. It seems that a scenario can arise where none of the conditions necessary to update the opacity of the canvas from 0 to 1 in pendingUpdate will ever be met.

https://github.com/atlas-viewer/atlas/blob/caa79a75182a95d12fdc43b5000b25bb6ac96bc5/src/modules/canvas-renderer/canvas-renderer.ts#L810-L826

Best as I can tell the cause of this is that when images are loading slowly schedulePaintToCanvas is incrementing imagesLoaded multiple times, meaning that this line can result in a negative value for imagesPending

https://github.com/atlas-viewer/atlas/blob/caa79a75182a95d12fdc43b5000b25bb6ac96bc5/src/modules/canvas-renderer/canvas-renderer.ts#L173

Something as simple as checking for this.imagesPending - this.imagesLoaded < 0 or even just a timeout on the opacity update fixes this in terms of setting the canvas visibility. Happy to set up a PR with a small fix like that but feels like it may also be a symptom of an issue elsewhere. So far the easiest way to replicate/test this has been to use in-browser network throttling at a low speed with the current stories in the repo.

stephenwf commented 2 months ago

Ahh because if (!this.imagesPending) will be true if it's negative. I'll have a look at replicating. I'll have a look soon to see if I can figure out when it's incrementing imagesLoaded multiple times.

danieltbrennan commented 2 months ago

@stephenwf I looked a bit wasn't able to make any clear conclusions about the exact cause of the repeated calls but in talking about this with @abrin we had an idea that we might be able to use the image id as a key to effectively dedupe them (or at least prevent the extra incrementing of imagesLoaded).

We're currently actively working on a new canvas panel implementation in one of our apps where this issue is causing some challenges from a UX perspective, so anything we can do to work around it even if as a temporary measure would be a great help.

stephenwf commented 2 months ago

If the fix in #50 is enough to unblock, I can get that merged and tagged for you 👍 alternatively, I can disable the fade-in (again!)