Community
Participate
Working Groups
The following code which uses CompoundDirectedGraphLayout produces the layout captured in the attached Compound.png. As you can see one of the edges inappropriately shoots out to the side. Eclipse version: 3.2RC7 draw2d version: org.eclipse.draw2d_3.2.0.v20060529.jar ------------------------------------------------------ NodeList nodes = new NodeList(); EdgeList edges = new EdgeList(); Subgraph p = new Subgraph("parent"); nodes.add(p); Node a = new Node("a", p); nodes.add(a); Node b = new Node("b", p); nodes.add(b); Node c = new Node("c", p); nodes.add(c); Node d = new Node("d", p); nodes.add(d); Node e = new Node("e", p); nodes.add(e); edges.add(new Edge(a, d)); edges.add(new Edge(a, c)); edges.add(new Edge(b, c)); edges.add(new Edge(b, d)); edges.add(new Edge(b, e)); edges.add(new Edge(c, d)); edges.add(new Edge(c, e)); CompoundDirectedGraph graph = new CompoundDirectedGraph(); graph.nodes = nodes; graph.edges = edges; new CompoundDirectedGraphLayout().visit(graph); --------------------------------------------------------------- non-compound version: NodeList nodes = new NodeList(); EdgeList edges = new EdgeList(); Node a = new Node("a"); nodes.add(a); Node b = new Node("b"); nodes.add(b); Node c = new Node("c"); nodes.add(c); Node d = new Node("d"); nodes.add(d); Node e = new Node("e"); nodes.add(e); edges.add(new Edge(a, d)); edges.add(new Edge(a, c)); edges.add(new Edge(b, c)); edges.add(new Edge(b, d)); edges.add(new Edge(b, e)); edges.add(new Edge(c, d)); edges.add(new Edge(c, e)); DirectedGraph graph = new DirectedGraph(); graph.nodes = nodes; graph.edges = edges; new DirectedGraphLayout().visit(graph);
Created attachment 43441 [details] shows edge that shoots out to the side
Created attachment 43442 [details] same graph but not compound looks fine
We need to see if this is a regreession in 3.2 or not...
Created attachment 44137 [details] same graph compound but using Draw2d 3.1.1 looks fine This problem appears to be a regression in 3.2. The problem doesn't occur in 3.1.1 with the same graph data. See attached...
Randy - can you have a look at this since you made some enhancements to the layout in 3.2? Upgrading severity since this is a regression.
I don't think this is a regression. The 3.1 release of the demo accesses the Edge#vNodes field, while the 3.2 release uses the newly added getPoints() method. I think that the vNodes are still in the same place as they were in 3.1.
Reducing severity since getPoints() is new function in 3.2.
From a user perspective, it has to be considered a regression. I copied the exact graph code into a 3.1.1 target and received the results above.
(In reply to comment #8) > From a user perspective, it has to be considered a regression. I copied the > exact graph code into a 3.1.1 target and received the results above. That's because you were using the 3.1.1 graph example. If you take the 3.1.1 graph example, and run it with the 3.2 graph layout, the provided test case works fine. So, it's not a regression. It is only the 3.2 graph example code which fails. Try running the provided snippet and looking at vNodes at debug time. They are in the correct place.
Ok... but certainly if people are using the graph demo as the accepted way to reconstruct their diagram from the graph layout then they could run into this. I'll leave it targeted for RC5 for now in case you discover that it is potentially more damaging or there is a simple fix. Otherwise, since there is a workaround (to reference 3.1.1 example for reconstructing the draw2d objects) then it can move to 3.2.1.
If someone wants to investigate a fix, the problem is in RouteEdges. It actually uses the ShortestPathRouter algorithm to route the edges around nodes. Perhaps this is an issue of there not being space between two nodes. Or, there is a bug in the intersection code.
Mohammed - could you look further into this?
RouteEdges is also used by DirectedGraphLayout. Conceivably this could be a problem for DirectedGraphLayout as well in a different use case? We have another client with edge bendpoints going out into space...
reproduced the problem with DirectedGraph and using simple graph data.
Created attachment 44454 [details] Direct graph use case
SImple Graph to reproduce the problem in Direct Graph Layout NodeList nodes = new NodeList(); EdgeList edges = new EdgeList(); Node a = new Node("a"); nodes.add(a); Node b = new Node("b"); nodes.add(b); Node c = new Node("c"); nodes.add(c); Node d = new Node("d"); nodes.add(d); edges.add(new Edge(a, b)); edges.add(new Edge(a, c)); edges.add(new Edge(a, d)); edges.add(new Edge(b, c)); DirectedGraph graph = new CompoundDirectedGraph(); graph.nodes = nodes; graph.edges = edges; new CompoundDirectedGraphLayout() .visit(graph);
A proposed patch to fix the problem
The patch (attached to bug 147159) changes the behavior for padding. Also, if I set the padding to 0 or maybe 1, the bug is still there. The problem is in the generation of the obstacles that the path is expected to go through. We should make sure there is always at least 1 pixel available for the path.
Created attachment 44830 [details] proposed fix
(In reply to comment #18) > The patch (attached to bug 147159) changes the behavior for padding. Also, if I > set the padding to 0 or maybe 1, the bug is still there. The problem is in the > generation of the obstacles that the path is expected to go through. We should > make sure there is always at least 1 pixel available for the path. The proposed fix make the compound Horizontal spacing calculate the delta on the edge in the same way as the HorizontalSpacing algorithm. If this is wrong then the HorizontalSpacing algorithm had a bug both algorithms had to calculate it the same way.
(In reply to comment #20) > (In reply to comment #18) > > The patch (attached to bug 147159) changes the behavior for padding. Also, if I > > set the padding to 0 or maybe 1, the bug is still there. The problem is in the > > generation of the obstacles that the path is expected to go through. We should > > make sure there is always at least 1 pixel available for the path. > The proposed fix make the compound Horizontal spacing calculate the delta on > the edge in the same way as the HorizontalSpacing algorithm. If this is wrong > then the HorizontalSpacing algorithm had a bug > both algorithms had to calculate it the same way. we need to close on this bugzilla; the issue is really bad for any one useing the compound layout the fix seems safe to me; we could also put the protection code Randy suggested to make sure that there is at least 1 pixel separation between the obsticals if needed
(In reply to comment #21) > [...] we could also put the protection code Randy suggested > to make sure that there is at least 1 pixel separation between the obsticals if > needed Did we fix the patch and add this protection code?
After discussion between Mohammed and Randy, we are safe to commit this patch. Please confirm if this is not the case.
Committed to HEAD
