GT-3020 - Function Graph - fixed edge visual state that was being

overwritten after mutation operations

Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/FGEdge.java

@@ -19,6 +19,16 @@ import ghidra.app.plugin.core.functiongraph.graph.vertex.FGVertex;
 import ghidra.graph.viewer.VisualEdge;
 import ghidra.program.model.symbol.FlowType;
 
+/**
+ * This version of the {@link VisualEdge} adds a few methods.
+ * 
+ * <p>The {@link #setDefaultAlpha(double)} method was added here instead of the base interface, as it
+ * was not needed any higher at the time of writing.  It can be pulled-up, but there is most 
+ * likely a better pattern for specifying visual attributes of an edge.  If we find we need more
+ * methods like this, then that is a good time for a refactor to change how we manipulate
+ * rending attributes from various parts of the API (e.g., from the layouts and from animation
+ * jobs).
+ */
 public interface FGEdge extends VisualEdge<FGVertex> {
 
 	public FlowType getFlowType();
@@ -27,6 +37,30 @@ public interface FGEdge extends VisualEdge<FGVertex> {
 
 	public void setLabel(String label);
 
+	/**
+	 * Set this edge's base alpha, which determines how much of the edge is visible/see through.
+	 * 0 is completely transparent.
+	 *   
+	 * <P>This differs from {@link #setAlpha(double)} in that the latter is used for 
+	 * temporary display effects.   This method is used to set the alpha value for the edge when
+	 * it is not part of a temporary display effect.
+	 * 
+	 * @param alpha the alpha value
+	 */
+	public void setDefaultAlpha(double alpha);
+
+	/**
+	 * Set this edge's base alpha, which determines how much of the edge is visible/see through.
+	 * 0 is completely transparent.
+	 *   
+	 * <P>This differs from {@link #getAlpha()} in that the latter is used for 
+	 * temporary display effects.   This method is used to set the alpha value for the edge when
+	 * it is not part of a temporary display effect.
+	* 
+	* @return the alpha value
+	*/
+	public double getDefaultAlpha();
+
 	@SuppressWarnings("unchecked")
 	// Suppressing warning on the return type; we know our class is the right type
 	@Override

Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/graph/FGEdgeImpl.java

@@ -38,7 +38,8 @@ public class FGEdgeImpl implements FGEdge {
 	private boolean inFocusedPath = false;
 	private boolean selected = false;
 	private double emphasis = 0D;
-	private double alpha = 1D;
+	private double defaultAlpha = 1D;
+	private double alpha = defaultAlpha;
 	private String edgeLabel = null;
 
 	public FGEdgeImpl(FGVertex startVertex, FGVertex destinationVertex, FlowType flowType,
@@ -100,6 +101,17 @@ public class FGEdgeImpl implements FGEdge {
 		return alpha;
 	}
 
+	@Override
+	public void setDefaultAlpha(double alpha) {
+		this.defaultAlpha = alpha;
+		this.alpha = alpha;
+	}
+
+	@Override
+	public double getDefaultAlpha() {
+		return defaultAlpha;
+	}
+
 	@Override
 	public List<Point2D> getArticulationPoints() {
 		return layoutArticulationPoints;
@@ -146,6 +158,7 @@ public class FGEdgeImpl implements FGEdge {
 		newEdge.layoutArticulationPoints = newPoints;
 
 		newEdge.alpha = alpha;
+		newEdge.defaultAlpha = defaultAlpha;
 		newEdge.inHoveredPath = inHoveredPath;
 		newEdge.inFocusedPath = inFocusedPath;
 		newEdge.selected = selected;

Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/util/job/AbstractGroupingFunctionGraphJob.java

@@ -68,7 +68,7 @@ public abstract class AbstractGroupingFunctionGraphJob extends AbstractFunctionG
 	 */
 	AbstractGroupingFunctionGraphJob(FGController controller,
 			GroupedFunctionGraphVertex groupVertex, Set<FGVertex> newVertices,
-			Set<FGVertex> verticesToRemove, boolean relayloutOverride, boolean useAnimation) {
+			Set<FGVertex> verticesToRemove, boolean relayoutOverride, boolean useAnimation) {
 
 		super(controller, useAnimation);
 
@@ -87,7 +87,7 @@ public abstract class AbstractGroupingFunctionGraphJob extends AbstractFunctionG
 		FunctionGraphOptions options = controller.getFunctionGraphOptions();
 		RelayoutOption relayoutOption = options.getRelayoutOption();
 		this.relayout = relayoutOption == VERTEX_GROUPING_CHANGES || relayoutOption == ALWAYS ||
-			relayloutOverride;
+			relayoutOverride;
 	}
 
 	@Override
@@ -158,7 +158,7 @@ public abstract class AbstractGroupingFunctionGraphJob extends AbstractFunctionG
 		return positions;
 	}
 
-	/**
+	/*
 	 * Subclasses must return locations for vertices.  This method will be called when no 
 	 * relayout will be performed.
 	 * 
@@ -191,15 +191,21 @@ public abstract class AbstractGroupingFunctionGraphJob extends AbstractFunctionG
 
 	@Override
 	protected void updateOpacity(double percentComplete) {
+
 		double oldComponentsAlpha = 1.0 - percentComplete;
 
 		Collection<FGVertex> vertices = getVerticesToBeRemoved();
 		for (FGVertex vertex : vertices) {
+
 			vertex.setAlpha(oldComponentsAlpha);
 
 			Collection<FGEdge> edges = getEdges(vertex);
 			for (FGEdge edge : edges) {
-				edge.setAlpha(oldComponentsAlpha);
+
+				// don't go past the alpha when removing
+				double defaultAlpha = edge.getDefaultAlpha();
+				double alpha = Math.min(oldComponentsAlpha, defaultAlpha);
+				edge.setAlpha(alpha);
 			}
 		}
 
@@ -210,7 +216,11 @@ public abstract class AbstractGroupingFunctionGraphJob extends AbstractFunctionG
 
 			Collection<FGEdge> edges = getEdges(vertex);
 			for (FGEdge edge : edges) {
-				edge.setAlpha(newComponentsAlpha);
+
+				// don't go past the alpha when adding
+				double defaultAlpha = edge.getDefaultAlpha();
+				double alpha = Math.min(newComponentsAlpha, defaultAlpha);
+				edge.setAlpha(alpha);
 			}
 		}
 	}

Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/util/job/MergeVertexFunctionGraphJob.java

@@ -19,6 +19,7 @@ import java.awt.Rectangle;
 import java.awt.geom.Point2D;
 import java.util.*;
 
+import org.apache.commons.collections4.IterableUtils;
 import org.jdesktop.animation.timing.Animator;
 import org.jdesktop.animation.timing.interpolation.PropertySetter;
 
@@ -197,22 +198,25 @@ public class MergeVertexFunctionGraphJob extends AbstractAnimatorJob {
 		parentVertex.setAlpha(oldComponentsAlpha);
 		childVertex.setAlpha(oldComponentsAlpha);
 
-		Collection<FGEdge> edges = getEdges(parentVertex);
+		Iterable<FGEdge> edges =
+			IterableUtils.chainedIterable(getEdges(parentVertex), getEdges(childVertex));
 		for (FGEdge edge : edges) {
-			edge.setAlpha(oldComponentsAlpha);
-		}
 
-		edges = getEdges(childVertex);
-		for (FGEdge edge : edges) {
-			edge.setAlpha(oldComponentsAlpha);
+			// don't go past the alpha when removing
+			double defaultAlpha = edge.getDefaultAlpha();
+			double alpha = Math.min(oldComponentsAlpha, defaultAlpha);
+			edge.setAlpha(alpha);
 		}
 
 		double newComponentsAlpha = percentComplete;
 		mergedVertex.setAlpha(newComponentsAlpha);
-
 		edges = getEdges(mergedVertex);
 		for (FGEdge edge : edges) {
-			edge.setAlpha(newComponentsAlpha);
+
+			// don't go past the alpha when adding
+			double defaultAlpha = edge.getDefaultAlpha();
+			double alpha = Math.min(newComponentsAlpha, defaultAlpha);
+			edge.setAlpha(alpha);
 		}
 	}
 

Ghidra/Features/FunctionGraph/src/main/java/ghidra/app/plugin/core/functiongraph/util/job/SplitVertexFunctionGraphJob.java

@@ -19,6 +19,7 @@ import java.awt.Rectangle;
 import java.awt.geom.Point2D;
 import java.util.*;
 
+import org.apache.commons.collections4.IterableUtils;
 import org.jdesktop.animation.timing.Animator;
 import org.jdesktop.animation.timing.interpolation.PropertySetter;
 
@@ -114,9 +115,21 @@ public class SplitVertexFunctionGraphJob extends AbstractAnimatorJob {
 
 		controller.synchronizeProgramLocationAfterEdit();
 
+		restoreEdgeDisplayAttributes();
+
 		viewer.repaint();
 	}
 
+	private void restoreEdgeDisplayAttributes() {
+
+		Iterable<FGEdge> edges =
+			IterableUtils.chainedIterable(getEdges(parentVertex), getEdges(childVertex));
+		for (FGEdge edge : edges) {
+			double alpha = edge.getDefaultAlpha();
+			edge.setAlpha(alpha);
+		}
+	}
+
 	public void setPercentComplete(double percentComplete) {
 		trace("setPercentComplete() callback: " + percentComplete);
 		updateNewVertexPositions(percentComplete);
@@ -214,7 +227,11 @@ public class SplitVertexFunctionGraphJob extends AbstractAnimatorJob {
 
 		Collection<FGEdge> edges = getEdges(toSplitVertex);
 		for (FGEdge edge : edges) {
-			edge.setAlpha(oldComponentsAlpha);
+
+			// don't go past the alpha when removing
+			double defaultAlpha = edge.getDefaultAlpha();
+			double alpha = Math.min(oldComponentsAlpha, defaultAlpha);
+			edge.setAlpha(alpha);
 		}
 
 		double newComponentsAlpha = percentComplete;
@@ -223,12 +240,19 @@ public class SplitVertexFunctionGraphJob extends AbstractAnimatorJob {
 
 		edges = getEdges(parentVertex);
 		for (FGEdge edge : edges) {
-			edge.setAlpha(newComponentsAlpha);
+
+			// don't go past the alpha when adding
+			double defaultAlpha = edge.getDefaultAlpha();
+			double alpha = Math.min(newComponentsAlpha, defaultAlpha);
+			edge.setAlpha(alpha);
 		}
 
 		edges = getEdges(childVertex);
 		for (FGEdge edge : edges) {
-			edge.setAlpha(newComponentsAlpha);
+			// don't go past the alpha when adding
+			double defaultAlpha = edge.getDefaultAlpha();
+			double alpha = Math.min(newComponentsAlpha, defaultAlpha);
+			edge.setAlpha(alpha);
 		}
 	}
 

Ghidra/Features/FunctionGraph/src/test/java/ghidra/app/plugin/core/functiongraph/FunctionGraphGroupVertices1Test.java

@@ -21,6 +21,7 @@ import java.awt.Color;
 import java.awt.geom.Point2D;
 import java.util.*;
 
+import org.apache.commons.collections4.IterableUtils;
 import org.junit.*;
 
 import docking.ActionContext;
@@ -37,6 +38,7 @@ import ghidra.framework.plugintool.util.PluginException;
 import ghidra.graph.viewer.options.RelayoutOption;
 import ghidra.program.model.address.Address;
 import ghidra.program.model.address.AddressSetView;
+import util.CollectionUtils;
 
 public class FunctionGraphGroupVertices1Test extends AbstractFunctionGraphTest {
 
@@ -809,14 +811,41 @@ public class FunctionGraphGroupVertices1Test extends AbstractFunctionGraphTest {
 		verifyDefaultColor(v2);
 	}
 
+	@Test
+	public void testEdgeDefaultAlphaPersistsAfterGrouping() {
+
+		graphFunction("01002cf5");
+
+		FGVertex v1 = vertex("01002cf5");
+		FGVertex v2 = vertex("01002d0f");
+
+		FunctionGraph graph = getFunctionGraph();
+		Iterable<FGEdge> edges = graph.getEdges(v1, v2);
+		assertEquals(1, IterableUtils.size(edges));
+		FGEdge edge = CollectionUtils.any(edges);
+
+		Double alpha = edge.getAlpha();
+		assertTrue(alpha < 1.0); // this is the default flow 
+
+		GroupedFunctionGraphVertex group = group("A", v1, v2);
+		ungroup(group);
+
+		edges = graph.getEdges(v1, v2);
+		assertEquals(1, IterableUtils.size(edges));
+		edge = CollectionUtils.any(edges);
+
+		Double alphAfterGroup = edge.getAlpha();
+		assertEquals(alpha, alphAfterGroup);
+	}
+
 	@Test
 	public void testSymbolAddedWhenGrouped_SymbolOutsideOfGroupNode() {
 		// TODO
 	}
 
-	//==================================================================================================
-	// Private Methods
-	//==================================================================================================
+//==================================================================================================
+// Private Methods
+//==================================================================================================
 
 	// @formatter:off
 	@Override

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

@@ -313,7 +313,7 @@ public class TestFGLayoutProvider extends FGLayoutProvider {
 				}
 
 				else if (startCol.index > endCol.index) { // flow return
-					e.setAlpha(.25);
+					e.setDefaultAlpha(.25);
 
 					Shape shape = transformer.apply(startVertex);
 					Rectangle bounds = shape.getBounds();
@@ -338,7 +338,7 @@ public class TestFGLayoutProvider extends FGLayoutProvider {
 
 				else {  // same column--nothing to route
 					// straight line, which is the default
-					e.setAlpha(.25);
+					e.setDefaultAlpha(.25);
 				}
 				newEdgeArticulations.put(e, articulations);
 			}

Ghidra/Features/FunctionGraphDecompilerExtension/src/main/java/ghidra/app/plugin/core/functiongraph/graph/jung/renderer/DNLArticulatedEdgeTransformer.java

@@ -17,7 +17,7 @@ package ghidra.app.plugin.core.functiongraph.graph.jung.renderer;
 
 import ghidra.app.plugin.core.functiongraph.graph.FGEdge;
 
-public class DecompilerDominanceArticulatedEdgeTransformer extends FGArticulatedEdgeTransformer {
+public class DNLArticulatedEdgeTransformer extends FGArticulatedEdgeTransformer {
 
 	@Override
 	public int getOverlapOffset(FGEdge edge) {

Ghidra/Features/FunctionGraphDecompilerExtension/src/main/java/ghidra/app/plugin/core/functiongraph/graph/layout/DecompilerNestedLayout.java

@@ -34,7 +34,7 @@ import ghidra.app.decompiler.DecompInterface;
 import ghidra.app.decompiler.DecompileOptions;
 import ghidra.app.plugin.core.functiongraph.graph.FGEdge;
 import ghidra.app.plugin.core.functiongraph.graph.FunctionGraph;
-import ghidra.app.plugin.core.functiongraph.graph.jung.renderer.DecompilerDominanceArticulatedEdgeTransformer;
+import ghidra.app.plugin.core.functiongraph.graph.jung.renderer.DNLArticulatedEdgeTransformer;
 import ghidra.app.plugin.core.functiongraph.graph.vertex.FGVertex;
 import ghidra.app.plugin.core.functiongraph.graph.vertex.GroupedFunctionGraphVertex;
 import ghidra.graph.VisualGraph;
@@ -103,7 +103,7 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 
 	@Override
 	public Function<FGEdge, Shape> getEdgeShapeTransformer() {
-		return new DecompilerDominanceArticulatedEdgeTransformer();
+		return new DNLArticulatedEdgeTransformer();
 	}
 
 	@Override
@@ -715,7 +715,7 @@ public class DecompilerNestedLayout extends AbstractFGLayout {
 		// assumption: edges that move to the left in this layout are return flows that happen
 		//             after the code block has been executed.  We dim those a bit so that they
 		//             produce less clutter.
-		e.setAlpha(.25);
+		e.setDefaultAlpha(.25);
 	}
 
 	private Column getOutermostCol(LayoutLocationMap<FGVertex, FGEdge> layoutLocations,

Ghidra/Framework/Graph/src/main/java/ghidra/graph/graphs/DefaultVisualGraph.java

@@ -15,8 +15,6 @@
  */
 package ghidra.graph.graphs;
 
-import static com.google.common.collect.Iterables.concat;
-import static com.google.common.collect.Iterables.filter;
 import static util.CollectionUtils.nonNull;
 
 import java.awt.Point;
@@ -206,9 +204,11 @@ public abstract class DefaultVisualGraph<V extends VisualVertex,
 
 		Collection<E> outs = nonNull(getOutEdges(start));
 		Collection<E> ins = nonNull(getInEdges(end));
+		Set<E> unique = new HashSet<>();
+		unique.addAll(outs);
+		unique.addAll(ins);
 
-		Iterable<E> concatenated = concat(outs, ins);
-		Iterable<E> filtered = filter(concatenated, e -> {
+		Iterable<E> filtered = IterableUtils.filteredIterable(unique, e -> {
 			return e.getStart().equals(start) && e.getEnd().equals(end);
 		});
 		return filtered;

Ghidra/Framework/Graph/src/main/java/ghidra/graph/viewer/GraphViewerUtils.java

@@ -699,9 +699,10 @@ public class GraphViewerUtils {
 			return createHollowEgdeLoopInGraphSpace(vertexShape, startX, startY);
 		}
 
-		// translate the edge to the starting vertex
+		// translate the edge from 0,0 to the starting vertex point
 		AffineTransform xform = AffineTransform.getTranslateInstance(startX, startY);
 		Shape edgeShape = renderContext.getEdgeShapeTransformer().apply(e);
+
 		double deltaX = endX - startX;
 		double deltaY = endY - startY;
 
@@ -713,7 +714,7 @@ public class GraphViewerUtils {
 		double dist = Math.sqrt(deltaX * deltaX + deltaY * deltaY);
 		xform.scale(dist, 1.0f);
 
-		// apply the transformations
+		// apply the transformations; converting the given shape from model space into graph space
 		return xform.createTransformedShape(edgeShape);
 	}
 

Ghidra/Framework/Graph/src/main/java/ghidra/graph/viewer/edge/VisualEdgeRenderer.java

@@ -168,7 +168,7 @@ public abstract class VisualEdgeRenderer<V extends VisualVertex, E extends Visua
 		Color baseColor = getBaseColor(graph, e);
 		Color hoveredColor = highlightColor;
 		Color focusedColor = baseColor;
-		Color selectedColor = highlightColor.darker();
+		Color selectedColor = highlightColor.darker(); // note: we can do better for selected color
 		Color selectedAccentColor = highlightColor;
 
 		float scale = StrictMath.min(scalex, scaley);
@@ -223,8 +223,11 @@ public abstract class VisualEdgeRenderer<V extends VisualVertex, E extends Visua
 			// basic shape
 			g.setPaint(fillPaint);
 			g.fill(edgeShape);
-		}
 
+			// Currently, graphs with complicated edge shapes (those with articulations) do not
+			// use a fill paint.  If we execute this code with articulated edges, the display 
+			// looks unusual.   So, for now, only 'fill' with these effects when the client has
+			// explicitly used a fill paint transformer.
 			if (isEmphasized) {
 				Stroke saveStroke = g.getStroke();
 				g.setPaint(fillPaint);
@@ -256,6 +259,7 @@ public abstract class VisualEdgeRenderer<V extends VisualVertex, E extends Visua
 				g.fill(edgeShape);
 				g.setStroke(saveStroke);
 			}
+		}
 
 		//
 		// Draw
@@ -403,10 +407,10 @@ public abstract class VisualEdgeRenderer<V extends VisualVertex, E extends Visua
 	 * @param rc the render context for the graph
 	 * @param graph the graph
 	 * @param e the edge to shape
-	 * @param x1 the start vertex point x
-	 * @param y1 the start vertex point y
-	 * @param x2 the end vertex point x
-	 * @param y2 the end vertex point y
+	 * @param x1 the start vertex point x; layout space
+	 * @param y1 the start vertex point y; layout space
+	 * @param x2 the end vertex point x; layout space
+	 * @param y2 the end vertex point y; layout space
 	 * @param isLoop true if the start == end, which is a self-loop
 	 * @param vertexShape the vertex shape (used in the case of a loop to draw a circle from the 
 	 *              shape to itself)
@@ -417,7 +421,7 @@ public abstract class VisualEdgeRenderer<V extends VisualVertex, E extends Visua
 
 	private BasicStroke getHoveredPathStroke(E e, float scale) {
 		float width = HOVERED_PATH_STROKE_WIDTH / (float) Math.pow(scale, .80);
-		return new BasicStroke(width, BasicStroke.CAP_ROUND, BasicStroke.JOIN_BEVEL, 0f,
+		return new BasicStroke(width, BasicStroke.CAP_ROUND, BasicStroke.JOIN_ROUND, 0f,
 			new float[] { width * 1, width * 2 }, width * 3 * dashingPatternOffset);
 	}
 

Ghidra/Framework/Graph/src/main/java/ghidra/graph/viewer/edge/VisualGraphPathHighlighter.java

@@ -149,6 +149,13 @@ public class VisualGraphPathHighlighter<V extends VisualVertex, E extends Visual
 			TaskMonitor timeoutMonitor = TimeoutTaskMonitor.timeoutIn(ALGORITHM_TIMEOUT,
 				TimeUnit.SECONDS, new TaskMonitorAdapter(true));
 
+			Set<V> sources = GraphAlgorithms.getSources(graph);
+			if (sources.isEmpty()) {
+				Msg.debug(this, "No sources found for graph; cannot calculate dominance: " +
+					graph.getClass().getSimpleName());
+				return null;
+			}
+
 			try {
 				// note: calling the constructor performs the work
 				return new ChkDominanceAlgorithm<>(graph, timeoutMonitor);
@@ -275,12 +282,12 @@ public class VisualGraphPathHighlighter<V extends VisualVertex, E extends Visual
 
 	public void setHoveredVertex(V hoveredVertex) {
 
-		if (workPauser.isPaused()) {
-			return; // hovers a transient, no need to remember the request
-		}
-
 		clearHoveredEdgesSwing();
 
+		if (workPauser.isPaused()) {
+			return; // hovers are transient, no need to remember the request
+		}
+
 		if (hoveredVertex == null) {
 			return;
 		}
@@ -629,9 +636,14 @@ public class VisualGraphPathHighlighter<V extends VisualVertex, E extends Visual
 			return;
 		}
 
-		if (!cf.isCompletedExceptionally()) {
+		if (cf.isCompletedExceptionally()) {
+			return;
+		}
+
 		// clear the contents of the future, as it is acting like a cache
-			clearer.accept(cf.getNow(null));
+		T result = cf.getNow(null);
+		if (result != null) {
+			clearer.accept(result);
 		}
 	}
 

Ghidra/Framework/Graph/src/main/java/ghidra/graph/viewer/renderer/ArticulatedEdgeRenderer.java

@@ -57,9 +57,13 @@ public class ArticulatedEdgeRenderer<V extends VisualVertex, E extends VisualEdg
 				new Point2D.Float((float) point.getX() + offset, (float) point.getY() + offset);
 			point = rc.getMultiLayerTransformer().transform(Layer.LAYOUT, offsetPoint);
 			path.lineTo((float) point.getX(), (float) point.getY());
+			path.moveTo((float) point.getX(), (float) point.getY());
 		}
 
 		path.lineTo(x2, y2);
+		path.moveTo(x2, y2);
+		path.closePath();
+
 		return path;
 	}
 

Ghidra/Framework/Graph/src/main/java/ghidra/graph/viewer/shape/ArticulatedEdgeTransformer.java

@@ -17,7 +17,7 @@ package ghidra.graph.viewer.shape;
 
 import java.awt.Shape;
 import java.awt.geom.*;
-import java.util.*;
+import java.util.List;
 
 import edu.uci.ics.jung.visualization.decorators.ParallelEdgeShapeTransformer;
 import ghidra.graph.viewer.*;
@@ -26,6 +26,8 @@ import ghidra.util.SystemUtilities;
 
 /**
  * An edge shape that renders as a series of straight lines between articulation points.
+ * @param <V> the vertex type
+ * @param <E> the edge type
  */
 public class ArticulatedEdgeTransformer<V extends VisualVertex, E extends VisualEdge<V>>
 		extends ParallelEdgeShapeTransformer<V, E> {
@@ -75,38 +77,50 @@ public class ArticulatedEdgeTransformer<V extends VisualVertex, E extends Visual
 		final double originY = p1.getY();
 
 		int offset = getOverlapOffset(e);
-		GeneralPath generalPath = new GeneralPath();
-		generalPath.moveTo(0, 0);
+		GeneralPath path = new GeneralPath();
+		path.moveTo(0, 0);
 		for (Point2D pt : articulations) {
-			generalPath.lineTo((float) (pt.getX() - originX) + offset,
-				(float) (pt.getY() - originY) + offset);
+			float x = (float) (pt.getX() - originX) + offset;
+			float y = (float) (pt.getY() - originY) + offset;
+			path.lineTo(x, y);
+			path.moveTo(x, y);
 		}
 
-		generalPath.lineTo((float) (p2.getX() - originX), (float) (p2.getY() - originY));
+		float p2x = (float) (p2.getX() - originX);
+		float p2y = (float) (p2.getY() - originY);
+		path.lineTo(p2x, p2y);
+		path.moveTo(p2x, p2y);
+		path.closePath();
 
-		ArrayList<Point2D> reverse = new ArrayList<>(articulations);
-		Collections.reverse(reverse);
-		for (Point2D pt : reverse) {
-			generalPath.lineTo((float) (pt.getX() - originX) + offset,
-				(float) (pt.getY() - originY) + offset);
-		}
 		AffineTransform transform = new AffineTransform();
-
 		final double deltaY = p2.getY() - originY;
 		final double deltaX = p2.getX() - originX;
 		if (deltaX == 0 && deltaY == 0) {
 			// this implies the source and destination node are at the same location, which
 			// is possible if the user drags it there or during animations
-			return transform.createTransformedShape(generalPath);
+			return transform.createTransformedShape(path);
 		}
 
 		double theta = StrictMath.atan2(deltaY, deltaX);
 		transform.rotate(theta);
 		double scale = StrictMath.sqrt(deltaY * deltaY + deltaX * deltaX);
 		transform.scale(scale, 1.0f);
+
+		//
+		// TODO
+		// The current design and use of this transformer is a bit odd.   We currently have code
+		// to create the edge shape here and in the ArticulatedEdgeRenderer.  Ideally, this 
+		// class would be the only one that creates the edge shape.  Then, any clients of the
+		// edge transformer would have to take the shape and then transform it to the desired 
+		// space (the view or graph space).  The transformations could be done using the 
+		// GraphViewerUtils.
+		//
+
 		try {
+			// TODO it is not clear why this is using an inverse transform; why not just create
+			// the transform that we want?
 			AffineTransform inverse = transform.createInverse();
-			Shape transformedShape = inverse.createTransformedShape(generalPath);
+			Shape transformedShape = inverse.createTransformedShape(path);
 			return transformedShape;
 		}
 		catch (NoninvertibleTransformException e1) {