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

Free Drawing, Grouping and loading JSON #2095

Closed
chrisrickard opened this issue Apr 8, 2015 · 27 comments · Fixed by #2101
Closed

Free Drawing, Grouping and loading JSON #2095

chrisrickard opened this issue Apr 8, 2015 · 27 comments · Fixed by #2101
Assignees

Comments

@chrisrickard
Copy link

When creating Group of free drawing segments, exporting toJSON, and then re-loading the same JSON - the paths are all misplaced.

See this fiddle: https://jsfiddle.net/xvzms0j6/17/
And an animated gif of an exact example: http://giphy.com/gifs/3oEduMXd7lMxKZk6ly

Essentially I wish to create a Group from free drawing paths, and allow placement to say the same when exporting and importing.

Any ideas?

@asturur asturur self-assigned this Apr 8, 2015
@asturur
Copy link
Member

asturur commented Apr 8, 2015

i m gonna check it later if is a bug i ll commit a fix.

@asturur
Copy link
Member

asturur commented Apr 8, 2015

screenshot_2015-04-08-09-58-48
sometimes i get the bug, sometimes not.

@chrisrickard
Copy link
Author

I think it has something to do with "what" you free draw :)

Circles with lines through them, or crosses always recreate the bug for me. But non-crossing lines don't seem too.

@chrisrickard
Copy link
Author

Any further help with this would be much appreciated... Unfortunately it is a showstopper in a HTML5 app I am releasing this week.

@kangax any ideas about this interesting issue?

@sapics
Copy link
Contributor

sapics commented Apr 9, 2015

@chrisrickard I have tested by recent version of fabric.js.
https://jsfiddle.net/sapics/x0sLhgtj/5/
The copied canvas has fill color and position is changed.
test

@chrisrickard
Copy link
Author

@sapix yeah I noticed that.. so was avoiding upgrading (as the position bug remained, but also had a fill color issue)

@sapics
Copy link
Contributor

sapics commented Apr 9, 2015

@chrisrickard
Copy link
Author

@sapics same result im afraid - http://imgur.com/Ax5hjrE

@sapics
Copy link
Contributor

sapics commented Apr 9, 2015

@chrisrickard
Copy link
Author

Hi @saphics, that seems to work - but creates another odd issue.

When you click canvas1 (after toJSON'ing).. it repositions the group? :/

See animated gif: http://giphy.com/gifs/xTiTntnWGVlz496L4I

@sapics
Copy link
Contributor

sapics commented Apr 9, 2015

@chrisrickard Oh, sorry. I fixed it to https://jsfiddle.net/sapics/x0sLhgtj/20/

@chrisrickard
Copy link
Author

@sapics Fantastic you did it, we make a great team 👍

@asturur
Copy link
Member

asturur commented Apr 9, 2015

so the bugs with current version are?
fill black and wrong position?

could you be more clear?

i m trying to squash as more bugs as possible

@sapics
Copy link
Contributor

sapics commented Apr 9, 2015

@asturur Sorry for late report. It would be caused by bugs of current version.
I make PR for fix it #2101. I am not good at English. Could you understand what I mean in #2101?

@asturur
Copy link
Member

asturur commented Apr 9, 2015

I see #2101, sadly i m not so group guru and i need time to understand if this is the right fix.
Maybe kangax will be faster.

@sapics
Copy link
Contributor

sapics commented Apr 9, 2015

@asturur Thank you for reading #2101. I would wait for PR review.

@chrisrickard
Copy link
Author

@sapics I think there may be an issue caused by your fix, so I have reopened this issue.

Steps to recreate:

  1. Draw and group the paths
  2. Alter the Group (resize, rotate etc).
  3. Reload the JSON into the canvas (loading from toJSON).

The result is the last action seems to be "rerun". E.g - if you resize the path smaller, every time you reload the JSON it gets smaller and smaller.

See this fiddle: https://jsfiddle.net/mq00xq3y/7/
And an animated gif of an exact example: http://giphy.com/gifs/l41lZIXB8fZwyxHiw

I wasn't 100% sure if it's from your change, but I couldn't seem to recreate it with the current dist version.

@chrisrickard chrisrickard reopened this Apr 15, 2015
@sapics
Copy link
Contributor

sapics commented Apr 15, 2015

@chrisrickard Thank you for your report!
I understand what you pointed out, and it is caused by my fix.
I fixed it, however, it causes another bug, what I have not understood yet.
I guess that It takes some time.

@sapics
Copy link
Contributor

sapics commented Apr 15, 2015

I fixed in https://jsfiddle.net/sapics/mq00xq3y/77/.
As written in it, I checked only for fabric.Rect.
I guess that some problem come from path type.

This issue have not fixed yet.

@sapics
Copy link
Contributor

sapics commented Apr 16, 2015

@chrisrickard I have fixed Path Class. it looks work well.
https://jsfiddle.net/sapics/mq00xq3y/91/

@asturur
Copy link
Member

asturur commented Apr 16, 2015

why path class? does it is not about group's object toObject method general problem?

@sapics
Copy link
Contributor

sapics commented Apr 16, 2015

@asturur There are two problems.
One of the problem comes from group.toObject which includes delegatedProperties.
Another problem comes from path.initialize. (Position difference comes from this.)

@chrisrickard
Copy link
Author

Seems to be fixed, thanks @sapics - your a lifesaver!

@newtriks
Copy link

I maybe mistaken, but simply taking the last fiddle from this thread by @sapics and replacing the source with the latest version of fabricjs, the issue of the path objects being filled on re-load seems to still be a problem? Or is it that the approach taken in the fiddle is no longer compatible?

https://jsfiddle.net/z8d4fb35/

screen shot 2015-06-15 at 21 12 53

On a side note, are you actually able to load a path object into the objects array as opposed to loading as JSON? So for example, if you serialized to a path object, could you take that object and manually start adding it to the canvas via pushing to the objects array?

@sapics
Copy link
Contributor

sapics commented Jun 16, 2015

Please try master-build as https://jsfiddle.net/sapics/z8d4fb35/2/.
I guess that it would be work!

@newtriks
Copy link

@sapics looks great thanks! Adding the object returned directly to canvas doesn't work i.e. canvas.add(pathObj);, still trying to work out how to access and push to the actual canvas items, particularly on load. Thanks for the help anyway.

screen shot 2015-06-16 at 12 08 07

@sapics
Copy link
Contributor

sapics commented Jun 16, 2015

@newtriks That's good!
canvas objects can get by canvas.getObjects.

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.

4 participants