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

Weird group behavior - initialization vs add #1582

Closed
inssein opened this issue Aug 9, 2014 · 13 comments
Closed

Weird group behavior - initialization vs add #1582

inssein opened this issue Aug 9, 2014 · 13 comments

Comments

@inssein
Copy link
Contributor

inssein commented Aug 9, 2014

I have been trying to figure out groups all day, and it seems that elements within a group position themselves different if added (group.add), versus initialized in the constructor.

I came up with an example here:
http://jsfiddle.net/anwjhs2o/1/

When we run the fiddle, we get expected results as this is a very popular demo (in the intro to fabric article). However, in the javascript, change the the "test" variable to be set to 1 so that it initializes the group differently, and see that the circles are now in different positions.

Is this expected behavior?

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@inssein
Copy link
Contributor Author

inssein commented Aug 9, 2014

it's obvious that one updates the bounds of the group, and one doesn't, but the size of the group itself is static throughout, and that's what I thought it was using to position itself.

@asturur
Copy link
Member

asturur commented Oct 21, 2014

I think you should simply use addWithUpdate method of group instead of add.
I don't know if this is intende behaviour or not, but add comes from the collection inheritance and it just put the object in the _objects array, it does not do anything else.
Maybe is just to use for performance reason when someone want to adds lot of objects and then update just one time.

The problem of addWithUpdate is that it reset your top - left position.
This we could avoid adding a boolean parameter that then calls _calcBound (it already does that) settings the "onlywidthandheigth" to true, skipping the calculation of top and left from the newly added objects.
@kangax, @Kienz what do you say?

@asturur asturur removed their assignment Oct 21, 2014
@kangax
Copy link
Member

kangax commented Oct 29, 2014

@asturur that's a good idea, I like it

@asturur asturur self-assigned this Oct 31, 2014
@asturur
Copy link
Member

asturur commented Oct 31, 2014

after deeper checks, what i said has not much sense.

When we multiselect object with mouse, we create a group, and we do not expect theyr position to change.

When we create a group with initialize + objects, like test = 1 in the provided fiddle, we create the group and later top and left get applied.
What we can have is a boolean in addWithUpdate and removeWithUpdate that restore original group top and left after the update si finished.
But this would be usable just if you specify top,left width and height. All of them.
So we would give an option to user but 4 costrains.
And also, if you create a 300 wide group, and add objects for width = 400, what will happen?

at the end best solution is add items when you create object, if you add later move the group with explicit request.

in that case it works with test = 1 and 0.
http://jsfiddle.net/asturur/anwjhs2o/3/

It is still a possible feature of course, but requires too much internal changes, i would support it if we decide to rework groups to support different relative coords ( from center, from topleft .... )

@asturur asturur removed their assignment Oct 31, 2014
@lukejagodzinski
Copy link

I also had problems with groups. I would expect that object will not move after being added to group. Of course I can use addWithUpdate but I would like also to move objects within the group and updates its boundings without affecting group position. I'm going to make my own version of group which is going to mimic behavior of DisplayObjectContainer from PIXI.js.

@asturur
Copy link
Member

asturur commented Jan 14, 2015

objects do not move.
i think in this issue the approach was wrong.
Someone has to choose if the group influence the objects or the objects influence the group, and then decide if the user has the option to switch between this behaviours.

When you select with mouse, you implicitly think that the objects are defining the group. When you add the object by code, and you specify top and left of the group and width and height, you are asking something weird.

Or you delete the objects position and you put everything in the group in the center, or topleft.... Otherwise what?

Create a group, in a single line of code you specify width of 300, top 50, in the same line of code, you insert 2 objects, one has left 100 and one has left 1000, and both top of 300 what should happen?

I hope what i'm pointing out is clear.
I'm scared of digging this old thread that will just put more work on the fire, but soon or later...

@lukejagodzinski
Copy link

What I mean is group that is dependent on objects that create it. Group width and height should be calculated by counting bounding rect around all its objects. The same with left and top. If you add new object to group or move it, left and top of group should be calculated according to that change. I'm going to create class fabric.Container that does exactly that. I think groups right now can be as they are because they're mostly used for selection.

@asturur
Copy link
Member

asturur commented Jan 14, 2015

I don't know, too much time since i read all that code of group.class, that i can say something that makes sense.
Group is more or less a container, with internal coordinates relative to its center.
So before writing another class, check carefully what the current lacks, just to not waste your own time!

@lukejagodzinski
Copy link

Yes you right I will investigate it deeply. I will be working with it soon so I will let know here what approach I will take. Maybe it can be made by improving fabric.Group. But I can't say right now. I have to investigate Group code.

@solex16
Copy link

solex16 commented Jan 15, 2015

Yes I also found working with instantiating groups to be a little
counter-intuitive because of the issues as demonstrated and discussed here..
Any work in tidying up this core feature would be much appreciated and I'm
sure very valuable for future users of the library..

Robert Franks

8 Bath Road
Beckington
Frome
Somerset

+44 (0)7745 653196

skype: robertfranks

Success is getting what you want
Happiness is wanting what you get..

On 14 January 2015 at 11:28, jagi [email protected] wrote:

Yes you right I will investigate it deeply. I will be working with it soon
so I will let know here what approach I will take. Maybe it can be made by
improving fabric.Group. But I can't say right now. I have to investigate
Group code.

Reply to this email directly or view it on GitHub
#1582 (comment).

@lukejagodzinski
Copy link

I was investigating how groups work and one things seems to be counter intuitive - it's origin point. Right now you can set it only to one of three values top, center, bottom for Y axis and left, center, right for X axis. When adding elements to group (addWithUpdate) Fabric tries to keep origin point in the left, top point of new group dimensions, so it causes change of left and top properties of group. What I would like to do is not changing left and top properties to change but rather move origin point to another position (number position).

The best situation would be to have Container element that have origin set to 0, 0 and position to 0, 0 point by default, what would cause it to be positioned in the same place as canvas scene. Now, adding element withing Container on position -100, -100 would not cause Container position change to -100, -100. It would still stay in the same position and origin wouldn't be affected too. However getting width and height would return proper values, the same about getBoundingRect.

I know that it would take entire Fabric to rewrite but I'm just dreaming :)

@asturur
Copy link
Member

asturur commented Sep 3, 2017

i think i will close this old issue just specifing in docs that:

add is suggested just for canvas
add is good for groups just for batch adding
add should be followed by an addWithUpdate

@ncou do you feel like adding a comment over the jsdocs explaining a bit this situation?

the method is add: in the collections mixin.

@ncou
Copy link
Collaborator

ncou commented Sep 4, 2017

Hi,

I am away from my personal comuter for 2 weeks. I could try to add a comment as soon as im back, but my english is not perfect :), and you speak about the "add" method from the Group object. is that right ?

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

No branches or pull requests

6 participants