Skip to content
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: [Zoom] Performance improvements (especially w/ trackpad zoom) #3396

Merged
merged 4 commits into from
Dec 16, 2023

Conversation

ffxsam
Copy link
Contributor

@ffxsam ffxsam commented Dec 11, 2023

Short description

Zooming with a trackpad was causing huge CPU spikes due to constant redraws. This PR imposes rate limiting via a delta threshold before actually redrawing the waveform.

Implementation details

Debounce & throttle were not appropriate here, as we don't necessarily want to throttle the events, we just want to ignore the "in-between" delta values and have a more notched zoom (a la Adobe Audition).

How to test it

Test using any trackpad with smooth scrolling (i.e. lots of event firing when scrolling).

Checklist

  • This PR is covered by e2e tests
  • [?] It introduces no breaking API changes

@ffxsam
Copy link
Contributor Author

ffxsam commented Dec 11, 2023

Still a work-in-progress. Standard mouse wheeling is a bit wonky now (doesn't always zoom). Delta accumulation logic needs tweaks.

Also, unsure if this is a breaking API change. IMO, the default behavior should be the zoom detents (deltaThreshold of 10). Users can have smooth zooming by setting this to 0.

Open to any ideas, changes, etc. This is just a start!

@ffxsam
Copy link
Contributor Author

ffxsam commented Dec 11, 2023

This might help explain why throttling wasn't the right approach:

In Audition, you can scroll as quickly as you want, but it's a stepped zoom rather than a fluid one. Also, maybe a deltaThreshold setting is too in-the-weeds, and we just need a simpler stepZoom boolean.

Audition.mp4

}

// Reset the accumulated delta
this.accumulatedDelta %= this.options.deltaThreshold
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it can probably be just set to 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna rework some of this. I'll mark the PR as ready for review once I think it's in a good spot.

@ffxsam
Copy link
Contributor Author

ffxsam commented Dec 11, 2023

Ok, just setting it to 0 seems to have fixed ignored mouse wheel movements.

The thing is, all mice are different, and some of them may have smaller delta jumps with each wheel notch. Tested on a Logitech Lift and a Logitech MX Master 3S (with smooth wheel enabled). Both had similar deltas (see below), but that's likely because they're both Logitech. 😉 In other words, some mice might have to scroll two notches before the zoom method is called. Not sure if this is a big deal though.

CleanShot 2023-12-11 at 11 47 50

@ffxsam ffxsam marked this pull request as ready for review December 11, 2023 17:50
src/plugins/zoom.ts Outdated Show resolved Hide resolved
Copy link
Owner

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍

@ffxsam
Copy link
Contributor Author

ffxsam commented Dec 12, 2023

Sure, thanks for the review!

Thoughts on the option itself? deltaThreshold provides more control for people to tweak it. Or we could just figure a value of 10 is good, and have stepZoom: boolean and default it to true.

@katspaugh
Copy link
Owner

katspaugh commented Dec 12, 2023

So to clarify, you ideally want the waveform to re-render every time the mouse wheel clicks, but we cannot count clicks, only a (pixel?) delta?

Looking at this MDN article, it looks like we can only see what mode the mouse wheel is in (scrolling per page, per line or per pixel).

And of course touchpads and trackballs don't have notches at all...

So I think 10 is a sensible default, especially if your two mice are around that ballbark.

@HoodyHuo @cmorbitzer good with you?

@ffxsam
Copy link
Contributor Author

ffxsam commented Dec 12, 2023

Right. Too many re-renders = bad news for those on lighter-weight computers. Step zoom is just the safer route.

Even when I compare Adobe Audition vs. iZotope RX with trackpad zoom, RX has smooth zoom, and my CPU usage jumps to 150-200+. With Audition (step zoom), my computer doesn't even get its heart rate going. 😁

@cmorbitzer
Copy link
Contributor

LGTM

I don't have an opinion on performance, but 10 felt choppy to me as a user, where 5 felt seamless enough. Do we get much of the same resource benefit by setting the threshold to 5?

@ffxsam
Copy link
Contributor Author

ffxsam commented Dec 12, 2023

A setting of 5 had more spikes for me (on a Mac Studio), but still better than totally smooth zoom. Maybe we default delta threshold to 5? I can test in a bit, and I'd like to update the docs a bit to explain this setting better.

@ffxsam
Copy link
Contributor Author

ffxsam commented Dec 12, 2023

Yep, 5 is good. Not as good as 10, but a good middle ground. Just updated the code, and I think the description for this setting will do.

@katspaugh katspaugh merged commit bd4fe41 into katspaugh:main Dec 16, 2023
2 checks passed
@katspaugh
Copy link
Owner

Thanks!

@ffxsam ffxsam deleted the fix/trackpad-zoom-perf branch December 21, 2023 15:09
@bernardwiesner
Copy link

@ffxsam Even setting the deltaThreshold to 50, when I scroll quickly on a big waveform, page suddenly crashes.

Could we not use a throttling instead? I mean user scrolls very quickly, but we only re-draw once after the scroll is over. It would not be as fluid but more stable.

@ffxsam
Copy link
Contributor Author

ffxsam commented Apr 3, 2024

@bernardwiesner Hey Bernard! I'm honestly not sure. If we used throttle, that would mean that scroll events would have no visible effect until the user stopped scrolling, which is not ideal.

If you're experiencing any kind of bug or performance issue, please feel free to open a new issue.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants