cytoscape / cytoscape.js-dagre

The Dagre layout for DAGs and trees for Cytoscape.js
MIT License
250 stars 83 forks source link

Add possibility to force nodes positions. #46

Closed alessandro308 closed 5 years ago

alessandro308 commented 5 years ago

Sometimes, can be useful to have a layout automatically computed by dagre without losing the possibility to specify the position of some nodes.

To do that I created a variable (dagreForcePosition) to insert in the node data that, if true, force the layout to use the user-specified node position instead of applying the dagre computed position. A use case is when you compute a dagre layout but want to leave the possibility to the user to move nodes.

For more flexibility, I also added a callback parameter to the layout options. This function gets two parameters, the instance of Cytoscape and the raw instance of Dagre. This can be useful to fix some nodes position knowing the dagre computations so to avoid ugly node overlappings.

maxkfranz commented 5 years ago

I'm trying to understand the motivation behind the options:

(1) dagreForcePosition : When would you use this feature instead of excluding certain nodes from the layout -- either by the calling collection or by locking?

(2) callback : What purpose does exposing the dagre object serve? How is this different than doing pre- or post-layout adjustments to position?

alessandro308 commented 5 years ago

Locking some nodes, means you have to add nodes locked, compute the layout (that is influenced by these nodes) and then unlock them to set the right position. With dagreForcePosition I suggest a way to exclude the nodes from the layout computation without introducing some extra logic in the code to generate the Cytoscape object.

About the callback, I explicit needs to know where some nodes are placed in order to set my "excluded" nodes position. So I need the final position to place my nodes before to invoke the rendering of the graph. Indeed, without the callback, if I move the nodes after layout, I need an extra function to update the graph to show the changes.

maxkfranz commented 5 years ago

Locking some nodes, means you have to add nodes locked, compute the layout (that is influenced by these nodes) and then unlock them to set the right position. With dagreForcePosition I suggest a way to exclude the nodes from the layout computation without introducing some extra logic in the code to generate the Cytoscape object.

From your description, it seems to me that the proper way to accommodate that function would be with pre- and post-layout adjustments like locking. That approach works with every layout and is flexible.

About the callback, I explicit needs to know where some nodes are placed in order to set my "excluded" nodes position. So I need the final position to place my nodes before to invoke the rendering of the graph. Indeed, without the callback, if I move the nodes after layout, I need an extra function to update the graph to show the changes.

Again, this sounds like you should be running the layout and then doing an adjustment afterwards.

Each layout is meant to be a small unit: It's just a function that returns positions according to its algorithm. The out-of-the-box options won't always be perfect for your application. You may have to alter the options to fit your app, and you may even have to make adjustments before or after a layout. Sometimes running just one layout isn't enough for some use-cases, and you need to run multiple layouts to get the effect you want. By keeping each layout simple and consistent, it's easy to do those kinds of things.

So unless I'm misunderstanding your pull request, I don't see anything novel here. This behaviour can be accommodated by the existing API in a more general way.

Further, this layout is just a wrapper for Dagre. It translates Cytoscape objects into Dagre objects so that the programmer never has to leave the Cytoscape mental model. I'd rather keep this extension as a simple wrapper; it really shouldn't be introducing new layout concepts outside of Dagre.