[Bug]: setDimensions triggers renderAndReset multiple times on mobile

[Fjonan]

CheckList

• I agree to follow this project's Code of Conduct
• I have read and followed the Contributing Guide
• I have read and followed the Issue Tracker Guide
• I have searched and referenced existing issues and discussions
• I am filing a BUG report.
• I have managed to reproduce the bug after upgrading to the latest version
• I have created an accurate and minimal reproduction

Version

6.0.2

In What environments are you experiencing the problem?

Firefox, Chrome

Node Version (if applicable)

None

Link To Reproduction

https://codepen.io/Fjonan/pen/JoPJmVr

Steps To Reproduce

1. open this CodePen on a mobile device
2. pinch to zoom the canvas
3. the website freezes for ~2 minutes
4. the canvas updates itself finally
5. now open the CodePen on a desktop device
6. use mouse scroll to zoom the canvas
7. the canvas updates after ~2 seconds

Expected Behavior

The canvas resize and sets a new zoom level should happen in a reasonable amount of time (~3 - 5 seconds).

Actual Behavior

On mobile devices the canvas update takes multiple minutes

I build a demo where zooming a canvas using pinch or mouse wheel uses CSS transform: scale to "zoom" the canvas which is a lot faster than using zoomToPoint(). After the user stopped zooming I am triggering a function that calculates the size and zoom from the CSS and applies them to the canvas, this way the canvas is updated and the image is sharp again.

Here is the demo: https://codepen.io/Fjonan/pen/JoPJmVr

On my desktop device this code works as expected. However on mobile it freezes the browser for multiple minutes. I used remote debugging to look into what is happening and it looks like requestRenderAll triggers renderAndReset multiple times

Screenshot from a performance recording - every red rectangle is a renderAndReset call.

This is from desktop, 20x slowdown and the renderAndReset is fired once and took ~4 seconds

As I understand requestRenderAll this should not be possible to happen and I don't understand why the exact same code behaves so differently.

In both cases - mouse wheel and pinch - 1000 ms after the user stopped zooming this function is executed, migrating the transform: scale() to a new canvas size and zoom:

  /**
   * Converts CSS transform scale to Fabric.js zoom so the blurry image gets sharp again
   */
  function canvasScaleToZoom() {    
    const width = canvas.wrapperEl..getBoundingClientRect().width
    const height = canvas.wrapperEl.getBoundingClientRect().height
    
    // calculate the offset of the canvas inside the wrapper
    const canvasBox = canvas.wrapperEl.getBoundingClientRect()
    const viewBox = wrapper.getBoundingClientRect()
    
    const offsetX = canvasBox.x - viewBox.x
    const offsetY = canvasBox.y - viewBox.y

    // we resize the canvas to the scaled values    
    canvas.setDimensions({height: height, width: width})
    canvas.setZoom(touchZoom)    
    
    // and reset the transform scale
    canvas.wrapperEl.style.transform = `translate(${offsetX}px, ${offsetY}px) scale(1)`
  }

Error Message & Stack Trace

No response

[asturur]

On ym machine the renderAll functions takes around 320ms when slown to 20x and 18 normally.

A couple of reason why mobile may rander twice is that the main thread is so busy it can't cancel the requestRender function in time before it is fired a second time by setZoom. You are calling both setDimension and setZoom.

A reason why mobile may crawl down in performance is the many objects.
You would be better off disabling chaching rather than having 600+ objects on screen each of them cached.
Mobile phones ( iphones especially ) have a limited amount of memory a canvas can allocate.
Try to disable caching.

Alos if you prefer to continue the discussion please reduce the demo to a button where you can call setDimensions, setZoom to arbitrary values rather than keeping around all the mousewheel code in a demo.

[Fjonan]

It might be obvious to you, but since Fabric.js has caching on by default and I never once encountered that particular bit of very useful information:

if you are using simple shapes that are a single draw call ( rect, circle, small polygon, ellipse, triangle ) and you are using only fill or only stroke, caching is at loss

… not in tutorials or the documentation as a recommendation to turn off for these kind of shapes.

So even if it is not an exact science It was not clear to me and I'd argue to a lot of other people, too. So thank you for making it clear.

[asturur]

It should be added to the caching page yes. I have an limited amount of time i can spend on fabricjs. Either i look at issues or i write documentation. I m trying to do a bit of everything, the current weak spot is great docs

[Fjonan]

I plan a write up on my little project. I make sure to add that particular bit of information. Hope it helps. 😁

[asturur]

Took this conversation as a reason to rewrite the cache page that is very old ( was written around fabric 2.0.
fabricjs/fabricjs.github.io#183
Still i need to change a lot of stuff to make it more useufl

[Fjonan]

And here is my write up on the little drag performance project.

[asturur]

If i disable cache, on my machine the normal time goes down to 8ms. It may change depending on how many objects are on screen on that moment.
Also please do not use lodash clone on the objects. Use either their clone method or create a new instance each time.

[Fjonan]

Also please do not use lodash clone on the objects.

I replaced fabric.util.clone from an earlier version (upgraded it to 6.5.3 for this test demo) and from the upgrade guide provided by the Fabric.js project:

consider migrating to a well known common library for those functionalities ( lodash, underscore ... )

I simply followed that advice.

[asturur]

Yes but that is related to cloning javascript objects, if you clone an instance i do not know what you get back.
if you do something like this:

myRect instanceof Rect  // should be true

const clonedRect = _.clone(myRect);
clonedRect instanceof Rect // should be false

const clonedRect2 = await myRect.clone();
clonedRect2 instanceof Rect // should be true.

Since clone is async to cover special cases it is less easy to use.
With rects that are not using patterns you can also do

const clonedRect = new Rect(myRect.toObject());