From f4418c30e9ffa955942495dce2d3db5b6616a9d9 Mon Sep 17 00:00:00 2001 From: Joel Keyser Date: Sun, 24 Nov 2024 16:58:51 -0600 Subject: [PATCH] feat: add option to include edge labels in layout 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 https://github.com/eclipse/elk/issues/1092 for more information. --- .../topic/components/Edge/StandaloneEdge.tsx | 1 + .../topic/components/Edge/svgPathDrawer.ts | 12 +++++- .../TopicWorkspace/MoreActionsDrawer.tsx | 21 +++++++++ src/web/topic/hooks/diagramHooks.ts | 8 +++- src/web/topic/utils/layout.ts | 43 ++++++++++++++++--- src/web/view/currentViewStore/layout.ts | 10 +++++ src/web/view/currentViewStore/store.ts | 14 ++++++ 7 files changed, 99 insertions(+), 10 deletions(-) diff --git a/src/web/topic/components/Edge/StandaloneEdge.tsx b/src/web/topic/components/Edge/StandaloneEdge.tsx index 881155f4..90dacf8c 100644 --- a/src/web/topic/components/Edge/StandaloneEdge.tsx +++ b/src/web/topic/components/Edge/StandaloneEdge.tsx @@ -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, diff --git a/src/web/topic/components/Edge/svgPathDrawer.ts b/src/web/topic/components/Edge/svgPathDrawer.ts index ef5d1ae6..d9077186 100644 --- a/src/web/topic/components/Edge/svgPathDrawer.ts +++ b/src/web/topic/components/Edge/svgPathDrawer.ts @@ -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. @@ -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]; @@ -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, @@ -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 }; }; diff --git a/src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx b/src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx index f12c76ff..abb8a5c8 100644 --- a/src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx +++ b/src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx @@ -1,4 +1,5 @@ import { + AutoAwesomeMotion, AutoStoriesOutlined, Build, Close, @@ -57,9 +58,11 @@ import { } from "@/web/view/currentViewStore/filter"; import { setLayoutThoroughness, + toggleAvoidEdgeLabelOverlap, toggleForceNodesIntoLayers, toggleLayerNodeIslandsTogether, toggleMinimizeEdgeCrossings, + useAvoidEdgeLabelOverlap, useForceNodesIntoLayers, useLayerNodeIslandsTogether, useLayoutThoroughness, @@ -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(); @@ -290,6 +294,7 @@ export const MoreActionsDrawer = ({ <> Diagram Config + {/* General showing/hiding diagram config */} + + + {/* Layout config */} + + toggleAvoidEdgeLabelOverlap(!avoidEdgeLabelOverlap)} + sx={{ borderRadius: "50%", border: "0" }} + > + + diff --git a/src/web/topic/hooks/diagramHooks.ts b/src/web/topic/hooks/diagramHooks.ts index b0d7d9ac..6b7df907 100644 --- a/src/web/topic/hooks/diagramHooks.ts +++ b/src/web/topic/hooks/diagramHooks.ts @@ -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, @@ -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 @@ -31,6 +33,7 @@ export const useLayoutedDiagram = (diagram: Diagram) => { String(forceNodesIntoLayers), String(layerNodeIslandsTogether), String(minimizeEdgeCrossings), + String(avoidEdgeLabelOverlap), String(thoroughness), ) .join(); @@ -50,6 +53,7 @@ export const useLayoutedDiagram = (diagram: Diagram) => { forceNodesIntoLayers, layerNodeIslandsTogether, minimizeEdgeCrossings, + avoidEdgeLabelOverlap, thoroughness, ); setLayoutedGraph(newLayoutedGraph); @@ -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; }) diff --git a/src/web/topic/utils/layout.ts b/src/web/topic/utils/layout.ts index 4cc94d52..82e8c1c4 100644 --- a/src/web/topic/utils/layout.ts +++ b/src/web/topic/utils/layout.ts @@ -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"; @@ -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; }; @@ -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[]; } @@ -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 }; @@ -176,6 +186,7 @@ export const layout = async ( partition: boolean, layerNodeIslandsTogether: boolean, minimizeEdgeCrossings: boolean, + avoidEdgeLabelOverlap: boolean, thoroughness: number, ): Promise => { const { nodes, edges } = diagram; @@ -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() @@ -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, + }; }), }; diff --git a/src/web/view/currentViewStore/layout.ts b/src/web/view/currentViewStore/layout.ts index ea357581..2231df85 100644 --- a/src/web/view/currentViewStore/layout.ts +++ b/src/web/view/currentViewStore/layout.ts @@ -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); }; @@ -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 }); diff --git a/src/web/view/currentViewStore/store.ts b/src/web/view/currentViewStore/store.ts index db8f3c71..6c897578 100644 --- a/src/web/view/currentViewStore/store.ts +++ b/src/web/view/currentViewStore/store.ts @@ -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; } @@ -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 };