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

Image setSrc, setElement and width/height value #4733

Closed
scriptspry opened this issue Feb 17, 2018 · 9 comments · Fixed by #4877
Closed

Image setSrc, setElement and width/height value #4733

scriptspry opened this issue Feb 17, 2018 · 9 comments · Fixed by #4877
Labels

Comments

@scriptspry
Copy link
Contributor

scriptspry commented Feb 17, 2018

Version

2.0.1

Test Case

Tried to load the same image with exact same code and got different outputs.

v1.7.22 - Code - Output

v2.0.1 - Code - Output

Expected Behavior

Expected the full image to be rendered.

Actual Behavior

Only got 1/4th? of the image rendered.


First off, I do have a "working" solution for even v2.0.1 here. But, it is a hack. Also, I have already gone through the v1 to v2 breaking changes documents, and read and understood the parts about fabric.Image and fabric.Image.fromObject.

Now, I have already spent a couple of hours of time on this, and came to a conclusion that library is misbehaving. Here's why:

  1. width = 128 & scaleX = 1 !== width = 256 & scaleX = 0.5 !!!

If the width and scaleX semantics for other shapes: rect, circle etc are rendering same output for both cases. Why does Image behave differently? Is it not an inconsistency with interpretation of what width and scaleX values are?

  1. We can almost never have scaleX as 1 for fabric.Image instances, except when image's width, height match fabric.Image object's width height.

This feels like a painful and limiting enforcement.

@asturur
Copy link
Member

asturur commented Feb 17, 2018

The situation is what is stated in the docs.
If you need a picture, big 512px to display in 128px, use scaleToWidth(128). That was the correct procedure also on 1.7

I do not see the inconsistency. Can you be more clear?

@scriptspry
Copy link
Contributor Author

The width and scaleX semantics for fabric.Image have changed in v2.

In v1:

Load an image of size 256, 256
Image has width 256, height 256, scaleX 1 scaleY 1
Image.scaleToWidth(128) now image has width 256, height 256, scaleX 0.5, scaleY 0.5
Now, Image.set({ width: 128, scaleX: 1, height: 128, scaleX: 1 })
And Image width is 128, scaleX 1, height is 128, scaleY 1 and full image is rendered after downscaling to 128x128.

In v2,

The last step failed.


I am starting to believe this may be a wrong expectation from my end. fabric is just using width and scaleX differently for fabric.Image.

It is no longer possible to stretch fit an image into fabric.Image with out knowing what the loaded image's naturalWidth and naturalHeight are.

It's only possible to set image size to desired size by doing something like this:

let img = i.getElement();

if (img && img.naturalWidth !== 0 && img.naturalHeight !== 0) {
	i.set({
		width: img.naturalWidth,
		height: img.naturalHeight,
		scaleX: desiredWidth / img.naturalWidth,
		scaleY: desiredHeight / img.naturalHeight
	});
}

That's the core of what triggered my problem. A fabric.Image's width scaleX is depending on loaded image's dimensions. If for any reason images referred in canvas change dimension, all the canvas jsons referring to this image need to be migrated.

@asturur
Copy link
Member

asturur commented Feb 18, 2018

So on your end loading the image as empty and then setting the src, is complocating things. Probably setSrc should reset wodth/height bit this would wipe crop information.

@asturur
Copy link
Member

asturur commented Feb 18, 2018

Taking an image and doing set Src or swapping the element to draw, is something that generally is part of a custom logic. Maybe setSrc should also reset the image width and height to the one available for the image.

@asturur asturur reopened this Feb 18, 2018
@asturur
Copy link
Member

asturur commented Feb 18, 2018

Let's change title and keep.open for discussion.

@scriptspry
Copy link
Contributor Author

scriptspry commented Feb 18, 2018

@asturur After a little more lingering I found discussion about my exact problem at #4235 from last year.

Seems like @ozooner had already fixed this in their branch.

Here's the updated fiddle that works with the forked v2 fabric build found in fiddle referenced in the comment.

It would be great to get the fix into the library. Image asset size should not effect (de)serialisation IMO.

EDIT: updated fiddle link.

@asturur asturur changed the title Unexplained fabric.Image rendering behaviour in v2 Image setSrc, setElement and width/height value Feb 18, 2018
@asturur
Copy link
Member

asturur commented Feb 18, 2018

So the deserialization from 1.7 is incompatible with 2.0, so the major version bump.
I posted a guide to restore images from pre 2.0

Here the problem is if a setSrc, should also fix width/height to full image or not.

@salomonsanz
Copy link

salomonsanz commented Mar 9, 2018

I agree with @scriptspry i have the same problem migrating from 1 to 2 fabric version.
In my fabric app, in order to optimize the users bandwidth and load speed, when user scales up an image, i recalculate the src to get a bigger one with more quality, but when i make the "setSrc" with the new image quality, fabric "crops" the new image. In v1 this works ok.

@asturur
Copy link
Member

asturur commented Mar 22, 2018

definitely setSrc should reset width/height information and set cropX, cropY to 0.

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

Successfully merging a pull request may close this issue.

3 participants