-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(web): viewport reactivity, off-screen thumbhashes being rendered #15435
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
With these changes, I rarely see the thumbhash rendered. Instead, it mostly transitions from the placeholder box to the thumbnail almost instantly, giving the impression of flashing in. So, we do not see a nice transition anymore. Is there a way to add it in? |
I think it's a combination of the intersection observer adding a slight delay, and the fact that the thumbnail can load faster because of this change (less blocking JS code). Should the thumbnail have a fade-in? |
Let's try that and see how it behaves |
Hello, I just ran another testing around. One difference I notice with On In this PR, I believe the timeline is loading when you scroll, leading to some janks when dragging the scroll thumb |
That's not the behavior I see in my prod instance. Scrubbing the timeline loads the buckets and thumbnails even if I'm moving it and never let go. |
Description
Changing the viewport results in a lot of repeated work because of the
viewport
andsafeViewport
variables being$state
objects andupdateViewport
separately triggering another update. This was exacerbated by the fact that thumbhashes were processed eagerly without checking if they're intersected, meaning that larger buckets could have hundreds of thousands of thumbhashes processed as a user changes the viewport.This PR makes three primary changes:
updateViewport
.updateViewport
's throttling is changed to debouncing and can be automatically shifted to a "large bucket" mode with more aggressive debouncing settings. The default trigger to enable this mode is to have more than 3000 assets in a single bucket. This effectively means that instead of having the layout react as you're dragging, it will react once you stop moving it.The result is that both initial page load, changing viewport and moving around in the timeline is more responsive than before. There is a caveat in that the initial page load is more likely to display gray boxes initially instead of the thumbhash because of the intersection check. Scrolling displays the thumbhash as normal, though.
This was primarily tested alongside the justified layout changes (will be a separate PR), but I split it out into its own PR.
How Has This Been Tested?
Tested with average bucket sizes of 200, 600, 1000, 2000, 3500 and 30000 and confirmed that it (more or less) held up at those numbers, 30000 still being quite slow but usable.
Edit: in the current iteration of this PR, small date groups (<=100 assets) will continue to have thumbhashes eagerly processed so the thumbhashes appear in the first frame. At this scale, it does not seem to cause major issues besides high RAM usage, although it is less responsive than the original version of this PR. I think getting the best of both worlds would require deeper changes, like having a single larger canvas for the date group that gets updated instead of many small canvases, or possibly using WebGL etc.