Skip to content

Commit

Permalink
feat: add option to include edge labels in layout
Browse files Browse the repository at this point in the history
so that they avoid each other.

wanted to always do this (i.e. without an option), but ELK treats labels
as nodes, creating inconsistent spacing between node layers, and
creating more node layers total.

this sometimes results in more-chaotic layouts than otherwise, so an
option is added for it.

see eclipse/elk#1092 for more information.
  • Loading branch information
keyserj committed Nov 24, 2024
1 parent 4e73246 commit f4418c3
Show file tree
Hide file tree
Showing 7 changed files with 99 additions and 10 deletions.
1 change: 1 addition & 0 deletions src/web/topic/components/Edge/StandaloneEdge.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { useIsGraphPartSelected } from "@/web/view/currentViewStore/store";
const convertToStandaloneFlowEdge = (edge: Edge, selected: boolean): EdgeProps => {
return {
id: edge.id,
// don't provide a position for the label, so it defaults to being placed between the two nodes
data: { ...edge.data, elkSections: [] },
label: edge.label,
selected: selected,
Expand Down
12 changes: 10 additions & 2 deletions src/web/topic/components/Edge/svgPathDrawer.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { getBezierPath } from "reactflow";

import { throwError } from "@/common/errorHandling";
import { scalePxViaDefaultFontSize } from "@/pages/_document.page";
import { EdgeProps } from "@/web/topic/components/Diagram/Diagram";
import { labelWidthPx } from "@/web/topic/utils/layout";

/**
* If `drawSimpleEdgePaths` is true, draw a simple bezier between the source and target.
Expand All @@ -14,7 +16,7 @@ import { EdgeProps } from "@/web/topic/components/Diagram/Diagram";
*/
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 { elkLabel, elkSections } = flowEdge.data!;
const elkSection = elkSections[0];
const bendPoints = elkSection?.bendPoints;
const firstBendPoint = bendPoints?.[0];
Expand All @@ -27,6 +29,7 @@ export const getPathDefinitionForEdge = (flowEdge: EdgeProps, drawSimpleEdgePath
lastBendPoint === undefined;

if (drawSimpleEdgePaths || missingBendPoints) {
// TODO: probably ideally would draw this path through the ELK label position if that's provided
const [pathDefinition, labelX, labelY] = getBezierPath({
sourceX: flowEdge.sourceX,
sourceY: flowEdge.sourceY,
Expand Down Expand Up @@ -70,7 +73,12 @@ export const getPathDefinitionForEdge = (flowEdge: EdgeProps, drawSimpleEdgePath
endPoint,
);

const { x: labelX, y: labelY } = getPathMidpoint(pathDefinition);
const { x: labelX, y: labelY } = elkLabel
? // Note: ELK label position is moved left by half of its width in order to center it.
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
{ x: elkLabel.x! + 0.5 * scalePxViaDefaultFontSize(labelWidthPx), y: elkLabel.y! }
: getPathMidpoint(pathDefinition);

return { pathDefinition, labelX, labelY };
};

Expand Down
21 changes: 21 additions & 0 deletions src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
AutoAwesomeMotion,
AutoStoriesOutlined,
Build,
Close,
Expand Down Expand Up @@ -57,9 +58,11 @@ import {
} from "@/web/view/currentViewStore/filter";
import {
setLayoutThoroughness,
toggleAvoidEdgeLabelOverlap,
toggleForceNodesIntoLayers,
toggleLayerNodeIslandsTogether,
toggleMinimizeEdgeCrossings,
useAvoidEdgeLabelOverlap,
useForceNodesIntoLayers,
useLayerNodeIslandsTogether,
useLayoutThoroughness,
Expand Down Expand Up @@ -141,6 +144,7 @@ export const MoreActionsDrawer = ({
const forceNodesIntoLayers = useForceNodesIntoLayers();
const layerNodeIslandsTogether = useLayerNodeIslandsTogether();
const minimizeEdgeCrossings = useMinimizeEdgeCrossings();
const avoidEdgeLabelOverlap = useAvoidEdgeLabelOverlap();
const layoutThoroughness = useLayoutThoroughness();

const fillNodesWithColor = useFillNodesWithColor();
Expand Down Expand Up @@ -290,6 +294,7 @@ export const MoreActionsDrawer = ({
<>
<Divider>Diagram Config</Divider>

{/* General showing/hiding diagram config */}
<ListItem disablePadding={false}>
<ToggleButton
value={showImpliedEdges}
Expand Down Expand Up @@ -329,6 +334,10 @@ export const MoreActionsDrawer = ({
>
<ShowChart />
</ToggleButton>
</ListItem>

{/* Layout config */}
<ListItem disablePadding={false}>
<ToggleButton
value={forceNodesIntoLayers}
title="Force nodes into layers"
Expand Down Expand Up @@ -365,6 +374,18 @@ export const MoreActionsDrawer = ({
>
<SsidChart />
</ToggleButton>
<ToggleButton
value={avoidEdgeLabelOverlap}
title="Avoid edge label overlap"
aria-label="Avoid edge label overlap"
color="primary"
size="small"
selected={avoidEdgeLabelOverlap}
onClick={() => toggleAvoidEdgeLabelOverlap(!avoidEdgeLabelOverlap)}
sx={{ borderRadius: "50%", border: "0" }}
>
<AutoAwesomeMotion />
</ToggleButton>
</ListItem>

<ListItem disablePadding={false}>
Expand Down
8 changes: 6 additions & 2 deletions src/web/topic/hooks/diagramHooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
import { isNode } from "@/web/topic/utils/graph";
import { LayoutedGraph, layout } from "@/web/topic/utils/layout";
import {
useAvoidEdgeLabelOverlap,
useForceNodesIntoLayers,
useLayerNodeIslandsTogether,
useLayoutThoroughness,
Expand All @@ -21,6 +22,7 @@ export const useLayoutedDiagram = (diagram: Diagram) => {
const forceNodesIntoLayers = useForceNodesIntoLayers();
const layerNodeIslandsTogether = useLayerNodeIslandsTogether();
const minimizeEdgeCrossings = useMinimizeEdgeCrossings();
const avoidEdgeLabelOverlap = useAvoidEdgeLabelOverlap();
const thoroughness = useLayoutThoroughness();

// re-layout if this changes
Expand All @@ -31,6 +33,7 @@ export const useLayoutedDiagram = (diagram: Diagram) => {
String(forceNodesIntoLayers),
String(layerNodeIslandsTogether),
String(minimizeEdgeCrossings),
String(avoidEdgeLabelOverlap),
String(thoroughness),
)
.join();
Expand All @@ -50,6 +53,7 @@ export const useLayoutedDiagram = (diagram: Diagram) => {
forceNodesIntoLayers,
layerNodeIslandsTogether,
minimizeEdgeCrossings,
avoidEdgeLabelOverlap,
thoroughness,
);
setLayoutedGraph(newLayoutedGraph);
Expand Down Expand Up @@ -85,11 +89,11 @@ export const useLayoutedDiagram = (diagram: Diagram) => {
(layoutedEdge) => layoutedEdge.id === edge.id,
);
if (!layoutedEdge) return null;
const { elkSections } = layoutedEdge;
const { elkLabel, elkSections } = layoutedEdge;

return {
...edge,
data: { ...edge.data, elkSections },
data: { ...edge.data, elkLabel, elkSections },
selected: edge.id === selectedGraphPart?.id, // add selected here because react flow uses it (as opposed to our custom components, which can rely on selectedGraphPart hook independently)
} as PositionedEdge;
})
Expand Down
43 changes: 37 additions & 6 deletions src/web/topic/utils/layout.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import ELK, { ElkEdgeSection, ElkNode, LayoutOptions } from "elkjs";
import ELK, { ElkEdgeSection, ElkLabel, ElkNode, LayoutOptions } from "elkjs";

import { throwError } from "@/common/errorHandling";
import { NodeType, nodeTypes } from "@/common/node";
Expand All @@ -11,6 +11,11 @@ import { edges as nodeEdges } from "@/web/topic/utils/node";
export type Orientation = "DOWN" | "UP" | "RIGHT" | "LEFT";
export const orientation: Orientation = "DOWN" as Orientation; // not constant to allow potential other orientations in the future, and keeping code that currently exists for handling "LEFT" orientation

/**
* Roughly accurate, using the average-length "addresses" label
*/
export const labelWidthPx = 85;

const priorities = Object.fromEntries(nodeTypes.map((type, index) => [type, index.toString()])) as {
[type in NodeType]: string;
};
Expand Down Expand Up @@ -138,6 +143,10 @@ interface LayoutedNode {

export interface LayoutedEdge {
id: string;
/**
* Will be undefined when edge labels are excluded from the layout calculation
*/
elkLabel?: ElkLabel;
elkSections: ElkEdgeSection[];
}

Expand All @@ -161,11 +170,12 @@ const parseElkjsOutput = (layoutedGraph: ElkNode): LayoutedGraph => {
});

const layoutedEdges = edges.map((edge) => {
const elkLabel = edge.labels?.[0]; // allowed to be missing if we're excluding labels from the layout calc
const elkSections = edge.sections;
if (!elkSections) {
return throwError("edge missing sections in layout", edge);
}
return { id: edge.id, elkSections };
return { id: edge.id, elkLabel, elkSections };
});

return { layoutedNodes, layoutedEdges };
Expand All @@ -176,6 +186,7 @@ export const layout = async (
partition: boolean,
layerNodeIslandsTogether: boolean,
minimizeEdgeCrossings: boolean,
avoidEdgeLabelOverlap: boolean,
thoroughness: number,
): Promise<LayoutedGraph> => {
const { nodes, edges } = diagram;
Expand All @@ -192,11 +203,13 @@ export const layout = async (
// allows grouping nodes by type (within a layer) when nodes are sorted by type
// tried using `position` to do this but it doesn't group nodes near their source node
"elk.layered.considerModelOrder.strategy": "PREFER_EDGES",
// these spacings are just what roughly seem to look good
// These spacings are just what roughly seem to look good.
// Note: Edge labels are given layers like nodes, so we need to halve the spacing between layers
// when including labels in the layout, in order to keep the same distance between nodes.
"elk.layered.spacing.nodeNodeBetweenLayers":
orientation === "DOWN"
? scalePxViaDefaultFontSize(130).toString()
: scalePxViaDefaultFontSize(220).toString(),
? scalePxViaDefaultFontSize(avoidEdgeLabelOverlap ? 65 : 130).toString()
: scalePxViaDefaultFontSize(avoidEdgeLabelOverlap ? 55 : 110).toString(),
"elk.spacing.nodeNode":
orientation === "DOWN"
? scalePxViaDefaultFontSize(20).toString()
Expand Down Expand Up @@ -250,7 +263,25 @@ export const layout = async (
edges: edges
.toSorted((edge1, edge2) => compareEdges(edge1, edge2, nodes))
.map((edge) => {
return { id: edge.id, sources: [edge.source], targets: [edge.target] };
return {
id: edge.id,
sources: [edge.source],
targets: [edge.target],
labels: avoidEdgeLabelOverlap
? [
{
text: edge.label,
layoutOptions: {
"edgeLabels.inline": "true",
},
width: scalePxViaDefaultFontSize(labelWidthPx),
// Give labels 0 height so they don't create more space between node layers;
// layout still avoids overlap with labels based on their widths when they have 0 height.
height: 0,
},
]
: undefined,
};
}),
};

Expand Down
10 changes: 10 additions & 0 deletions src/web/view/currentViewStore/layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ export const useMinimizeEdgeCrossings = () => {
return useCurrentViewStore((state) => state.minimizeEdgeCrossings);
};

export const useAvoidEdgeLabelOverlap = () => {
return useCurrentViewStore((state) => state.avoidEdgeLabelOverlap);
};

export const useLayoutThoroughness = () => {
return useCurrentViewStore((state) => state.layoutThoroughness);
};
Expand All @@ -37,6 +41,12 @@ export const toggleMinimizeEdgeCrossings = (minimize: boolean) => {
emitter.emit("changedLayoutConfig");
};

export const toggleAvoidEdgeLabelOverlap = (avoid: boolean) => {
useCurrentViewStore.setState({ avoidEdgeLabelOverlap: avoid });

emitter.emit("changedLayoutConfig");
};

export const setLayoutThoroughness = (thoroughness: number) => {
useCurrentViewStore.setState({ layoutThoroughness: thoroughness });

Expand Down
14 changes: 14 additions & 0 deletions src/web/view/currentViewStore/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,19 @@ export interface ViewState {
forceNodesIntoLayers: boolean;
layerNodeIslandsTogether: boolean;
minimizeEdgeCrossings: boolean;
/**
* Ideally we'd always avoid edge label overlap, because the diagram seems more chaotic when
* there's overlap, but unfortunately including labels in the layout create a few problems:
* 1. spacing between node layers can be consistent,
* 2. more node layers can be created than otherwise.
*
* These problems both result in a layout that often seems even more chaotic than just
* accepting edge overlap.
*
* This ELK ticket exists to address these issues, and if resolved, we should be able to
* unconditionally include edge labels in layout: https://github.com/eclipse/elk/issues/1092
*/
avoidEdgeLabelOverlap: boolean;
layoutThoroughness: number;
}

Expand Down Expand Up @@ -94,6 +107,7 @@ export const initialViewState: ViewState = {
forceNodesIntoLayers: true,
layerNodeIslandsTogether: false,
minimizeEdgeCrossings: true,
avoidEdgeLabelOverlap: false,
layoutThoroughness: 100, // by default, prefer keeping parents close to children over keeping node types together
};

Expand Down

0 comments on commit f4418c3

Please sign in to comment.