diff --git a/src/web/topic/components/Diagram/Diagram.tsx b/src/web/topic/components/Diagram/Diagram.tsx index 2670cbf1..ae6ecad9 100644 --- a/src/web/topic/components/Diagram/Diagram.tsx +++ b/src/web/topic/components/Diagram/Diagram.tsx @@ -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"; @@ -71,13 +70,13 @@ const edgeTypes: Record<"FlowEdge", ComponentType> = { 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) => { diff --git a/src/web/topic/components/Edge/ScoreEdge.tsx b/src/web/topic/components/Edge/ScoreEdge.tsx index d27ee030..b05153e2 100644 --- a/src/web/topic/components/Edge/ScoreEdge.tsx +++ b/src/web/topic/components/Edge/ScoreEdge.tsx @@ -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"; @@ -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, @@ -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}`; @@ -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); @@ -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 = ( { return { id: edge.id, - data: edge.data, + data: { ...edge.data, elkSections: [] }, label: edge.label, selected: selected, source: edge.source, diff --git a/src/web/topic/components/Edge/svgPathDrawer.ts b/src/web/topic/components/Edge/svgPathDrawer.ts new file mode 100644 index 00000000..ef5d1ae6 --- /dev/null +++ b/src/web/topic/components/Edge/svgPathDrawer.ts @@ -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}`; +}; diff --git a/src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx b/src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx index 95b3c16d..f12c76ff 100644 --- a/src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx +++ b/src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx @@ -14,6 +14,7 @@ import { PhotoCamera, PowerInput, Route, + ShowChart, SsidChart, Upload, WbTwilight, @@ -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, @@ -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(); @@ -307,6 +317,18 @@ export const MoreActionsDrawer = ({ > + setDrawSimpleEdgePaths(!drawSimpleEdgePaths)} + sx={{ borderRadius: "50%", border: "0" }} + > + + { .join(); const [prevDiagramHash, setPrevDiagramHash] = useState(null); - const [layoutedNodes, setLayoutedNodes] = useState(null); + const [layoutedGraph, setLayoutedGraph] = useState(null); const [hasNewLayout, setHasNewLayout] = useState(false); const selectedGraphPart = useSelectedGraphPart(); @@ -40,26 +45,28 @@ export const useLayoutedDiagram = (diagram: Diagram) => { setPrevDiagramHash(diagramHash); const layoutDiagram = async () => { - const newLayoutedNodes = await layout( + const newLayoutedGraph = await layout( diagram, forceNodesIntoLayers, layerNodeIslandsTogether, minimizeEdgeCrossings, thoroughness, ); - setLayoutedNodes(newLayoutedNodes); + setLayoutedGraph(newLayoutedGraph); setHasNewLayout(true); }; void layoutDiagram(); } - if (!layoutedNodes) return { layoutedDiagram: null, hasNewLayout, setHasNewLayout }; + if (!layoutedGraph) return { layoutedDiagram: null, hasNewLayout, setHasNewLayout }; const layoutedDiagram: PositionedDiagram = { ...diagram, nodes: diagram.nodes .map((node) => { - const layoutedNode = layoutedNodes.find((layoutedNode) => layoutedNode.id === node.id); + const layoutedNode = layoutedGraph.layoutedNodes.find( + (layoutedNode) => layoutedNode.id === node.id, + ); if (!layoutedNode) return null; return { @@ -72,7 +79,21 @@ export const useLayoutedDiagram = (diagram: Diagram) => { }; }) .filter((node): node is PositionedNode => node !== null), - edges: diagram.edges.map((edge) => ({ ...edge, selected: edge.id === selectedGraphPart?.id })), + edges: diagram.edges + .map((edge) => { + const layoutedEdge = layoutedGraph.layoutedEdges.find( + (layoutedEdge) => layoutedEdge.id === edge.id, + ); + if (!layoutedEdge) return null; + const { elkSections } = layoutedEdge; + + return { + ...edge, + data: { ...edge.data, 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; + }) + .filter((edge): edge is PositionedEdge => edge !== null), }; // loading a new diagram but layout hasn't finished yet diff --git a/src/web/topic/utils/diagram.ts b/src/web/topic/utils/diagram.ts index 51074662..3428c54d 100644 --- a/src/web/topic/utils/diagram.ts +++ b/src/web/topic/utils/diagram.ts @@ -1,4 +1,5 @@ import { Edge, Node } from "@/web/topic/utils/graph"; +import { LayoutedEdge } from "@/web/topic/utils/layout"; export interface Diagram { nodes: Node[]; @@ -13,6 +14,12 @@ export interface PositionedNode extends Node { selected: boolean; } +export interface PositionedEdge extends Edge { + data: Edge["data"] & Omit; // edge already has id directly on it + selected: boolean; +} + export interface PositionedDiagram extends Diagram { nodes: PositionedNode[]; + edges: PositionedEdge[]; } diff --git a/src/web/topic/utils/layout.ts b/src/web/topic/utils/layout.ts index 6ee019ad..4cc94d52 100644 --- a/src/web/topic/utils/layout.ts +++ b/src/web/topic/utils/layout.ts @@ -1,5 +1,6 @@ -import ELK, { ElkNode, LayoutOptions } from "elkjs"; +import ELK, { ElkEdgeSection, ElkNode, LayoutOptions } from "elkjs"; +import { throwError } from "@/common/errorHandling"; import { NodeType, nodeTypes } from "@/common/node"; import { scalePxViaDefaultFontSize } from "@/pages/_document.page"; import { nodeHeightPx, nodeWidthPx } from "@/web/topic/components/Node/EditableNode.styles"; @@ -129,28 +130,62 @@ const calculateNodeHeight = (label: string) => { return nodeHeightPx + scalePxViaDefaultFontSize(additionalHeightPx); }; -export interface NodePosition { +interface LayoutedNode { id: string; x: number; y: number; } -// TODO?: feels a little weird to have layout know Ameliorate domain, like DiagramType +export interface LayoutedEdge { + id: string; + elkSections: ElkEdgeSection[]; +} + +export interface LayoutedGraph { + layoutedNodes: LayoutedNode[]; + layoutedEdges: LayoutedEdge[]; +} + +const parseElkjsOutput = (layoutedGraph: ElkNode): LayoutedGraph => { + const { children, edges } = layoutedGraph; + if (!children || !edges) { + return throwError("layouted graph missing children or edges", layoutedGraph); + } + + const layoutedNodes = children.map((node) => { + const { x, y } = node; + if (x === undefined || y === undefined) { + return throwError("node missing x or y in layout", node); + } + return { id: node.id, x, y }; + }); + + const layoutedEdges = edges.map((edge) => { + const elkSections = edge.sections; + if (!elkSections) { + return throwError("edge missing sections in layout", edge); + } + return { id: edge.id, elkSections }; + }); + + return { layoutedNodes, layoutedEdges }; +}; + export const layout = async ( diagram: Diagram, partition: boolean, layerNodeIslandsTogether: boolean, minimizeEdgeCrossings: boolean, thoroughness: number, -): Promise => { +): Promise => { const { nodes, edges } = diagram; // see support layout options at https://www.eclipse.org/elk/reference/algorithms/org-eclipse-elk-layered.html const layoutOptions: LayoutOptions = { algorithm: "layered", "elk.direction": orientation, - // results in a more centered layout - seems to moreso ignore edges when laying out - "elk.edgeRouting": "POLYLINE", + // results in edges curving more around other things + "elk.edgeRouting": "SPLINES", "elk.spacing.edgeEdge": "0", // other placement strategies seem to either spread nodes out too much, or ignore edges between layers "elk.layered.nodePlacement.strategy": "NETWORK_SIMPLEX", @@ -185,6 +220,9 @@ export const layout = async ( // Elk defaults to minimizing, but sometimes this makes nodes in the same layer be spread out a lot; // turning this off prioritizes nodes in the same layer being close together at the cost of more edge crossings. "crossingMinimization.strategy": minimizeEdgeCrossings ? "LAYER_SWEEP" : "NONE", + // Edges should all connect to a node at the same point, rather than different points + // this is particularly needed when drawing edge paths based on this layout algorithm's output. + mergeEdges: "true", }; const graph: ElkNode = { @@ -221,26 +259,22 @@ export const layout = async ( try { const layoutedGraph = await elk.layout(graph, { layoutOptions, - logging: true, - measureExecutionTime: true, + // log-related options throw error with SPLINES edge routing somehow; see https://github.com/kieler/elkjs/issues/309 + // logging: true, + // measureExecutionTime: true, }); - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return layoutedGraph.children!.map((node) => { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return { id: node.id, x: node.x!, y: node.y! }; - }); + const parsedGraph = parseElkjsOutput(layoutedGraph); + return parsedGraph; } catch (error) { const layoutedGraph = await elk.layout(graph, { layoutOptions: { ...layoutOptions, "elk.partitioning.activate": "false" }, - logging: true, - measureExecutionTime: true, + // log-related options throw error with SPLINES edge routing somehow; see https://github.com/kieler/elkjs/issues/309 + // logging: true, + // measureExecutionTime: true, }); - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return layoutedGraph.children!.map((node) => { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return { id: node.id, x: node.x!, y: node.y! }; - }); + const parsedGraph = parseElkjsOutput(layoutedGraph); + return parsedGraph; } }; diff --git a/src/web/view/currentViewStore/store.ts b/src/web/view/currentViewStore/store.ts index dfc4b830..db8f3c71 100644 --- a/src/web/view/currentViewStore/store.ts +++ b/src/web/view/currentViewStore/store.ts @@ -41,6 +41,20 @@ export interface ViewState { * The criteria table can be used to see these relationships more clearly. */ showProblemCriterionSolutionEdges: boolean; + /** + * Draw a simple cubic bezier for edge paths, rather than drawing paths that are output from the + * layout algorithm. + * + * This only exists because the layout output does not result in vertical slopes at the start and + * end points of each path. These vertical slopes often seem desirable, because they go with the + * top-down flow of the diagram. + * + * TODO: modify algorithm for drawing path from layout algorithm, such that it has vertical slopes + * at the start and end points of each path. + * + * Note: this doesn't affect layout at all, just what's shown. + */ + drawSimpleEdgePaths: boolean; // layout forceNodesIntoLayers: boolean; @@ -75,6 +89,7 @@ export const initialViewState: ViewState = { showImpliedEdges: false, showProblemCriterionSolutionEdges: true, + drawSimpleEdgePaths: true, forceNodesIntoLayers: true, layerNodeIslandsTogether: false, @@ -136,6 +151,10 @@ export const useFormat = () => { return useCurrentViewStore((state) => state.format); }; +export const useDrawSimpleEdgePaths = () => { + return useCurrentViewStore((state) => state.drawSimpleEdgePaths); +}; + export const useCanGoBackForward = () => { const temporalStore = useTemporalStore(); @@ -158,6 +177,10 @@ export const setFormat = (format: Format) => { useCurrentViewStore.setState({ format }, false, "setFormat"); }; +export const setDrawSimpleEdgePaths = (draw: boolean) => { + useCurrentViewStore.setState({ drawSimpleEdgePaths: draw }, false, "setDrawSimpleEdgePaths"); +}; + /** * Use the initialState to fill in missing values. *