Tweaked edge routing

Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/ColSegmentList.java

@@ -116,7 +116,7 @@ public class ColSegmentList<E> {
 
 	private void assignOffsets(ColSegmentList<E> group) {
 		// First sort the column segments in a left to right ordering.
-		Collections.sort(group.edgeSegments);
+		group.sort();
 
 		// Column segments are extend both to the right and left of the grid line, so first
 		// find a starting edge to assign to 0 and then work in both directions giving offsets
@@ -138,6 +138,28 @@ public class ColSegmentList<E> {
 		}
 	}
 
+	private void sort() {
+		if (isUniformFlow()) {
+			Collections.sort(edgeSegments, (s1, s2) -> s1.compareToUsingFlows(s2));
+		}
+		else {
+			Collections.sort(edgeSegments, (s1, s2) -> s1.compareToIgnoreFlows(s2));
+		}
+	}
+
+	private boolean isUniformFlow() {
+		if (edgeSegments.isEmpty()) {
+			return true;
+		}
+		boolean firstSegmentIsUpwardFlowing = edgeSegments.get(0).isFlowingUpwards();
+		for (ColumnSegment<E> columnSegment : edgeSegments) {
+			if (columnSegment.isFlowingUpwards() != firstSegmentIsUpwardFlowing) {
+				return false;
+			}
+		}
+		return true;
+	}
+
 	private void assignOffsets(ColSegmentList<E> group, int center) {
 		List<ColSegmentList<E>> nonOverlappingSegments = new ArrayList<>();
 

Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/ColumnSegment.java

@@ -27,7 +27,7 @@ import ghidra.graph.viewer.layout.GridPoint;
  *
  * @param <E> The edge type
  */
-public class ColumnSegment<E> extends EdgeSegment<E> implements Comparable<ColumnSegment<E>> {
+public class ColumnSegment<E> extends EdgeSegment<E> {
 
 	// specifies the orientation of the row attached to this segment (either top or bottom)
 	enum RowOrientation {
@@ -93,12 +93,11 @@ public class ColumnSegment<E> extends EdgeSegment<E> implements Comparable<Colum
 		return points.get(pointIndex + 1).row;
 	}
 
-	@Override
-	public int compareTo(ColumnSegment<E> other) {
-		// To make the comparison reversible and transitive, it is important that we are
-		// consistent in the order we compare segments. We arbitrarily chose to always compare
-		// the order by following the shape of the top and only considering the shape on the bottom
-		// if the tops are equal.
+	public int compareToIgnoreFlows(ColumnSegment<E> other) {
+		// When comparing edge segments that have mixed flow directions, we arbitrarily chose to
+		// always compare the order by following the shape of the top and only considering the shape
+		// on the bottom if the tops are equal. This needs to be consistent so that the comparison
+		// is transitive and reversible.
 		//
 		// NOTE: Segments are compared by following the next or previous segments until one of the
 		// segments definitively determines the order. When comparing segments in a particular
@@ -107,8 +106,42 @@ public class ColumnSegment<E> extends EdgeSegment<E> implements Comparable<Colum
 		// use the appropriate direction comparison so that it will follow that direction until
 		// it finds a difference or it simply returns 0, in which case the original to
 		// compareTo can then try the other direction. As a consequence of this, the basic obvious
-		// comparison of the grid columns first had to be moved into both the compareTops and the
-		// compareBottoms.
+		// comparison of first comparing the grid column's index had to be moved into both the
+		// compareTops and the compareBottoms.
+
+		int result = compareTops(other);
+		if (result == 0) {
+			result = compareBottoms(other);
+		}
+		return result;
+	}
+
+	public int compareToUsingFlows(ColumnSegment<E> other) {
+		// When comparing segments that flow in the same direction, we prefer to compare previous
+		// edges first and if they are equal, we compare follow-on edges. This yields better
+		// results (less edge crossings) for some edge cases because it allows all the segments
+		// in an edge to compare consistently in the same direction which is not guaranteed in 
+		// the ignore flow case. However, this comparison can only be used for sorting when all
+		// the segments in a list flow in the same direction. Otherwise the comparison is not
+		// transitive, which could result in breaking the sort algorithm.
+
+		// NOTE: Segments are compared by following the next or previous segments until one of the
+		// segments definitively determines the order. When comparing segments in a particular
+		// direction, is is important not to directly call the compareTo methods as that could result
+		// in an infinite loop. Instead, when comparing in a particular direction, just directly
+		// use the appropriate direction comparison so that it will follow that direction until
+		// it finds a difference or it simply returns 0, in which case the original 
+		// compareTo can then try the other direction. As a consequence of this, the basic obvious
+		// comparison of first comparing the grid column's index had to be moved into both the
+		// compareTops and the compareBottoms.
+
+		if (isFlowingUpwards()) {
+			int result = compareBottoms(other);
+			if (result == 0) {
+				result = compareTops(other);
+			}
+			return result;
+		}
 
 		int result = compareTops(other);
 		if (result == 0) {
@@ -306,11 +339,11 @@ public class ColumnSegment<E> extends EdgeSegment<E> implements Comparable<Colum
 	}
 
 	private RowSegment<E> getTopRowSegment() {
-		return flowsUp() ? next : previous;
+		return isFlowingUpwards() ? next : previous;
 	}
 
 	private RowSegment<E> getBottomRowSegment() {
-		return flowsUp() ? previous : next;
+		return isFlowingUpwards() ? previous : next;
 	}
 
 	private RowOrientation getOrientationForTopRow() {
@@ -318,7 +351,8 @@ public class ColumnSegment<E> extends EdgeSegment<E> implements Comparable<Colum
 			return RowOrientation.TERMINAL;
 		}
 		RowSegment<E> topRowSegment = getTopRowSegment();
-		int topRowOtherCol = flowsUp() ? topRowSegment.getEndCol() : topRowSegment.getStartCol();
+		int topRowOtherCol =
+			isFlowingUpwards() ? topRowSegment.getEndCol() : topRowSegment.getStartCol();
 		return topRowOtherCol < getCol() ? RowOrientation.LEFT : RowOrientation.RIGHT;
 	}
 
@@ -328,11 +362,11 @@ public class ColumnSegment<E> extends EdgeSegment<E> implements Comparable<Colum
 		}
 		RowSegment<E> bottomRowSegment = getBottomRowSegment();
 		int bottomRowOtherCol =
-			flowsUp() ? bottomRowSegment.getStartCol() : bottomRowSegment.getEndCol();
+			isFlowingUpwards() ? bottomRowSegment.getStartCol() : bottomRowSegment.getEndCol();
 		return bottomRowOtherCol < getCol() ? RowOrientation.LEFT : RowOrientation.RIGHT;
 	}
 
-	private boolean flowsUp() {
+	public boolean isFlowingUpwards() {
 		return getStartRow() > getEndRow();
 	}
 

Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/RowSegment.java

@@ -27,7 +27,7 @@ import ghidra.graph.viewer.layout.GridPoint;
  *
  * @param <E> The edge type
  */
-public class RowSegment<E> extends EdgeSegment<E> implements Comparable<RowSegment<E>> {
+public class RowSegment<E> extends EdgeSegment<E> {
 	// specifies the orientation of the column attached to this segment (either left or right)
 	enum ColumnOrientation {
 		UP,		// the attached column extends upwards from this row		 
@@ -90,8 +90,55 @@ public class RowSegment<E> extends EdgeSegment<E> implements Comparable<RowSegme
 		return Math.max(getStartCol(), getEndCol());
 	}
 
-	@Override
-	public int compareTo(RowSegment<E> other) {
+	// When comparing edge segments that have mixed flow directions, we arbitrarily chose to
+	// always compare the order by following the shape of the left and only considering the shape
+	// on the right if the lefts are equal. This needs to be consistent so that the comparison
+	// is transitive and reversible.
+	//
+	// NOTE: Segments are compared by following the next or previous segments until one of the
+	// segments definitively determines the order. When comparing segments in a particular
+	// direction, is is important not to directly call the compareTo methods as that could result
+	// in an infinite loop. Instead, when comparing in a particular direction, just directly
+	// use the appropriate direction comparison so that it will follow that direction until
+	// it finds a difference or it simply returns 0, in which case the original to
+	// compareTo can then try the other direction. As a consequence of this, the basic obvious
+	// comparison of first comparing the grid row's index had to be moved into both the
+	// compareLefts and the compareRights.
+
+	public int compareToIgnoreFlows(RowSegment<E> other) {
+		int result = compareLefts(other);
+		if (result == 0) {
+			result = compareRights(other);
+		}
+		return result;
+	}
+
+	// When comparing segments that flow in the same direction, we prefer to compare previous
+	// edges first and if they are equal, we compare follow-on edges. This yields better
+	// results (less edge crossings) for some edge cases because it allows all the segments
+	// in an edge to compare consistently in the same direction which is not guaranteed in 
+	// the ignore flow case. However, this comparison can only be used for sorting when all
+	// the segments in a list flow in the same direction. Otherwise the comparison is not
+	// transitive, which could result in breaking the sort algorithm.
+
+	// NOTE: Segments are compared by following the next or previous segments until one of the
+	// segments definitively determines the order. When comparing segments in a particular
+	// direction, is is important not to directly call the compareTo methods as that could result
+	// in an infinite loop. Instead, when comparing in a particular direction, just directly
+	// use the appropriate direction comparison so that it will follow that direction until
+	// it finds a difference or it simply returns 0, in which case the original to
+	// compareTo can then try the other direction.As a consequence of this, the basic obvious
+	// comparison of first comparing the grid row's index had to be moved into both the
+	// compareLefts and the compareRights.
+	public int compareToUsingFlows(RowSegment<E> other) {
+		if (isFlowingLeft()) {
+			int result = compareRights(other);
+			if (result == 0) {
+				result = compareLefts(other);
+			}
+			return result;
+		}
+
 		int result = compareLefts(other);
 		if (result == 0) {
 			result = compareRights(other);
@@ -229,25 +276,26 @@ public class RowSegment<E> extends EdgeSegment<E> implements Comparable<RowSegme
 
 	private ColumnOrientation getOrientationForLeftColumn() {
 		ColumnSegment<E> leftSegment = getLeftColSegment();
-		int leftColOtherRow = flowsLeft() ? leftSegment.getEndRow() : leftSegment.getStartRow();
+		int leftColOtherRow = isFlowingLeft() ? leftSegment.getEndRow() : leftSegment.getStartRow();
 		return leftColOtherRow < getRow() ? ColumnOrientation.UP : ColumnOrientation.DOWN;
 	}
 
 	private ColumnOrientation getOrientationForRightColumn() {
 		ColumnSegment<E> rightSegment = getRightColSegment();
-		int rightColOtherRow = flowsLeft() ? rightSegment.getStartRow() : rightSegment.getEndRow();
+		int rightColOtherRow =
+			isFlowingLeft() ? rightSegment.getStartRow() : rightSegment.getEndRow();
 		return rightColOtherRow < getRow() ? ColumnOrientation.UP : ColumnOrientation.DOWN;
 	}
 
 	private ColumnSegment<E> getLeftColSegment() {
-		return flowsLeft() ? next : previous;
+		return isFlowingLeft() ? next : previous;
 	}
 
 	private ColumnSegment<E> getRightColSegment() {
-		return flowsLeft() ? previous : next;
+		return isFlowingLeft() ? previous : next;
 	}
 
-	private boolean flowsLeft() {
+	boolean isFlowingLeft() {
 		return getStartCol() > getEndCol();
 	}
 

Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/RowSegmentList.java

@@ -78,7 +78,7 @@ public class RowSegmentList<E> {
 	 */
 	public void assignOffsets() {
 		// sorts the row edge segments from top to bottom
-		Collections.sort(edgeSegments);
+		sort();
 
 		Map<Integer, RowSegmentList<E>> offsetMap =
 			LazyMap.lazyMap(new HashMap<>(), k -> new RowSegmentList<E>(0));
@@ -88,6 +88,28 @@ public class RowSegmentList<E> {
 		}
 	}
 
+	private void sort() {
+		if (isUniformFlow()) {
+			Collections.sort(edgeSegments, (s1, s2) -> s1.compareToUsingFlows(s2));
+		}
+		else {
+			Collections.sort(edgeSegments, (s1, s2) -> s1.compareToIgnoreFlows(s2));
+		}
+	}
+
+	private boolean isUniformFlow() {
+		if (edgeSegments.isEmpty()) {
+			return true;
+		}
+		boolean firstSegmentIsFlowingLeft = edgeSegments.get(0).isFlowingLeft();
+		for (RowSegment<E> rowSegment : edgeSegments) {
+			if (rowSegment.isFlowingLeft() != firstSegmentIsFlowingLeft) {
+				return false;
+			}
+		}
+		return true;
+	}
+
 	protected void assignOffset(Map<Integer, RowSegmentList<E>> offsetMap, RowSegment<E> segment) {
 
 		// assigning offsets to rows is easy, just find the first offset group that we don't

Ghidra/Features/FunctionGraph/src/test.slow/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/FlowChartLayoutTest.java

@@ -18,6 +18,7 @@ package ghidra.app.plugin.core.functiongraph.graph.layout.flowchart;
 import org.junit.Before;
 import org.junit.Test;
 
+import ghidra.graph.graphs.LabelTestVertex;
 import ghidra.graph.support.TestVisualGraph;
 import ghidra.util.exception.CancelledException;
 
@@ -261,4 +262,112 @@ public class FlowChartLayoutTest extends AbstractFlowChartLayoutTest {
 				.colSegment(down(1), offset(-3)));
 	}
 
+	@Test
+	public void testGraphThatBenefitsFromComparingEdgesUsingFlow() throws CancelledException {
+		LabelTestVertex X = v('X');
+		LabelTestVertex Y = v('Y');
+		LabelTestVertex Z = v('Z');
+		LabelTestVertex W = v('W');
+
+		edge(A, B);
+		edge(A, C);
+		edge(A, D);
+		edge(B, K);
+		edge(K, J);
+		edge(D, F);
+		edge(G, I);
+		edge(D, E);
+		edge(E, G);
+		edge(F, Z);
+		edge(F, H);
+		edge(E, I);
+		edge(J, W);
+		edge(W, X);
+		edge(W, Y);
+		edge(W, Z);
+
+		applyLayout();
+
+//		showGraph();
+
+		assertVertices("""
+				.........
+				.....A...
+				.........
+				...B.C.D.
+				.........
+				...K..E.F
+				.........
+				...J..G.H
+				.........
+				...W..I..
+				.........
+				.X.Y.Z...
+				""");
+
+		assertEdge(e(A, B)
+				.colSegment(down(1), offset(-2))
+				.rowSegment(left(2), offset(0))
+				.colSegment(down(1), offset(0)));
+
+		assertEdge(e(A, C)
+				.colSegment(down(2), offset(0)));
+
+		assertEdge(e(A, D)
+				.colSegment(down(1), offset(2))
+				.rowSegment(right(2), offset(0))
+				.colSegment(down(1), offset(0)));
+
+		assertEdge(e(B, K)
+				.colSegment(down(2), offset(0)));
+
+		assertEdge(e(K, J)
+				.colSegment(down(2), offset(0)));
+
+		assertEdge(e(J, W)
+				.colSegment(down(2), offset(0)));
+
+		assertEdge(e(W, X)
+				.colSegment(down(1), offset(-2))
+				.rowSegment(left(2), offset(0))
+				.colSegment(down(1), offset(0)));
+
+		assertEdge(e(W, Y)
+				.colSegment(down(2), offset(0)));
+
+		assertEdge(e(W, Z)
+				.colSegment(down(1), offset(2))
+				.rowSegment(right(2), offset(0))
+				.colSegment(down(1), offset(-1)));
+
+		assertEdge(e(D, E)
+				.colSegment(down(1), offset(-1))
+				.rowSegment(left(1), offset(0))
+				.colSegment(down(1), offset(0)));
+
+		assertEdge(e(D, F)
+				.colSegment(down(1), offset(1))
+				.rowSegment(right(1), offset(0))
+				.colSegment(down(1), offset(0)));
+
+		assertEdge(e(E, I)
+				.colSegment(down(1), offset(-2))
+				.rowSegment(left(1), offset(0))
+				.colSegment(down(2), offset(-1))
+				.rowSegment(right(1), offset(0))
+				.colSegment(down(1), offset(-2)));
+
+		assertEdge(e(E, G)
+				.colSegment(down(2), offset(0)));
+
+		assertEdge(e(F, H)
+				.colSegment(down(2), offset(0)));
+
+		assertEdge(e(F, Z)
+				.colSegment(down(1), offset(-2))
+				.rowSegment(left(3), offset(2))
+				.colSegment(down(5), offset(1)));
+
+	}
+
 }

Ghidra/Features/FunctionGraph/src/test/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/EdgeSegmentTest.java

@@ -40,7 +40,7 @@ public class EdgeSegmentTest {
 	private TestVertex v3 = v(3);
 	private TestVertex v4 = v(4);
 	private TestEdge e12 = e(v1, v2);
-	private TestEdge e13 = e(v1, v2);
+	private TestEdge e13 = e(v1, v3);
 	private TestEdge e14 = e(v1, v4);
 	private TestEdge e23 = e(v2, v3);
 	private TestEdge e24 = e(v2, v4);
@@ -342,7 +342,7 @@ public class EdgeSegmentTest {
 		GridPoint p = p(0, 0);
 
 		ColumnSegment<TestEdge> up1_left = endSegment(e13, p, UP, LEFT, UP);
-		ColumnSegment<TestEdge> up2_left = segment(e23, p, UP_2, LEFT, UP);
+		ColumnSegment<TestEdge> up2_left = endSegment(e23, p, UP_2, LEFT, UP);
 
 		assertLessThan(up1_left, up2_left);
 		assertGreaterThan(up2_left, up1_left);
@@ -354,7 +354,7 @@ public class EdgeSegmentTest {
 		GridPoint p = p(0, 0);
 
 		ColumnSegment<TestEdge> up1_right = endSegment(e13, p, UP, RIGHT, UP);
-		ColumnSegment<TestEdge> up2_right = segment(e23, p, UP_2, RIGHT, UP);
+		ColumnSegment<TestEdge> up2_right = endSegment(e23, p, UP_2, RIGHT, UP);
 
 		assertLessThan(up2_right, up1_right);
 		assertGreaterThan(up1_right, up2_right);
@@ -891,36 +891,32 @@ public class EdgeSegmentTest {
 
 	}
 
-	private void assertCompareEquals(ColumnSegment<TestEdge> s1, ColumnSegment<TestEdge> s2) {
-		int result = s1.compareTo(s2);
-		if (result != 0) {
-			fail("Expected comparsion to be equals, but compareTo was " + result);
-		}
-	}
-
 	private void assertLessThan(ColumnSegment<TestEdge> s1, ColumnSegment<TestEdge> s2) {
-		int result = s1.compareTo(s2);
+		boolean sameFlow = s1.isFlowingUpwards() == s1.isFlowingUpwards();
+		int result = sameFlow ? s1.compareToUsingFlows(s2) : s1.compareToIgnoreFlows(s2);
 		if (result >= 0) {
 			fail("Expected comparsion to be less than, but compareTo was " + result);
 		}
 	}
 
 	private void assertGreaterThan(RowSegment<TestEdge> s1, RowSegment<TestEdge> s2) {
-		int result = s1.compareTo(s2);
+		boolean sameFlow = s1.isFlowingLeft() == s1.isFlowingLeft();
+		int result = sameFlow ? s1.compareToUsingFlows(s2) : s1.compareToIgnoreFlows(s2);
 		if (result <= 0) {
 			fail("Expected comparsion to be greater than, but compareTo was " + result);
 		}
 	}
 
 	private void assertLessThan(RowSegment<TestEdge> s1, RowSegment<TestEdge> s2) {
-		int result = s1.compareTo(s2);
+		int result = s1.compareToUsingFlows(s2);
 		if (result >= 0) {
 			fail("Expected comparsion to be less than, but compareTo was " + result);
 		}
 	}
 
 	private void assertGreaterThan(ColumnSegment<TestEdge> s1, ColumnSegment<TestEdge> s2) {
-		int result = s1.compareTo(s2);
+		boolean sameFlow = s1.isFlowingUpwards() == s1.isFlowingUpwards();
+		int result = sameFlow ? s1.compareToUsingFlows(s2) : s1.compareToIgnoreFlows(s2);
 		if (result <= 0) {
 			fail("Expected comparsion to be greater than, but compareTo was " + result);
 		}
@@ -976,7 +972,7 @@ public class EdgeSegmentTest {
 	}
 
 	private TestEdge e(TestVertex vertex1, TestVertex vertex2) {
-		return new TestEdge(v1, v2);
+		return new TestEdge(vertex1, vertex2);
 	}
 
 }