fix(render.js): zoom handler event affects animations

[Brostafa]

PR Checklist

Please check all that apply to this PR using "x":

• I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
• I have checked that this PR does not introduce a breaking change
• This PR introduces breaking changes and I have provided a detailed explanation below

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting)
• Refactoring (no functional changes)
• Documentation changes
• Other - Please describe:

Fixes

Issue Number: #66

What is the current behavior?

The zoom handler function makes the zoom animations choppy and slow.

What is the new behavior?

The zoom handler function animates smoothly the same way as using mouse wheel

Other information

It was just a problem with passing immediately = true parameter to OpenSeaDragon library.

[btzr-io]

@Brostafa This occurs when you use the wheel mouse (scroll on canvas)
Here is commit that created this issue maybe you could take a look: a388079

[Brostafa]

@btzr-io After some profiling I found the issue was with setting state on every zoom event
a388079#diff-9d13ed777fc203aef753b28ab30f44efR153 so I have added a debounce to reduce setState invocations and when I re-ran profiling after adding debounce it seems to have become a little bit smoother. Please double-check from your side.

[btzr-io]

@Brostafa That's what I think, but it was working fine even when the state was changed on every zoom, try it without this lines on osd.config:

// Animations
  springStiffness: 12,
  springStiffness: 12,

[btzr-io]

@Brostafa I created a similar solution, the only problem now is there a small delay (as expected) on the text update so is not the best ux practice:

    let isScrolling = false
    let handleStopScrolling = null

    this.viewer.addHandler('canvas-scroll', event => {
      // Reset scrolling flag
      isScrolling = true
      // Clear our timeout throughout the scroll
      window.clearTimeout(handleStopScrolling)
      // Set a timeout to run after scrolling ends
      handleStopScrolling = setTimeout(() => {
        this.updateZoom()
        isScrolling = false;
      },  750);
    })

    this.viewer.addHandler('zoom', ({zoom}) => {
      // Unable to update zoom on scroll inside this event handler:
      // Bad peformance from multiple context update state calls
      if(!isScrolling) {
        this.updateZoom(zoom)
      }
    })

[btzr-io]

See: #66 (comment)

[btzr-io]

@Brostafa I dont see this issue on chrome / brave / electron, only on firefox
What browser are you using ?

could you test it on different browsers and let me know if you see the bug ?

[btzr-io]

@Brostafa I created a patch for firefox (at least works on my pc) but this issue require more testing and optimizations, here dd9cf57

If you think you can improve something in that commit or look in to another issue let me know, I'll close this for now, thanks again for all you help ✌️

[Brostafa]

@btzr-io I couldn't get Firefox on Arch Linux (having dependency hell from doing manual partial updates). Anyway, I booted up Windows to test on Firefox. And I can confirm your commit did improve Firefox performance but didn't improve it for Chrome, that's definitely a step forward either way. I also tested with debounce and it made both Firefox and Chrome smoother but it's not as consistent as your commit. I am having different results when testing the same code and I don't even understand what's really going on 😆

but not so much for Chrome

May I know why you specifically apply optimization for firefox only? dd9cf57#diff-9d13ed777fc203aef753b28ab30f44efR206-R215

// This can run smooth on chromium based browsers (chrome, brave, electon)
I have seen that comment but does that mean the optimization doesn't play well with chromium based browsers?

---

Also, regarding testing I may be seeing things but seems when I tested on Windows Chrome it was a bit different from Linux Chrome (both Chrome versions weren't the same though). So if you will have a testing checklist maybe testing on 3 platform may be worth it to ensure the solution is working cross-platform.

The test I did was mainly using my eyes like I scroll and see if there's any hiccups in the zoom but not sure if this is a really good way to measure performance. For instance React Native has FPS counter to help debug slow running code while chrome have DevTools to profile things I am not quite adept at using it.

[btzr-io]

@Brostafa I only see this issue on firefox (linux) does it behaves similar on chrome`( windows / linux )?

I agree we should have an appropriate way of testing this, however at least in my pc it was very easy to notice when a switch between firefox and chromium based browsers (crhome, brave, electron )

[btzr-io]

My only concern about this new fix is the small delay on the text update caused by the debouncer, feels kind of odd. Thats why I didnt implemented on all browsers.

I'm still trying to find a way to update the current-zoom text without a noticeable dalay

[btzr-io]

@Brostafa We dont need the debounce function when the zoom is not animated ( triggered by buttons or text input).

Perhaps we should be more consistent whit animations and enable them on button and input triggers ? Feel free to open an issue about this if you have the time ( any other idea / bug )

[Brostafa]

@btzr-io Yes, Chrome has little bit of hiccups on Windows. However, when applying your solution (removing it from firefox-only block), Chrome becomes smooth too. Check this video (it doesn't have audio) https://www.loom.com/share/3655f1849f0b4fa29c4ac667cb35579e but again the hiccups are quite small (not that noticable). It may also not be that visible on video as it wasn't recording on a high FPS-rate but if you have Windows laying around you may feel it.

[btzr-io]

Nice thanks for testing and providing more info into this, would you like to sent a pr with those changes?

[Brostafa]

Nice thanks for testing, would you like to sent a pr with those changes?

I am uncertain because your points here:

My only concern about this new fix is the small delay on the text update caused by the debouncer, feels kind of odd. Thats why I didnt implemented on all browsers.

I'm still trying to find a way to update the current-zoom text without a noticeable dalay

are valid and I am not sure of a way to address them. So, I will agree with how you solved the issue for Firefox, if someone complained later we know how to solve the smooth scroll problem or maybe OpenSeaDragon themselves will make internal optimizations that we don't have to worry about this anymore

[btzr-io]

@Brostafa I think for now is ok, maybe I'll also add this fall back for mobile browsers.
Thanks again ✌️

[Brostafa]

Perhaps we should be more consistent whit animations and enable them on button and input triggers ? Feel free to open an issue about this if you have the time ( any other idea / bug )

I agree with this. I think too it should animate on button click (not sure if it's worth making PR tho as its changing immediate: true param in 2 lines 😄)

[Brostafa]

@Brostafa I think for now is ok, maybe I'll also add this fall back for mobile browsers.

Brilliant!

[btzr-io]

For zooming with click now is doubleClick 53c7aef