Skip to content

Commit

Permalink
feat: add option to draw edges using layout calc
Browse files Browse the repository at this point in the history
edges overlap less with nodes and other edges when using layout calc.

includes change edge routing to SPLINES because that seems to
avoid overlap better.

wanted to make this default/not behind an option, but unfortunately
the edges drawn via layout calc do not have vertical slopes at their
start and end points. I think this can be ok in some situations, but
others look bad and would much prefer the vertical slopes (which the
simple edges guarantee).

also added a comment TODO for modifying the edge drawing via layout
calc to have vertical slopes - if this were done, we should be able
to remove this config option and just _always_ draw via layout calc.
  • Loading branch information
keyserj committed Nov 24, 2024
1 parent 71921ee commit 4e73246
Show file tree
Hide file tree
Showing 9 changed files with 307 additions and 49 deletions.
7 changes: 3 additions & 4 deletions src/web/topic/components/Diagram/Diagram.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ import { PanDirection, panDirections, useViewportUpdater } from "@/web/topic/hoo
import { connectNodes, reconnectEdge } from "@/web/topic/store/createDeleteActions";
import { useDiagram } from "@/web/topic/store/store";
import { useUserCanEditTopicData } from "@/web/topic/store/userHooks";
import { Diagram as DiagramData } from "@/web/topic/utils/diagram";
import { type Edge, type Node } from "@/web/topic/utils/graph";
import { Diagram as DiagramData, PositionedEdge, PositionedNode } from "@/web/topic/utils/diagram";
import { hotkeys } from "@/web/topic/utils/hotkeys";
import { FlowNodeType } from "@/web/topic/utils/node";
import { tutorialIsOpen } from "@/web/tutorial/tutorial";
Expand Down Expand Up @@ -71,13 +70,13 @@ const edgeTypes: Record<"FlowEdge", ComponentType<EdgeProps>> = { FlowEdge: Flow
// react-flow passes exactly DefaultNodeProps but data can be customized
// not sure why, but DefaultNodeProps has xPos and yPos instead of Node's position.x and position.y
export interface NodeProps extends DefaultNodeProps {
data: Node["data"];
data: PositionedNode["data"];
}

export interface EdgeProps extends DefaultEdgeProps {
// we'll always pass data - why does react-flow make it nullable :(
// can't figure out how to amend this to make it non-nullable, since react-flow's Edge is defined as a type, not an interface
data?: Edge["data"];
data?: PositionedEdge["data"];
}

const onEdgeUpdate: OnEdgeUpdateFunc = (oldEdge, newConnection) => {
Expand Down
22 changes: 10 additions & 12 deletions src/web/topic/components/Edge/ScoreEdge.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Box, Typography } from "@mui/material";
import lowerCase from "lodash/lowerCase";
import { EdgeLabelRenderer, getBezierPath } from "reactflow";
import { EdgeLabelRenderer } from "reactflow";

import { RelationName } from "@/common/edge";
import { useSessionUser } from "@/web/common/hooks";
Expand All @@ -14,6 +14,7 @@ import {
edgeColor,
highlightedEdgeColor,
} from "@/web/topic/components/Edge/ScoreEdge.styles";
import { getPathDefinitionForEdge } from "@/web/topic/components/Edge/svgPathDrawer";
import { CommonIndicators } from "@/web/topic/components/Indicator/CommonIndicators";
import {
LeftCornerStatusIndicators,
Expand All @@ -25,12 +26,12 @@ import { useIsNodeSelected } from "@/web/topic/store/edgeHooks";
import { useUserCanEditTopicData } from "@/web/topic/store/userHooks";
import { Edge } from "@/web/topic/utils/graph";
import { useUnrestrictedEditing } from "@/web/view/actionConfigStore";
import { setSelected } from "@/web/view/currentViewStore/store";
import { setSelected, useDrawSimpleEdgePaths } from "@/web/view/currentViewStore/store";

const flowMarkerId = "flowMarker";
const nonFlowMarkerId = "nonFlowMarker";

// copied directly from html generated by react-flow - jank but the package doesn't export its marker definition
// mostly copied from react-flow's marker html - jank but the package doesn't export its marker definition
// https://github.com/xyflow/xyflow/blob/f0117939bae934447fa7f232081f937169ee23b5/packages/react/src/container/EdgeRenderer/MarkerDefinitions.tsx#L29-L41
const svgMarkerDef = (inReactFlow: boolean, spotlight: Spotlight) => {
const id = `${inReactFlow ? flowMarkerId : nonFlowMarkerId}-${spotlight}`;
Expand Down Expand Up @@ -95,6 +96,7 @@ export const ScoreEdge = ({ inReactFlow, ...flowEdge }: EdgeProps & Props) => {
const { sessionUser } = useSessionUser();
const userCanEditTopicData = useUserCanEditTopicData(sessionUser?.username);
const unrestrictedEditing = useUnrestrictedEditing();
const drawSimpleEdgePaths = useDrawSimpleEdgePaths();

const edge = convertToEdge(flowEdge);
const isNodeSelected = useIsNodeSelected(edge.id);
Expand All @@ -105,21 +107,17 @@ export const ScoreEdge = ({ inReactFlow, ...flowEdge }: EdgeProps & Props) => {
? "secondary"
: "normal";

const [edgePath, labelX, labelY] = getBezierPath({
sourceX: flowEdge.sourceX,
sourceY: flowEdge.sourceY,
sourcePosition: flowEdge.sourcePosition,
targetX: flowEdge.targetX,
targetY: flowEdge.targetY,
targetPosition: flowEdge.targetPosition,
});
const { pathDefinition, labelX, labelY } = getPathDefinitionForEdge(
flowEdge,
drawSimpleEdgePaths,
);

const path = (
<StyledPath
id={flowEdge.id}
style={flowEdge.style}
className="react-flow__edge-path"
d={edgePath}
d={pathDefinition}
// assumes that we always want to point from child to parent
markerStart={`url(#${inReactFlow ? flowMarkerId : nonFlowMarkerId}-${spotlight})`}
spotlight={spotlight}
Expand Down
2 changes: 1 addition & 1 deletion src/web/topic/components/Edge/StandaloneEdge.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { useIsGraphPartSelected } from "@/web/view/currentViewStore/store";
const convertToStandaloneFlowEdge = (edge: Edge, selected: boolean): EdgeProps => {
return {
id: edge.id,
data: edge.data,
data: { ...edge.data, elkSections: [] },
label: edge.label,
selected: selected,
source: edge.source,
Expand Down
154 changes: 154 additions & 0 deletions src/web/topic/components/Edge/svgPathDrawer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
import { getBezierPath } from "reactflow";

import { throwError } from "@/common/errorHandling";
import { EdgeProps } from "@/web/topic/components/Diagram/Diagram";

/**
* If `drawSimpleEdgePaths` is true, draw a simple bezier between the source and target.
* Otherwise, use the ELK layout's bend points to draw a more complex path.
*
* TODO: modify complex-path algorithm such that curve has vertical slopes at start and end points.
* The lack of this implementation is the main reason why the `drawSimpleEdgePaths` option exists.
* Tried inserting a control point directly below `startPoint` and above `endPoint`, and that
* resulted in vertical slopes, but the curve to/from the next bend points became jagged.
*/
export const getPathDefinitionForEdge = (flowEdge: EdgeProps, drawSimpleEdgePaths: boolean) => {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- reactflow types data as nullable but we always pass it, so it should always be here
const { elkSections } = flowEdge.data!;
const elkSection = elkSections[0];
const bendPoints = elkSection?.bendPoints;
const firstBendPoint = bendPoints?.[0];
const lastBendPoint = bendPoints?.[bendPoints.length - 1];

const missingBendPoints =
elkSection === undefined ||
bendPoints === undefined ||
firstBendPoint === undefined ||
lastBendPoint === undefined;

if (drawSimpleEdgePaths || missingBendPoints) {
const [pathDefinition, labelX, labelY] = getBezierPath({
sourceX: flowEdge.sourceX,
sourceY: flowEdge.sourceY,
sourcePosition: flowEdge.sourcePosition,
targetX: flowEdge.targetX,
targetY: flowEdge.targetY,
targetPosition: flowEdge.targetPosition,
});

return { pathDefinition, labelX, labelY };
}

if (elkSections.length > 1) {
return throwError("No implementation yet for edge with multiple sections", flowEdge);
}

/**
* TODO: start/end would ideally use `flowEdge.source`/`.target` because those are calculated
* to include the size of the handles, so the path actually points to the edge of the handle
* rather than the edge of the node.
*
* However: the layout's bend points near the start/end might be too high/low and need to shift
* down/up in order to make the curve smooth when pointing to the node handles.
*/
const startPoint = elkSection.startPoint;
const endPoint = elkSection.endPoint;
const points = [startPoint, ...bendPoints, endPoint];

// Awkwardly need to filter out duplicates because of a bug in the layout algorithm.
// Should be able to remove this logic after https://github.com/eclipse/elk/issues/1085.
const pointsWithoutDuplicates = points.filter((point, index) => {
const pointBefore = points[index - 1];
if (index === 0 || pointBefore === undefined) return true;
return pointBefore.x !== point.x || pointBefore.y !== point.y;
});
const bendPointsWithoutDuplicates = pointsWithoutDuplicates.slice(1, -1);

const pathDefinition = drawBezierCurvesFromPoints(
startPoint,
bendPointsWithoutDuplicates,
endPoint,
);

const { x: labelX, y: labelY } = getPathMidpoint(pathDefinition);
return { pathDefinition, labelX, labelY };
};

const getPathMidpoint = (pathDefinition: string) => {
// This seems like a wild solution to calculate label position based on svg path,
// but on average, this takes 0.05ms per edge; 100 edges would take 5ms, which seems plenty fast enough.
// Note: got this from github copilot suggestion.
// Also tried reusing one `path` element globally, re-setting its `d` attribute each time,
// but that didn't seem to save any significant amount of performance.
const path = document.createElementNS("http://www.w3.org/2000/svg", "path");
path.setAttribute("d", pathDefinition);
const pathLength = path.getTotalLength();

return path.getPointAtLength(pathLength / 2);
};

interface Point {
x: number;
y: number;
}

/**
* Copied mostly from https://github.com/eclipse/elk/issues/848#issuecomment-1248084547
*
* Could refactor to ensure everything is safer, but logic seems fine enough to trust.
*/
/* eslint-disable @typescript-eslint/no-non-null-assertion */
/* eslint-disable functional/no-let */
/* eslint-disable functional/no-loop-statements */
/* eslint-disable functional/immutable-data */
const drawBezierCurvesFromPoints = (
startPoint: Point,
bendPoints: Point[],
endPoint: Point,
): string => {
// If no bend points, we should've drawn a simple curve before getting here
if (bendPoints.length === 0) throwError("Expected bend points", startPoint, bendPoints, endPoint);

// not sure why end is treated as a control point, but algo seems to work and not sure a better name
const controlPoints = [...bendPoints, endPoint];

const path = [`M ${ptToStr(startPoint)}`];

// if there are groups of 3 points, draw cubic bezier curves
if (controlPoints.length % 3 === 0) {
for (let i = 0; i < controlPoints.length; i = i + 3) {
const [c1, c2, p] = controlPoints.slice(i, i + 3);
path.push(`C ${ptToStr(c1!)}, ${ptToStr(c2!)}, ${ptToStr(p!)}`);
}
}
// if there's an even number of points, draw quadratic curves
else if (controlPoints.length % 2 === 0) {
for (let i = 0; i < controlPoints.length; i = i + 2) {
const [c, p] = controlPoints.slice(i, i + 2);
path.push(`Q ${ptToStr(c!)}, ${ptToStr(p!)}`);
}
}
// else, add missing points and try again
// https://stackoverflow.com/a/72577667/1010492
else {
for (let i = controlPoints.length - 3; i >= 2; i = i - 2) {
const missingPoint = midPoint(controlPoints[i - 1]!, controlPoints[i]!);
controlPoints.splice(i, 0, missingPoint);
}
const newBendPoints = controlPoints.slice(0, -1);
return drawBezierCurvesFromPoints(startPoint, newBendPoints, endPoint);
}

return path.join(" ");
};

export const midPoint = (pt1: Point, pt2: Point) => {
return {
x: (pt2.x + pt1.x) / 2,
y: (pt2.y + pt1.y) / 2,
};
};

export const ptToStr = ({ x, y }: Point) => {
return `${x} ${y}`;
};
30 changes: 26 additions & 4 deletions src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
PhotoCamera,
PowerInput,
Route,
ShowChart,
SsidChart,
Upload,
WbTwilight,
Expand Down Expand Up @@ -64,7 +65,12 @@ import {
useLayoutThoroughness,
useMinimizeEdgeCrossings,
} from "@/web/view/currentViewStore/layout";
import { resetView, useFormat } from "@/web/view/currentViewStore/store";
import {
resetView,
setDrawSimpleEdgePaths,
useDrawSimpleEdgePaths,
useFormat,
} from "@/web/view/currentViewStore/store";
import { resetQuickViews } from "@/web/view/quickViewStore/store";
import {
toggleFillNodesWithColor,
Expand Down Expand Up @@ -124,15 +130,19 @@ export const MoreActionsDrawer = ({
const format = useFormat();
const isTableActive = format === "table";

const unrestrictedEditing = useUnrestrictedEditing();
const flashlightMode = useFlashlightMode();
const readonlyMode = useReadonlyMode();

const showImpliedEdges = useShowImpliedEdges();
const showProblemCriterionSolutionEdges = useShowProblemCriterionSolutionEdges();
const unrestrictedEditing = useUnrestrictedEditing();
const drawSimpleEdgePaths = useDrawSimpleEdgePaths();

const forceNodesIntoLayers = useForceNodesIntoLayers();
const layerNodeIslandsTogether = useLayerNodeIslandsTogether();
const minimizeEdgeCrossings = useMinimizeEdgeCrossings();
const flashlightMode = useFlashlightMode();
const readonlyMode = useReadonlyMode();
const layoutThoroughness = useLayoutThoroughness();

const fillNodesWithColor = useFillNodesWithColor();
const indicateWhenNodeForcedToShow = useIndicateWhenNodeForcedToShow();

Expand Down Expand Up @@ -307,6 +317,18 @@ export const MoreActionsDrawer = ({
>
<Grid4x4 />
</ToggleButton>
<ToggleButton
value={drawSimpleEdgePaths}
title="Draw edges using simple paths"
aria-label="Draw edges using simple paths"
color="primary"
size="small"
selected={drawSimpleEdgePaths}
onClick={() => setDrawSimpleEdgePaths(!drawSimpleEdgePaths)}
sx={{ borderRadius: "50%", border: "0" }}
>
<ShowChart />
</ToggleButton>
<ToggleButton
value={forceNodesIntoLayers}
title="Force nodes into layers"
Expand Down
Loading

0 comments on commit 4e73246

Please sign in to comment.