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

Layout doesn't run if all nodes share the same position #14

Closed
maxkfranz opened this issue May 12, 2016 · 15 comments
Closed

Layout doesn't run if all nodes share the same position #14

maxkfranz opened this issue May 12, 2016 · 15 comments
Labels

Comments

@maxkfranz
Copy link
Member

maxkfranz commented May 12, 2016

Refer to cytoscape/cytoscape.js#1372

@maxkfranz
Copy link
Member Author

@metincansiper Any idea why this would be happening? Thanks!

@ugurdogrusoz
Copy link
Contributor

Regular layout starts with random positions; so this can only happen when all nodes are at the same position and incremental layout is enabled. One shouldn't run incremental layout when nodes don't really have any initial positions (all are at the same location). One way to avoid this is to randomly move nodes at least a little bit.
So what exactly is observed when they are at the same location?

@aindlq
Copy link

aindlq commented May 13, 2016

I tried to enable randomized: true, move nodes out of the same point, apply random layout before cose-bilkent, but it has no effect on "cose-bilkent" layout.

see http://jsbin.com/yuremezoma/1/edit?html,output , try to collapse and then expand some compound node

@ugurdogrusoz
Copy link
Contributor

Interesting. Why do you apply layout on child content upon expand? Why not just break the parent-child relations upon collapse but keep the child nodes around with their locations and restore them upon expand? Here is an example, where we do exactly that: http://cs.bilkent.edu.tr/~ivis/SBGNViz.js_editor/sample-app/

@maxkfranz
Copy link
Member Author

Maybe it's because the layout call is on a collection of nodes with no edges?

@aindlq
Copy link

aindlq commented May 13, 2016

Because user wants to be able to move around collapsed node. In your example when node is expanded it will be moved back to the initial position, but we want to expand it in place.

@metincansiper
Copy link
Contributor

metincansiper commented May 13, 2016

@aindlq
In SBGNViz we handled the movement of collapsed nodes case in the following way. Before collapsing a compound we save the position of it before the collapse operation. Then when it is being expanded we calculate the difference between its current position and the position just before it is collapsed. Finally, we apply the same movements to the restored nodes.

@maxkfranz
I think that in cose-bilkent we are creating layout nodes to apply CoSE layout on them in top-down approach starting from the orphan nodes. I think that because of the reason that the orphan nodes are not passed to the layout the layout nodes are not created for these nodes. I hope that this problem can be solved if we start from the nodes whose parent do not exist between the nodes passed to the layout, instead of starting from the orphans, returned by cy.orphans().

@maxkfranz
Copy link
Member Author

@metincansiper That sounds right. node.parent().intersection( layoutNodes ).empty() is a safer check

metincansiper added a commit that referenced this issue May 15, 2016
Starting to create l level nodes from the top most nodes instead of
orphan nodes
@metincansiper
Copy link
Contributor

metincansiper commented May 15, 2016

I have created a function returning the list of top most nodes in a node list. Then I used this method where I was using nodes.oprhans() previously. The problem seems to be solved now.

@ugurdogrusoz
Copy link
Contributor

@aindlq could you please confirm your problem is fixed and close this issue if so?

@maxkfranz
Copy link
Member Author

@metincansiper @ugurdogrusoz If this change fixes it, then it can be gulp published as 1.3.1

@aindlq
Copy link

aindlq commented May 17, 2016

Hm, maybe I'm doing something wrong but the only change that I can observe is that compound node is fit to viewport.

see http://jsbin.com/qimecaxose/edit?html,output

@metincansiper Thank you for the suggestion, I really like the way you're doing it. This way layout of nodes inside compound after expand will be the same as before +1

@metincansiper
Copy link
Contributor

@aindlq Thanks for noticing the bug. I made some corrections and it seems to be working now. I have also tried your fiddle and see that upon expand operation layout is being performed now. However, compound node still fits to viewport in your fiddle if you do not want this, you can specify the fit option of the layout as false. Could you please confirm that your problem is fixed. If it is fixed we can publish 1.3.1.

@aindlq
Copy link

aindlq commented May 18, 2016

@metincansiper thank you! works fine.

metincansiper added a commit that referenced this issue May 20, 2016
Fixed a layout actually related to tilling bug emerged while fixing the
bugs related to this issue
@metincansiper
Copy link
Contributor

@aindlq we realized that while I was fixing this bug I made a logical mistake and something went wrong in version 1.3.1. Then I corrected that mistake and published version 1.3.2. We wanted to inform you about this. You may want to upgrade to cose-bilkent version 1.3.2.

metincansiper added a commit that referenced this issue May 26, 2016
Changed getTopMostNodes function (Covered an uncovered case)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants