Rotated background image disappearing upon panning

[driek-vhees]

Hi awesome FabricJS team 👋 and first of all, thank you for all the awesome work on this library 🙇
This is also my first bug report so please correct me if I made a mistake somewhere in the process 🙇

Version

4.2.0

Test Case

(Unfortunately I was not able to reproduce behavior below in JSfiddle, but managed it in CodePen. I hope that is also OK)
https://codepen.io/driek/pen/yLJLVwW
I have placed two buttons, "rotate 48 degrees" and "rotate 54 degrees".
When pressing either button, the image will be rotated and panned accordingly to create illusion that image stays centered.
For reference, I also display a semi-transparent rectangle to indicate the original position of the background image.
When pressing the former button, the image is still visible (and so is said rectangle).
However, when pressing the latter button, the background is not visible anymore (see also behavior description below).
NOTE: since pressing a button will invoke canvas.relativePan, the buttons cannot be pressed in sequence but the page needs to be refreshed to reset to initial state before another button can be pressed.

Information about environment

Nodejs or browser?
Browser

Which browsers?
Verified Codepen test case with:

• Chrome (85.0.4183.121)
• Firefox (81.0)

Steps to reproduce

• Initialize a canvas which is in "landscape" with a background image that has a wide width but shallow height. The canvas has a zoom ratio to fit the image exactly.
• Rotate the background image using setBackgroundImage and update angle option to the degree that the original position lays outside the viewport.
  (for convenience I set originX and originY to "center" and for values of top and left I used center point of minimum required dimensions to fit the rotated image in my example)
• use relativePan to create the illusion that the background image stays centered

Expected Behavior

The background image to always be visible

Actual Behavior

The image disappears.
My best guess is that the position/dimension of the background image before rotation is being used to check whether or not to render the image as part of optimization. If you un-comment the line in my example so setCoordsis invoked on the background image however, the image will be visible as expected.

Currently I think this behavior is applicable to panning which means it also happens with zoomToPoint. (<= not part of the test case, but came across this while developing locally)
If you are aware of other places where the same kind of thing could happen, pointing out those places is highly appreciated. (for risk assessment to higher ups and QA on my side)
However, if my approach is wrong to begin with, please point that as well. Thank you in advance for your time and effort 🙇

[asturur]

The reason you are experience the bug is because there is an optimization to skip offScreen object from rendering.
Everytime you finish a bulk of changes, you should call .setCoords() on the objects you changed. Or setViewport if you changed the zoom/pan.
Let me see if i can fix the codepen.

[asturur]

https://jsfiddle.net/asturur/2Lrnockw/1/

[driek-vhees]

Hi @asturur 👋
Thank you for the feedback and JSfiddle 🙇 .
I was not sure whether fabricJS should call setCoords as part of calculation of image bounds or the source that uses it.
In this case whenever we rotate the background image, we must make sure to call setCoords on the background image ourselves it seems.
That is good to know and thank you for pointing it out. Maybe to prevent similar questions in the future, would it be possible to add this somewhere in the docs of background image or on the wiki (it is easy to miss that the background image is also an "ordinary" canvas object)?

Regarding setViewportTransform, I noticed that it will update the "regular" objects but not the background. Should also the setCoords on the background be called in here?

fabric.js/src/static_canvas.class.js

Lines 681 to 687 in 4f48cc5

	for (i = 0, len = this._objects.length; i < len; i++) {
	object = this._objects[i];
	object.group || object.setCoords(true);
	}
	if (activeObject) {
	activeObject.setCoords();
	}

(I am not super familiar with CanvasJS codebase so please disregard this comment if I am wrong 😅 )

Thank you again for your swift reply and also the rest of the team for the great work on this great lib 👍

[asturur]

Sorry for the late response.
Yes definitely setViewportTransform should call it on the backgroundImage and overlay too.
i'll fix that.