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

fabric 2.4.0 - issue using clipPath when using toJSON / loadFromJSON. #5266

Closed
danrwilson opened this issue Sep 24, 2018 · 25 comments · Fixed by #5279
Closed

fabric 2.4.0 - issue using clipPath when using toJSON / loadFromJSON. #5266

danrwilson opened this issue Sep 24, 2018 · 25 comments · Fixed by #5279

Comments

@danrwilson
Copy link

Version

2.4.0

Test Case

http://jsfiddle.net/danwilsonme/Da7SP/3071/

Information about environment

Version 69.0.3497.100 (Official Build) (64-bit)

Steps to reproduce

Once image has loaded, click the to JSON button, throws the error "Uncaught TypeError: path.shouldCache is not a function" in console.

Expected Behavior

To duplicate the clipPath using to toJSON / loadFromJSON methods.

Actual Behavior

Uncaught TypeError: path.shouldCache is not a function
at klass._drawClipPath (fabric.js:13565)
at klass.drawObject (fabric.js:13555)
at klass.renderCache (fabric.js:13459)
at klass.render (fabric.js:13437)
at klass._renderObjects (fabric.js:7513)
at klass.renderCanvas (fabric.js:7463)
at klass.renderAll (fabric.js:9574)
at fabric.js:12227
at klass._setBgOverlay (fabric.js:12248)

Any suggestions?

@danrwilson
Copy link
Author

I have managed to get it working by removing the previous clipPath before hitting the loadFromJSON, then setting it again once the image has been added to the canvas using the reviveropt.

@TsuchiyaMasahiro
Copy link

TsuchiyaMasahiro commented Sep 24, 2018

I have the same error. But I can't solve the problem.
It's my jsfiddle.
http://jsfiddle.net/uemon/ae0gv8tm/94/

When I click the 'load from JSON' button,
the following error is shown in console.


Uncaught TypeError: e.shouldCache is not a function
at i._drawClipPath (VM181 fabric.min.js:1)
at i.drawObject (VM181 fabric.min.js:1)
at i.renderCache (VM181 fabric.min.js:1)
at i.render (VM181 fabric.min.js:1)
at i._renderObjects (VM181 fabric.min.js:1)
at i.renderCanvas (VM181 fabric.min.js:1)
at i.renderAll (VM181 fabric.min.js:1)
at VM181 fabric.min.js:1
at r (VM181 fabric.min.js:1)
at i.__setBgOverlay (VM181 fabric.min.js:1)


Is it a bug? Or should I use the 'reviveropt' of the loadfromJSON when I use 'clipPath' ?

Thank you in advance.

@shenguan
Copy link

I have managed to get it working by removing the previous clipPath before hitting the loadFromJSON, then setting it again once the image has been added to the canvas using the reviveropt.

I have the same question. How could I make it.

@TsuchiyaMasahiro
Copy link

I have managed to get it working, too.
Here is my jsfiddle.
http://jsfiddle.net/uemon/ae0gv8tm/153/

I used callback function to set clipPath again.
In my way, I have to add a lavel to each object.

I hope the bug is fixed in ver 2.4.1.

@rikkarv
Copy link

rikkarv commented Sep 27, 2018

I think the problem has the origin in the loadFromJson() because it enlivens the image in json format to a fabric object.

However, he observes that this image has a clipPath and tries to draw the clipPath which may raise this error because the clipPath probably remains in json format and not a fabric object

@rikkarv
Copy link

rikkarv commented Sep 27, 2018

document.getElementById('toJson').onclick = function(e) {
    json = canvas.toJSON();
    json.objects.forEach(object => {
        if (object.clipPath) {
            fabric.util.enlivenObjects([object.clipPath], function(arg1) {
                object.clipPath = arg1[0];
            }
        }
    });
    canvas2.loadFromJSON(json, canvas2.renderAll.bind(canvas2));
}

@TsuchiyaMasahiro
Copy link

TsuchiyaMasahiro commented Sep 27, 2018

Oh..you are great!
I updated my jsfiddle. It works fine. Thank you, and thank you @asturur everyday, too!
http://jsfiddle.net/uemon/utryjax4/4/

@TsuchiyaMasahiro
Copy link

TsuchiyaMasahiro commented Sep 28, 2018

I have one problem.
In the first canvas (='c1'), I can move the Rect, and according to the position of the Rect, the clipped image is shown.
But after 'loadfromJSON', I can move the Rect, but the clipped image is shown at same position regardless of the position of the Rect.
I want to display the image according to the position of the Rect again.
What should I do ?
Thank you in advance.
http://jsfiddle.net/uemon/utryjax4/4/

@rikkarv
Copy link

rikkarv commented Sep 28, 2018

@TsuchiyaMasahiro it seems to me that you have set clipPath with "absolutePositioned: true" and by doing so you have the clip path fixed realtive to canvas instead of the clipped object, and that applies to both canvas (despite your statement "In the first canvas (='c1'), I can move the Rect, and according to the position of the Rect, the clipped image is shown" which was not been confirmed).

Please confirm if:

  1. moving clipped object doesn't change the clipping object positioning in BOTH canvas;
  2. removing "absolutePositioned: true" makes the clipping object move according to clipped object in BOTH canvas;

However, i have to say that maybe i didn't understand well your issue and i am a bit confused because you are using the same variable "clipPath" to be added to the canvas and also to be used has clip.

Wait, now i understand you are talking of the "Rect" and not the image!!!

Ok, without understanding what it really happens i could try to guess that in the first canvas, the fabric object (added in line 22 - canvas.add(clipPath);) and the clipping object (added in line 26 - clipPath: clipPath) share the same variable and maybe i am saying a big mistake but they are pointing to the same reference, if you change any setting of the variable, for example setting "clipPath.name = 'xpto'" you are applying it to both objects. And what happens in the second canvas? There is no relation between this objects.

I don't know if you were trying to solve this with the 'clip_id' and 'clip_target' but you should not forget to pass that properties in toJSON(['clip_id', 'clip_target'])

@TsuchiyaMasahiro
Copy link

explanation

@rikkarv Sorry, my poor English explanation...
I changed variable name, and updated my jsfiddle.
http://jsfiddle.net/uemon/utryjax4/84/

Can you understand my problem?

@rikkarv
Copy link

rikkarv commented Sep 28, 2018

$("#toJSON").on('click', function(){
  json = canvas.toJSON(['clip_id', 'clip_target', 'absolutePositioned']);
});
$("#loadJSON").on('click', function(){
  json.objects.forEach(object => {
    if (object.clipPath) {
      fabric.util.enlivenObjects([object.clipPath], function(arg1) {
        object.clipPath = arg1[0];
      });
    }
  });
  canvas2.loadFromJSON(json, function() {
    canvas2.forEachObject(object => {
      if (object.clipPath) {
        let target = object.clip_target;
        canvas2.forEachObject(object2 => {
          if (object2.clip_id && object2.clip_id === target) {
            object.clipPath = object2;
          }
        });
      }
    });
    canvas2.renderAll.bind(canvas2);
  });
});

You have to keep the previous solution ("object.clipPath = arg1[0];") to avoid the error that gave origin to this issue. However, after loading the json, in his callback you have to replace the clipPath and say: "now you have to look to that fabric object - the rectangle - and see where he is going to".

At this point and after making things working, i have to say that this is not a issue from library point of view. I understand what @TsuchiyaMasahiro pretends to achieve and i agree that this could be a very nice feature to implement, but it is not a issue like the one that gave origin to this conversation. In fact the loadFromJson() with clipping has to be fixed in source code because it raises an error that seems to be "solved" with the temporary workaround proposed.

@rikkarv
Copy link

rikkarv commented Sep 28, 2018

@TsuchiyaMasahiro i think my english is the worst in the planet, you don't have to be sorry and i think, never the less, that we are making progresses

@rikkarv
Copy link

rikkarv commented Sep 28, 2018

@rikkarv
Copy link

rikkarv commented Sep 28, 2018

We can replace the:

  json.objects.forEach(object => {
    if (object.clipPath) {
      fabric.util.enlivenObjects([object.clipPath], function(arg1) {
        object.clipPath = arg1[0];
      });
    }
  });

for:

  json.objects.forEach(object => {
    if (object.clipPath) {
      object.clipPath = 'temp';
    }
  });

Here, the main point is to avoid the error "Uncaught TypeError: path.shouldCache is not a function"

EDIT: this doesn't work you really have to set a fabric object

@TsuchiyaMasahiro
Copy link

wow, excellent !
your code is beautiful.
Thank you so much.

@rikkarv
Copy link

rikkarv commented Sep 28, 2018

@TsuchiyaMasahiro thank you for your kind words.
However, be careful because i'm a self-learning and sometimes our solutions are prone to other errors.
Let's keep this issue open and waiting for the real experts evaluation. But i'm glad if my contribution makes their work more easier in detecting the real causes

@rikkarv
Copy link

rikkarv commented Sep 28, 2018

@TsuchiyaMasahiro i recommend setting canvas to "preserveObjectStacking: true" because previously when we moved the clipped object towards the clipping area it was ok but when it was the clipping area moving towards the clipped object it was not working

@TsuchiyaMasahiro
Copy link

@rikkarv Thank you for your kind advice.
I updated jsfiddle. That is what I wanted to do!
http://jsfiddle.net/uemon/utryjax4/90/
Thank you again.

@shenguan
Copy link

@rikkarv hello! When I try this code.

  json.objects.forEach(object => {
    if (object.clipPath) {
      fabric.util.enlivenObjects([object.clipPath], function(arg1) {
        object.clipPath = arg1[0];
      });
    }
  });

Error comes when it runs to fabric.util.enlivenObjects, see the error below:

Uncaught RangeError: Maximum call stack size exceeded
    at extend (fabric.js:1798)
    at extend (fabric.js:1816)
    at extend (fabric.js:1810)
    at extend (fabric.js:1816)
    at extend (fabric.js:1816)
    at extend (fabric.js:1810)
    at extend (fabric.js:1816)
    at extend (fabric.js:1816)
    at extend (fabric.js:1810)
    at extend (fabric.js:1816)

@rikkarv
Copy link

rikkarv commented Sep 29, 2018

@shenguan can you please provide a fiddle? What object are you using as clipPath?

@asturur
Copy link
Member

asturur commented Sep 29, 2018

I forgot fabric.Image had its own version of fromObject method.
I updated it handle clipPath.
Thanks for reporting

(remember that if you need to clip images with RECTANGLES you can also use the crop functionality)

@asturur asturur closed this as completed Sep 29, 2018
@shenguan
Copy link

@rikkarv I found my error.
I use following code

var canvasJSON = canvas.toJSON(['clip_id','clip_target','clipPath','crossOrigin']);

which case my error when canvas.toJSON has clipPath. After console.log(canvasJSON), the canvas object has clipPath: undefined. At this time, fabric.util.enlivenObjects() has problems.

@TeniSoftTechnologies
Copy link

TeniSoftTechnologies commented Dec 5, 2023

None of these worked for me. May be my case was different. If my case is applicable to you then you can implement this.

I realised when you are loading the json on a different device with different width and height, scaling is applied on each and every object so that your design fits the new canvas size.

Now when scaling the objects, make sure to also scale the clipPath. Otherwise the image will get bounce off the clip path. This is how i fixed it.

NB: Make sure you include the canvas width and height when saving the json data: eg:
{
json:canvas.toJSON(),
canvas_width:200,
canvas_height:200
}

var scaleFactor = Math.min(newCanvasWidth/oldCanvasWidth, newCanvasHeight/oldCanvasHeight)
 var objects = canvas.getObjects();
    for (var i in objects) {
        objects[i].scaleX = objects[i].scaleX * scaleFactor;
        objects[i].scaleY = objects[i].scaleY * scaleFactor;
        objects[i].left = objects[i].left * scaleFactor;
        objects[i].top = objects[i].top * scaleFactor;
// Now this willl fix the clip path on the new canvas size
        if(Object.hasOwn(objects[i],"clipPath") && objects[i].clipPath){
          objects[i].clipPath.scaleX = objects[i].clipPath.scaleX * scaleFactor;
          objects[i].clipPath.scaleY = objects[i].clipPath.scaleY * scaleFactor;
          objects[i].clipPath.left = objects[i].clipPath.left * scaleFactor;
          objects[i].clipPath.top = objects[i].clipPath.top * scaleFactor;
        }
        objects[i].setCoords();
    }
    var obj = canvas.backgroundImage;
    if(obj){
        obj.scaleX = obj.scaleX * scaleFactor;
        obj.scaleY = obj.scaleY * scaleFactor;
    }
    canvas.discardActiveObject();
    canvas.renderAll();
    canvas.calcOffset();

//You can create a function, copy the code and call it after canvas.loadFromJSON

@ShaMan123
Copy link
Contributor

seems to me that setting the clipPath's originX/Y to center will do the same wrok with less overhead

@TeniSoftTechnologies
Copy link

seems to me that setting the clipPath's originX/Y to center will do the same wrok with less overhead

I haven't tried that. But i can see that centering it will give you a different positioning if the original positioning wasn't centered.

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.

7 participants