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

Zoom and pan using canvas.setZoom() should affect canvas background image #2946

Closed
raffareis opened this issue May 6, 2016 · 20 comments · Fixed by #3019
Closed

Zoom and pan using canvas.setZoom() should affect canvas background image #2946

raffareis opened this issue May 6, 2016 · 20 comments · Fixed by #3019

Comments

@raffareis
Copy link

raffareis commented May 6, 2016

Version

1.6.1

Test Case

1.6.1 (Actual): https://jsfiddle.net/mL58b3rr/10/
1.6.0-rc.1 (Expected) : https://jsfiddle.net/1c6n1bry/3/
(They are both the exact same code, the only difference is the library`s version)

Steps to reproduce

Try Zooming in and out

Expected Behavior

Prior to version 1.6.0 (1.6.0-rc1), when setting the canvas zoom, through canvas.setZoom(), would also set the zoom of the background image. 1.6.0 and on, only the elements are being zoomed / panned

Actual Behavior

the canvas' Background image is static and don't change on canvas.setZoom(), in versions 1.6.0+

@asturur
Copy link
Member

asturur commented May 6, 2016

That is hard to say.
When viewport zoom was introduced the backgroundImage and overlayimage were unaffected.
In a refactor of renderAll we put them under effect of zoom. We realized controls had problem under certain conditions and we switched back to normal behaviour.

So the expected behaviour is background not zooming.
If you need the background to zoom you can still put an unevented unselectable image as first object in canvas.
I mark as a possible feature and we see if there is interest to put an(other) option.

@asturur asturur changed the title Zoom and pan using canvas.setZoom() are not updating canvas background image Zoom and pan using canvas.setZoom() should affect canvas background image May 6, 2016
@tonybart1337
Copy link

tonybart1337 commented May 6, 2016

If you need the background to zoom you can still put an unevented unselectable image as first object in canvas.

It doesn't work if you select any objects within that image. Selected objects and image will be merged into group, hence you will be able to move a group, hence image will move with other objects.

example

@asturur
Copy link
Member

asturur commented May 6, 2016

https://jsfiddle.net/mL58b3rr/12/

its is selectable:false, not selected:false

To me it looks ok. What do you think?

@tonybart1337
Copy link

https://jsfiddle.net/mL58b3rr/12/

its is selectable:false, not selected:false

To me it looks ok. What do you think?

Yes. It works just fine now, but +1 for this feature, cuz I have to add some sendToBack calls if I use the solution above.

@asturur
Copy link
Member

asturur commented May 6, 2016

i agree, we ll see if we can implement it in a easy way without messing up code. consider overlayImage has a specular situation. controls above overlay or control before overlay...

@raffareis
Copy link
Author

@asturur thanks a lot for the solution!

But still i agree with @AshtonRoute , i too have a complicated "layer" control that will be messed, but i will make the necessary adjustments.

@asturur
Copy link
Member

asturur commented May 6, 2016

lets get some genuine +1 and then we will see...

@butch2k
Copy link

butch2k commented May 14, 2016

As i use fabric for RPG tabletop, having the bg image follow zooming behavior would be simpler, especially when it comes down to layer management.
So for me it's a +1 as well.

@asturur
Copy link
Member

asturur commented May 14, 2016

As i use fabric for RPG tabletop
I wanna see this! where?

@HunterMitchell
Copy link

@asturur persoanlly I think there would be no pint for a background image if it didn't scale/zoom with the canvas. This broke my current implementation.

@HunterMitchell
Copy link

@asturur Let me elaborate further. I think that scaling the backgroundImage is mandatory. If you set a background image, you expect that to expand with the canvas (just like background images in css repeat unless otherwise stated). If users did not want the scaling, then they shouldn't be using a "background" image.
This is a defiantly a +1 for me, as every single release has fixed/introduced a new bug to hinder my project. (i.e. group selection, this "bug", etc.)

@asturur
Copy link
Member

asturur commented May 16, 2016

i understand the point of view, i somewhat agree.
i want to clarify somwthing:
the background image was never zoomed before. it was an accidental build made public.

now imagine to use zoom without enlarging the canvas, how should the overlay image behave?

and the overlay with enlarged canvas?

if using panning should the background move or not? because we can use panning without zooming also, if the background image follow zoom it follows also panning.

All that questions are the reason why i m hesitant to modify it now

@HunterMitchell
Copy link

I completely agree. My main reasoning was the backgroundImage needs to act like a "background image". Let me attempt to answer your questions the way i think it should work.

now image to use zoom without enlarging the canvas, how should the overlay image behave?

The overlay image should act the same. It should also scale/zoom with the canvas (after all, the image should be considered a non-selectable object am i right?). If there is a need for the image not to scale, then instead of "adding" the image to the canvas, it should be inserted right before the image (forcing it to always be on top).

and the overlay with enlarged canvas?

I think this is the same question as above.

if using panning should the background move or not?

This is actually something I never though of, however the way i look at it is that when you are panning, you're panning the content AND the background - again the background image should act like a object. I require in my project that the background and the content above stay in the same position.

Conclusion: Honestly, I think it would be easy to add a flag/variable to enable background scaling. canvas.backgroundScaling = true.

@asturur
Copy link
Member

asturur commented May 16, 2016

i know, but from my experience the growing of flags is the reason why we have bug issues and fix with every single release. i try something later

@HunterMitchell
Copy link

HunterMitchell commented May 16, 2016

Sounds good to me. I already fixed the issue by simply setting the backgroundImage size whenever I zoom my canvas like so:

canvas.backgroundImage.setWidth(canvas.backgroundImage.getOriginalSize().width * canvas.getZoom());
@canvas.backgroundImage.setHeight(canvas.backgroundImage.getOriginalSize().height * canvas.getZoom());

@asturur
Copy link
Member

asturur commented May 16, 2016

I'll try to setup a demo with the options that come to my mind and we'll see general use case for them.
Imagine a canvas used for a rgb tabletop where the background is a chessboard representing a room. Of course you want the bacground to move and zoom with the viewport. If the the overlay is used to represents some hidden area ( roofs that get semi transparent when you enter in the room ) you want to move them also.

But if you zoom out instead of zooming in, the image will get smaller, how do you react to that?
If you pan without zooming, the image is not there anymore. is this fine?

Please everyone interested contribute with his opinion.

@butch2k
Copy link

butch2k commented May 16, 2016

Here is my point of view.
First standard BG Image with no pan/zoom
http://www.prz.in/files/Clipboard01.jpg

Second with zoom out/panning, BGImage must reduce and area left blank should be colored with background color.
http://www.prz.in/files/Clipboard02.jpg

Same goes with overlay

@asturur
Copy link
Member

asturur commented May 16, 2016

I agree with you. As of now we either draw background color OR background image.
The only point is that behaviour from 1.5 to 1.6.2 is different ( other than that 1.6.0rc1 accident ). And that would be an back incompatible change... or another flag.
Let's see how long takes me for the planned feature of milestone 1.6.3 and let' see if there is space also for this.

@asturur
Copy link
Member

asturur commented May 25, 2016

in regards of #3007, and with the intention of simplify things. This is effectively a bug as it is now. So we will get a parameter soon than expected. I think defaul behaviour will be the one described here.

@asturur
Copy link
Member

asturur commented May 30, 2016

i close this just because i will handle it with #3007.

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 a pull request may close this issue.

5 participants