diff --git a/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/ColSegmentList.java b/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/ColSegmentList.java index 888749033e..15eaec964c 100644 --- a/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/ColSegmentList.java +++ b/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/ColSegmentList.java @@ -116,7 +116,7 @@ public class ColSegmentList { private void assignOffsets(ColSegmentList 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 { } } + 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 columnSegment : edgeSegments) { + if (columnSegment.isFlowingUpwards() != firstSegmentIsUpwardFlowing) { + return false; + } + } + return true; + } + private void assignOffsets(ColSegmentList group, int center) { List> nonOverlappingSegments = new ArrayList<>(); diff --git a/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/ColumnSegment.java b/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/ColumnSegment.java index aec1da0263..f839a19f47 100644 --- a/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/ColumnSegment.java +++ b/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 The edge type */ -public class ColumnSegment extends EdgeSegment implements Comparable> { +public class ColumnSegment extends EdgeSegment { // specifies the orientation of the row attached to this segment (either top or bottom) enum RowOrientation { @@ -93,12 +93,11 @@ public class ColumnSegment extends EdgeSegment implements Comparable 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 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 extends EdgeSegment implements Comparable 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 extends EdgeSegment implements Comparable getTopRowSegment() { - return flowsUp() ? next : previous; + return isFlowingUpwards() ? next : previous; } private RowSegment getBottomRowSegment() { - return flowsUp() ? previous : next; + return isFlowingUpwards() ? previous : next; } private RowOrientation getOrientationForTopRow() { @@ -318,7 +351,8 @@ public class ColumnSegment extends EdgeSegment implements Comparable 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 extends EdgeSegment implements Comparable 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(); } diff --git a/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/RowSegment.java b/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/RowSegment.java index 548c3d3856..edbf7e40a8 100644 --- a/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/RowSegment.java +++ b/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 The edge type */ -public class RowSegment extends EdgeSegment implements Comparable> { +public class RowSegment extends EdgeSegment { // 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 extends EdgeSegment implements Comparable 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 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 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 extends EdgeSegment implements Comparable 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 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 getLeftColSegment() { - return flowsLeft() ? next : previous; + return isFlowingLeft() ? next : previous; } private ColumnSegment getRightColSegment() { - return flowsLeft() ? previous : next; + return isFlowingLeft() ? previous : next; } - private boolean flowsLeft() { + boolean isFlowingLeft() { return getStartCol() > getEndCol(); } diff --git a/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/RowSegmentList.java b/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/RowSegmentList.java index 105f6aecff..8052d7076c 100644 --- a/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/RowSegmentList.java +++ b/Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/RowSegmentList.java @@ -78,7 +78,7 @@ public class RowSegmentList { */ public void assignOffsets() { // sorts the row edge segments from top to bottom - Collections.sort(edgeSegments); + sort(); Map> offsetMap = LazyMap.lazyMap(new HashMap<>(), k -> new RowSegmentList(0)); @@ -88,6 +88,28 @@ public class RowSegmentList { } } + 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 rowSegment : edgeSegments) { + if (rowSegment.isFlowingLeft() != firstSegmentIsFlowingLeft) { + return false; + } + } + return true; + } + protected void assignOffset(Map> offsetMap, RowSegment segment) { // assigning offsets to rows is easy, just find the first offset group that we don't diff --git a/Ghidra/Features/FunctionGraph/src/test.slow/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/FlowChartLayoutTest.java b/Ghidra/Features/FunctionGraph/src/test.slow/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/FlowChartLayoutTest.java index 6816c5ef8b..9d180a569f 100644 --- a/Ghidra/Features/FunctionGraph/src/test.slow/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/FlowChartLayoutTest.java +++ b/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))); + + } + } diff --git a/Ghidra/Features/FunctionGraph/src/test/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/EdgeSegmentTest.java b/Ghidra/Features/FunctionGraph/src/test/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/EdgeSegmentTest.java index 52ac1992c7..08c4e810d4 100644 --- a/Ghidra/Features/FunctionGraph/src/test/java/ghidra/app/plugin/core/functiongraph/graph/layout/flowchart/EdgeSegmentTest.java +++ b/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 up1_left = endSegment(e13, p, UP, LEFT, UP); - ColumnSegment up2_left = segment(e23, p, UP_2, LEFT, UP); + ColumnSegment 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 up1_right = endSegment(e13, p, UP, RIGHT, UP); - ColumnSegment up2_right = segment(e23, p, UP_2, RIGHT, UP); + ColumnSegment 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 s1, ColumnSegment s2) { - int result = s1.compareTo(s2); - if (result != 0) { - fail("Expected comparsion to be equals, but compareTo was " + result); - } - } - private void assertLessThan(ColumnSegment s1, ColumnSegment 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 s1, RowSegment 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 s1, RowSegment 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 s1, ColumnSegment 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); } }