diff --git a/CHANGELOG.md b/CHANGELOG.md index 81e8097..203afaf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,30 @@ platform API version. ### Changed +- **After a drop, the routes an edge cannot keep are drawn by libavoid, with + the edges on one port as one shared path.** The layout draws the edges into + a port as a trunk; the built-in router gives every edge its own channel and + drew a comb, and libavoid did the same because every edge on a port was + started at its own staggered point. Now they start at one point and + libavoid keeps them on one path for as long as they go the same way, a + trunk with a junction. The half-nudge kink the stagger was for is taken + out after routing. libavoid is the router from now on; the built-in one + remains the fallback when libavoid is unavailable or cannot solve an edge, + the choice for a reroute of every edge in a diagram of more than 200 + edges, and available throughout with `WORKFLOW_DIAGRAM_ROUTER=manhattan`. +- **The edges of a node being dragged are not drawn while it moves.** They + come back as soon as a route for the node's new place is in the model, as + a schematic editor redraws a block's nets after it is placed; before, they + trailed the pointer on their old routes and jumped at the drop. A move by + arrow key hides nothing, and a cancelled drag draws them again at once. A + node held still mid-drag keeps its edges hidden: a drag with no move for 5s + is taken as one that never finishes. A bounds pass mid-drag, which draws + the server's model in the hidden layer with the node still at its old + place, no longer draws the edges again for the rest of the drag. If no + route arrives within 1.5s of the drop the edges are drawn as they were. +- The server logs one `[dialogram perf] reroute` line per reroute it + persists: edges kept, edges routed, the router and the time taken. + - **The Java ELK server is rebuilt on ELK 0.10.0, and elkjs is pinned to 0.10.0**, so a graph lays out the same on either engine; before, the server's ELK 0.8.1 put some graphs' nodes in another order than elkjs 0.9.3. diff --git a/packages/diagram-client/src/elk-live-drag-router.ts b/packages/diagram-client/src/elk-live-drag-router.ts index 7ab0113..37cf005 100644 --- a/packages/diagram-client/src/elk-live-drag-router.ts +++ b/packages/diagram-client/src/elk-live-drag-router.ts @@ -1,8 +1,8 @@ -import { EditorContextService, IActionDispatcher, TYPES } from '@eclipse-glsp/client'; +import { EditorContextService, IActionDispatcher, ModifyCSSFeedbackAction, TYPES } from '@eclipse-glsp/client'; import type { GModelElement, SEdgeImpl } from '@eclipse-glsp/client'; import { Action, IActionHandler, MoveAction } from '@eclipse-glsp/sprotty'; import { inject, injectable } from 'inversify'; -import { WorkflowDiagramTypes } from '@dialogram/shared'; +import { WorkflowDiagramTypes, portAnchor } from '@dialogram/shared'; import { WorkflowRerouteEdgesAvoidOverlapsOperation } from './editing-action-handlers'; type ModelIndexLike = { @@ -90,6 +90,86 @@ function setIncidentEdgesForDrag(root: Readonly, edgeIds: Set>(); + +/** + * Whether this edge is hidden because the node it is attached to is being + * dragged. It is hidden from the first move until its stored route reaches + * its ports again: at the drop, with the first model update that carries a + * route for the node's new place, whichever server round trip brings it, or + * at once when the drag is cancelled and the node goes back. No update is + * waited for by name, since the ChangeBounds answer, the reroute answer or + * a bounds pass may be the one that carries the route. + * + * A question, not a step: it is asked of whatever model is being drawn, and + * the hidden model a bounds pass draws mid-drag has the node where the + * server last saw it, with routes that reach its ports. Were an edge crossed + * off here, that pass would cross it off for the rest of the drag. + */ +export function isEdgeHiddenForDrag(root: Readonly | undefined, edge: Readonly): boolean { + if (!root) { + return false; + } + const hidden = dragHiddenEdgeIds.get(getDragKey(root)); + if (!hidden?.has(String((edge as any).id))) { + return false; + } + return !routeReachesPorts(root, edge); +} + +function absolutePosition(element: any): { x: number; y: number } { + let x = 0; + let y = 0; + for (let current = element; current; current = current.parent) { + x += current.position?.x ?? 0; + y += current.position?.y ?? 0; + } + return { x, y }; +} + +function anchorOfPort(port: any): { x: number; y: number } | undefined { + if (!port || !isPortElement(port)) { + return undefined; + } + return portAnchor({ + absolute: absolutePosition(port), + size: port.size ?? port.bounds, + direction: port.args?.['cal:portDirection'] as string | undefined, + side: port.args?.['cal:portSide'] as string | undefined, + type: port.type + }); +} + +/** Whether the edge's stored route starts and ends where its ports are now. */ +function routeReachesPorts(root: Readonly, edge: Readonly): boolean { + const route = (edge as any).routingPoints as Array<{ x: number; y: number }> | undefined; + if (!Array.isArray(route) || route.length < 2) { + return false; + } + const index: ModelIndexLike | undefined = (root as any)?.index; + const source = anchorOfPort(getById(index, (edge as any).sourceId)); + const target = anchorOfPort(getById(index, (edge as any).targetId)); + if (!source || !target) { + return false; + } + const near = (a: { x: number; y: number }, b: { x: number; y: number }): boolean => + Math.abs(a.x - b.x) < 0.5 && Math.abs(a.y - b.y) < 0.5; + return near(route[0], source) && near(route[route.length - 1], target); +} + +/** How long hidden edges may wait for the server's routes before they are drawn as they were. */ +const REVEAL_FALLBACK_MS = 1500; +/** A finished move with no further move this long after it is the last of its series. */ +const QUIET_MS = 700; +/** A drag with no move this long is taken as one that never finishes. */ +const ABANDONED_DRAG_MS = 5000; + @injectable() export class WorkflowElkLiveDragRouter implements IActionHandler { @inject(EditorContextService) @@ -117,6 +197,10 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { /** Which path armed the pending reroute — reported to the server for timing. */ private pendingTrigger: 'finished' | 'quiet' | 'requeue' = 'quiet'; + /** The moved ids the hidden edge set was computed for, so a drag computes it once. */ + private hiddenForMovedIds = ''; + private revealTimer: ReturnType | undefined; + handle(action: Action): void { if (action.kind !== MoveAction.KIND) { return; @@ -155,14 +239,31 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { // treat brief pauses mid-drag as a drag end. markAnyDragActive(root, 650); - // Schedule a final reroute shortly after the MoveAction stream goes quiet. + // A drag in progress hides the moved nodes' edges until the drop's + // routes arrive. A move that is finished as it arrives (an arrow key) + // hides nothing: the routes follow within the round trip, and a blink + // on every key press would be worse than the stub it replaces. + if ((action as any).finished !== true) { + // A drop's reveal still pending belongs to the drag before this one. + if (this.revealTimer) { + clearTimeout(this.revealTimer); + this.revealTimer = undefined; + } + this.hideIncidentEdges(root); + } + + // Schedule a final reroute after the MoveAction stream goes quiet. While + // the pointer is down the drop brings a finished move, so for a drag this + // only catches one that never finishes (the pointer released off the + // diagram); it waits long enough that a node held still mid-drag is not + // taken for a drop, which would draw its edges again before the drop. if (this.dragEndTimer) { clearTimeout(this.dragEndTimer); } this.dragEndTimer = setTimeout(() => { this.pendingTrigger = 'quiet'; void this.flushPendingReroute(false); - }, 700); + }, (action as any).finished === true ? QUIET_MS : ABANDONED_DRAG_MS); // If this MoveAction marks the end of the drag, reroute right after release. if ((action as any).finished === true) { @@ -186,6 +287,45 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { // Preview rerouting is disabled; compute incident edges once on drag-end/quiet. } + private hideIncidentEdges(root: Readonly): void { + const movedIds = Array.from(this.pendingMovedElementIds).sort().join('\u0000'); + if (movedIds === this.hiddenForMovedIds && dragHiddenEdgeIds.has(getDragKey(root))) { + return; + } + const { incidentEdgeIds } = this.collectIncidentEdgesFromMovedElements(root, this.pendingMovedElementIds, new Map()); + this.hiddenForMovedIds = movedIds; + if (incidentEdgeIds.size === 0) { + return; + } + dragHiddenEdgeIds.set(getDragKey(root), incidentEdgeIds); + } + + /** From the final reroute on, the hidden edges may wait this long for a route; then they are drawn as they are. */ + private armReveal(): void { + if (dragHiddenEdgeIds.size === 0) { + return; + } + if (this.revealTimer) { + clearTimeout(this.revealTimer); + } + this.revealTimer = setTimeout(() => this.revealHiddenEdges(), REVEAL_FALLBACK_MS); + } + + private revealHiddenEdges(): void { + if (this.revealTimer) { + clearTimeout(this.revealTimer); + this.revealTimer = undefined; + } + this.hiddenForMovedIds = ''; + const wereHidden = dragHiddenEdgeIds.size > 0; + dragHiddenEdgeIds.clear(); + if (wereHidden) { + // The edges are drawn at the next render; make sure there is one, as + // no model update need follow (the fallback fires on its own). + void this.actionDispatcher.dispatch(ModifyCSSFeedbackAction.create({ elements: [], add: [] }) as any).catch(() => undefined); + } + } + private collectIncidentEdgesFromMovedElements( root: Readonly, movedElementIds: Set, @@ -348,6 +488,8 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { if (!preview) { this.forceNonPreviewFlush = false; this.lastDispatchedEdgeIds = []; + // Nothing to reroute (a cancelled drag): draw the edges as they are. + this.revealHiddenEdges(); } return; } @@ -365,6 +507,9 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { } this.dispatchInFlight = true; + if (!effectivePreview) { + this.armReveal(); + } try { await this.actionDispatcher.dispatch( WorkflowRerouteEdgesAvoidOverlapsOperation.create({ @@ -387,6 +532,8 @@ export class WorkflowElkLiveDragRouter implements IActionHandler { this.lastDispatchedEdgeIds = []; } } catch (e) { + // The routes are not coming: draw the edges as they are. + this.revealHiddenEdges(); // Re-queue so we can retry on the next tick; avoids getting stuck with stale routes. for (const id of edgeIds) { this.dispatchPendingEdgeIds.add(id); diff --git a/packages/diagram-client/src/stock-features.module.ts b/packages/diagram-client/src/stock-features.module.ts index ea5df0d..5e6aba9 100644 --- a/packages/diagram-client/src/stock-features.module.ts +++ b/packages/diagram-client/src/stock-features.module.ts @@ -152,6 +152,8 @@ export const workflowFeaturesModule = new ContainerModule((bind, unbind, isBound rebind(ChangeBoundsTool).to(WorkflowChangeBoundsTool).inSingletonScope(); // Drag-time rerouting: client streams MoveAction updates; server recomputes routes (ELK fixed). + // The moved node's edges are hidden through the drag and drawn again once + // a route for the node's new place is in the model (`isEdgeHiddenForDrag`). configureActionHandler(context, MoveAction.KIND, WorkflowElkLiveDragRouter); // The live tier: edges routed in the webview during a drag, with the same diff --git a/packages/diagram-client/src/views.ts b/packages/diagram-client/src/views.ts index fb30f2e..20f2059 100644 --- a/packages/diagram-client/src/views.ts +++ b/packages/diagram-client/src/views.ts @@ -44,7 +44,7 @@ import { import { EDGE_OPEN_ANCHOR_X_ARG, EDGE_OPEN_ANCHOR_Y_ARG } from './edge-open-mouse-listener'; import { clientBehavior } from './profile'; import { captionIsShared, carriedTextOf } from './edge-caption-sharing'; -import { isIncidentEdgeInActiveDrag } from './elk-live-drag-router'; +import { isEdgeHiddenForDrag, isIncidentEdgeInActiveDrag } from './elk-live-drag-router'; import { BoundaryPortGeometry, WorkflowDiagramCss, WorkflowDiagramMetadata, WorkflowDiagramTypes, normalizePortSide } from '@dialogram/shared'; import { WorkflowDiagramConstants } from '@dialogram/shared'; @@ -1765,6 +1765,11 @@ export class WorkflowEdgeView extends PolylineEdgeView { // is not visible. That shared geometry is what made this possible: the // client router previously computed anchors differently, which is why // this code used to bypass the router registry entirely. + // An edge of a node being dragged is not drawn until a route for the + // node's new place arrives (see `isEdgeHiddenForDrag`). + if (isEdgeHiddenForDrag(rootOfEdge(edge), edge as any)) { + return svg('g', { class: { 'workflow-edge': true, 'edge-hidden-for-drag': true } }); + } const serverRoute = (edge as any).routingPoints as { x: number; y: number }[] | undefined; const hasServerRoute = Array.isArray(serverRoute) && serverRoute.length >= 2; const liveRouted = isIncidentEdgeInActiveDrag(rootOfEdge(edge), String((edge as any).id)); diff --git a/packages/diagram-client/test/edges-hidden-while-dragged.test.ts b/packages/diagram-client/test/edges-hidden-while-dragged.test.ts new file mode 100644 index 0000000..ab93f0a --- /dev/null +++ b/packages/diagram-client/test/edges-hidden-while-dragged.test.ts @@ -0,0 +1,152 @@ +/** + * The edges of a node being dragged are not drawn while it moves: they come + * back as soon as their stored route reaches the ports again, which is the + * first model update with a route for the node's new place, or the node + * going back where it was. A move that is finished as it arrives (an arrow + * key) hides nothing. + */ +import 'reflect-metadata'; +import { describe, expect, it, vi } from 'vitest'; +import { WorkflowElkLiveDragRouter, isEdgeHiddenForDrag } from '../src/elk-live-drag-router'; + +type Point = { x: number; y: number }; +type Element = { + id: string; type: string; parent?: Element; children?: Element[]; + position?: Point; size?: { width: number; height: number }; + sourceId?: string; targetId?: string; routingPoints?: Point[]; +}; + +/** 140x60 nodes with a 7x7 input port on the left and an output port on the right; an output anchor is (x+140, y+29.5), an input one (x, y+29.5). */ +function model(): { root: any; elements: Map } { + const elements = new Map(); + const root: any = { id: 'root', type: 'graph', children: [] as Element[] }; + const node = (id: string, x: number, y: number): void => { + const n: Element = { id, type: 'node:actor', parent: root, children: [], position: { x, y }, size: { width: 140, height: 60 } }; + for (const [port, px] of [['in', 0], ['out', 133]] as Array<[string, number]>) { + const p: Element = { id: `${id}.${port}`, type: port === 'in' ? 'port:input' : 'port:output', parent: n, position: { x: px, y: 26 }, size: { width: 7, height: 7 } }; + n.children!.push(p); elements.set(p.id, p); + } + root.children.push(n); elements.set(id, n); + }; + const edge = (from: string, to: string, route: Point[]): void => { + const e: Element = { id: `${from}-${to}`, type: 'edge:connection', parent: root, sourceId: `${from}.out`, targetId: `${to}.in`, routingPoints: route }; + root.children.push(e); elements.set(e.id, e); + }; + node('a', 0, 0); node('b', 0, 200); node('t', 400, 100); node('c', 0, 400); node('d', 400, 400); + const into = (from: Point, to: Point): Point[] => [from, { x: 270, y: from.y }, { x: 270, y: to.y }, to]; + edge('a', 't', into({ x: 140, y: 29.5 }, { x: 400, y: 129.5 })); + edge('b', 't', into({ x: 140, y: 229.5 }, { x: 400, y: 129.5 })); + edge('c', 'd', [{ x: 140, y: 429.5 }, { x: 400, y: 429.5 }]); + root.index = { getById: (id: string) => elements.get(id), all: () => elements.values() }; + return { root, elements }; +} + +function router(root: any): { handler: WorkflowElkLiveDragRouter; dispatched: any[] } { + const dispatched: any[] = []; + const handler: any = new WorkflowElkLiveDragRouter(); + handler.editorContextService = { modelRoot: root }; + handler.actionDispatcher = { dispatch: async (action: any) => { dispatched.push(action); } }; + return { handler, dispatched }; +} + +/** The pointer moves the node, as sprotty's feedback does, and the drag router hears of it. */ +function drag(m: ReturnType, handler: any, id: string, to: Point, finished = false): void { + m.elements.get(id)!.position = to; + handler.handle({ kind: 'move', moves: [{ elementId: id, toPosition: to }], finished }); +} +const hidden = (m: ReturnType, ...ids: string[]): boolean[] => ids.map(id => isEdgeHiddenForDrag(m.root, m.elements.get(id)! as any)); + +describe('edges hidden while their node is dragged', () => { + it('hides the moved node\'s edges through the drag, not the others', () => { + const m = model(); + const { handler } = router(m.root); + drag(m, handler, 't', { x: 400, y: 120 }); + expect(hidden(m, 'a-t', 'b-t', 'c-d')).toEqual([true, true, false]); + drag(m, handler, 't', { x: 400, y: 160 }); + expect(hidden(m, 'a-t', 'b-t', 'c-d')).toEqual([true, true, false]); + }); + + it('draws an edge again as soon as a route for the node\'s new place is in the model', async () => { + vi.useFakeTimers(); + try { + const m = model(); + const { handler, dispatched } = router(m.root); + drag(m, handler, 't', { x: 400, y: 160 }); + drag(m, handler, 't', { x: 400, y: 160 }, true); + expect(hidden(m, 'a-t', 'b-t')).toEqual([true, true]); + // The server's route for one edge arrives (whichever round trip brings it). + m.elements.get('a-t')!.routingPoints = [{ x: 140, y: 29.5 }, { x: 270, y: 29.5 }, { x: 270, y: 189.5 }, { x: 400, y: 189.5 }]; + expect(hidden(m, 'a-t', 'b-t')).toEqual([false, true]); + await vi.advanceTimersByTimeAsync(100); + expect(dispatched.length).toBe(1); + m.elements.get('b-t')!.routingPoints = [{ x: 140, y: 229.5 }, { x: 270, y: 229.5 }, { x: 270, y: 189.5 }, { x: 400, y: 189.5 }]; + expect(hidden(m, 'a-t', 'b-t')).toEqual([false, false]); + } finally { + vi.useRealTimers(); + } + }); + + it('keeps them hidden while the node is held still mid-drag', async () => { + vi.useFakeTimers(); + try { + const m = model(); + const { handler, dispatched } = router(m.root); + drag(m, handler, 't', { x: 400, y: 160 }); + await vi.advanceTimersByTimeAsync(3000); + expect(hidden(m, 'a-t', 'b-t')).toEqual([true, true]); + expect(dispatched.length).toBe(0); + drag(m, handler, 't', { x: 400, y: 170 }); + drag(m, handler, 't', { x: 400, y: 170 }, true); + await vi.advanceTimersByTimeAsync(100); + expect(dispatched.length).toBe(1); + } finally { + vi.useRealTimers(); + } + }); + + it('stays hidden through a bounds pass that draws the node where the server last saw it', () => { + const m = model(); + const { handler } = router(m.root); + drag(m, handler, 't', { x: 400, y: 160 }); + expect(hidden(m, 'a-t', 'b-t')).toEqual([true, true]); + // The server's model, drawn in the hidden layer for its bounds: the same + // root id, the node still at its old place, the routes reaching its ports. + const server = model(); + expect(hidden(server, 'a-t', 'b-t')).toEqual([false, false]); + expect(hidden(m, 'a-t', 'b-t')).toEqual([true, true]); + drag(m, handler, 't', { x: 400, y: 170 }); + expect(hidden(m, 'a-t', 'b-t')).toEqual([true, true]); + }); + + it('draws them again at once when the drag is cancelled and the node goes back', () => { + const m = model(); + const { handler } = router(m.root); + drag(m, handler, 't', { x: 400, y: 160 }); + expect(hidden(m, 'a-t')).toEqual([true]); + m.elements.get('t')!.position = { x: 400, y: 100 }; + expect(hidden(m, 'a-t', 'b-t')).toEqual([false, false]); + }); + + it('draws them as they were when no route arrives', async () => { + vi.useFakeTimers(); + try { + const m = model(); + const { handler } = router(m.root); + drag(m, handler, 't', { x: 400, y: 160 }); + drag(m, handler, 't', { x: 400, y: 160 }, true); + await vi.advanceTimersByTimeAsync(100); + expect(hidden(m, 'a-t')).toEqual([true]); + await vi.advanceTimersByTimeAsync(1600); + expect(hidden(m, 'a-t')).toEqual([false]); + } finally { + vi.useRealTimers(); + } + }); + + it('hides nothing for a move by arrow key', () => { + const m = model(); + const { handler } = router(m.root); + drag(m, handler, 't', { x: 400, y: 101 }, true); + expect(hidden(m, 'a-t', 'b-t')).toEqual([false, false]); + }); +}); diff --git a/packages/diagram-client/test/glsp-stub.ts b/packages/diagram-client/test/glsp-stub.ts index 0139bc0..2bd9506 100644 --- a/packages/diagram-client/test/glsp-stub.ts +++ b/packages/diagram-client/test/glsp-stub.ts @@ -304,3 +304,8 @@ const moveFeedbackModule = requireGlsp('@eclipse-glsp/client/lib/features/tools/ export const ChangeBoundsTool = changeBoundsToolModule.ChangeBoundsTool; export const ChangeBoundsListener = changeBoundsToolModule.ChangeBoundsListener; export const FeedbackMoveMouseListener = moveFeedbackModule.FeedbackMoveMouseListener; + +export const ModifyCSSFeedbackAction = { + KIND: 'modifyCSSFeedback', + create: (options: { elements?: unknown[]; add?: string[]; remove?: string[] } = {}) => ({ kind: 'modifyCSSFeedback', ...options }) +}; diff --git a/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts b/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts index 13746a5..09a4888 100644 --- a/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts +++ b/packages/diagram-server/src/operations/reroute-edges-avoid-overlaps-handler.ts @@ -142,20 +142,25 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation private readonly debugReroute: number = Number(process.env.WORKFLOW_DIAGRAM_DEBUG_REROUTE ?? '0') || 0; /** - * Which router computes edge routes. + * Which router computes the routes an edge cannot keep (see `slideRoute`). * - * libavoid measures better than the built-in router on every axis in an - * offline harness (crossings, overlaps, bends, length), but it changed drag - * behaviour for the worse on a real network in a way the harness does not - * reproduce, so it is OPT-IN until that is understood: + * libavoid, routing the edges on one port as one shared path: that is + * what the layout drew, a trunk into the port, and what the built-in + * router cannot draw, since it gives every edge its own channel. libavoid + * also measures better on crossings, overlaps, bends and length. What + * made it worse on a real drag before was the stagger of the endpoints on + * a port, which turned every fan-in into a comb; `sharedPortPaths` is the + * cure. The built-in router remains the fallback when libavoid is + * unavailable or cannot solve an edge, and the choice for a reroute of + * every edge in a large diagram, since libavoid's cost is superlinear in + * the connector count (58 edges 337ms). * - * WORKFLOW_DIAGRAM_ROUTER=libavoid - * - * Do not flip the default back without a reproduction — see - * `dumpRoutingInput` below for capturing one. + * WORKFLOW_DIAGRAM_ROUTER=manhattan the built-in router throughout */ private readonly useLibavoid: boolean = - (process.env.WORKFLOW_DIAGRAM_ROUTER ?? '').toLowerCase() === 'libavoid'; + (process.env.WORKFLOW_DIAGRAM_ROUTER ?? '').toLowerCase() !== 'manhattan'; + /** A reroute of every edge goes to libavoid only up to this many edges. */ + private static readonly LIBAVOID_MAX_EDGES = 200; /** * `WORKFLOW_DIAGRAM_DUMP_ROUTING=1` writes the exact router input — node @@ -1015,6 +1020,7 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation private async executeOperation(operation: WorkflowRerouteEdgesAvoidOverlapsOperation, timing?: RerouteTiming): Promise { const nowMs = Date.now(); + const startedAt = nowMs; const src = operation.source ?? 'client'; this.modelState.set('workflow.lastRerouteAtMs', nowMs); this.modelState.set('workflow.lastRerouteSource', src); @@ -1133,6 +1139,7 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation console.info('[cal-reroute] slide — ' + slidEdgeIds.size + ' edges kept their routes'); } await this.persistRoutes(root, operation); + this.logPerf(operation, startedAt, slidEdgeIds.size, 0, 'slide'); return; } @@ -1190,7 +1197,7 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation if (timing) { timing.anchorMs = Date.now() - anchorStartedAt; } const routeStartedAt = Date.now(); let usedLibavoid = false; - if (this.useLibavoid) { + if (this.useLibavoid && routable.length <= WorkflowRerouteEdgesAvoidOverlapsOperationHandler.LIBAVOID_MAX_EDGES) { const shapes: RouterObstacle[] = obstacles.map(o => ({ id: o.id, // `obstacles` carries PAD already baked in; libavoid keeps its own @@ -1212,7 +1219,8 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation idealNudgingDistance: WorkflowRerouteEdgesAvoidOverlapsOperationHandler.EDGE_GAP, // Matches the built-in router's stub, so edges leave a port the // same distance either way and clearance stays at PAD. - portStub: WorkflowRerouteEdgesAvoidOverlapsOperationHandler.STUB + portStub: WorkflowRerouteEdgesAvoidOverlapsOperationHandler.STUB, + sharedPortPaths: true }); if (routes) { usedLibavoid = true; @@ -1256,6 +1264,14 @@ export class WorkflowRerouteEdgesAvoidOverlapsOperationHandler extends Operation } await this.persistRoutes(root, operation); + this.logPerf(operation, startedAt, slidEdgeIds.size, routed, usedLibavoid ? 'libavoid' : 'manhattan'); + } + + /** Always-on breadcrumb, like the layout's: what a persisted reroute did and how long it took. */ + private logPerf(operation: WorkflowRerouteEdgesAvoidOverlapsOperation, startedAt: number, kept: number, routed: number, router: string): void { + if (operation.preview) { return; } + // eslint-disable-next-line no-console + console.log(`[dialogram perf] reroute (${operation.source ?? 'client'}): ${kept} kept, ${routed} routed by ${router}, ${Date.now() - startedAt}ms`); } /** Persist the layout, positions and routes; not for a preview (a drag). */ diff --git a/packages/diagram-server/src/routing/libavoid-router.ts b/packages/diagram-server/src/routing/libavoid-router.ts index 309e777..1993691 100644 --- a/packages/diagram-server/src/routing/libavoid-router.ts +++ b/packages/diagram-server/src/routing/libavoid-router.ts @@ -101,12 +101,23 @@ export interface LibavoidRoutingOptions { * and the stub is axis-aligned so the result stays orthogonal. */ portStub?: number; + /** + * Edges on one port start at one point, so libavoid routes them as one + * shared path: a trunk with a junction where they part, the way a layout + * draws a fan-in and a schematic draws a bus out of a pin. Off, each edge + * on a port starts at its own staggered point and is nudged into its own + * channel, which reads as a comb of parallel lines where edges meet. + * The shared endpoints are not nudged apart (libavoid's default), so an + * edge still leaves its port straight. + */ + sharedPortPaths?: boolean; } const DEFAULT_OPTIONS: Required = { shapeBufferDistance: 15, idealNudgingDistance: 8, - portStub: 20 + portStub: 20, + sharedPortPaths: true }; /** @@ -125,6 +136,8 @@ function candidateDirs(): string[] { if (typeof __dirname === 'string') { dirs.push(path.join(__dirname, 'libavoid')); dirs.push(__dirname); + // A dev run or a test from the sources: the vendored build in the repo. + dirs.push(path.join(__dirname, '..', '..', '..', '..', 'vendor', 'libavoid')); } return dirs; } @@ -188,6 +201,32 @@ export async function isLibavoidAvailable(): Promise { * traversable. Returns Infinity when there is nothing to constrain. */ +/** + * Put a route end that libavoid nudged off its port's row back onto it. + * + * Edges that start at one point are not nudged apart as shared paths, but the + * nudging of segments connected to shapes still moves their first segments + * half a nudging distance each way, and the L back to the anchor then draws a + * 4px kink a few pixels off the port. A step smaller than the nudging + * distance is that kink, not a turn; the first segment goes back onto the + * row, and the bend after it follows, so the edges leave the port together + * and part where their ways part. + */ +function unkink(points: RouterPoint[], anchor: RouterPoint, nudge: number): void { + if (points.length < 2) { + return; + } + const atStart = Math.abs(points[0].x - anchor.x) <= Math.abs(points[points.length - 1].x - anchor.x); + const i0 = atStart ? 0 : points.length - 1; + const i1 = atStart ? 1 : points.length - 2; + const step = Math.abs(points[i0].y - anchor.y); + if (step < 0.01 || step >= nudge || Math.abs(points[i0].y - points[i1].y) > 0.01) { + return; + } + points[i0] = { x: points[i0].x, y: anchor.y }; + points[i1] = { x: points[i1].x, y: anchor.y }; +} + /** * The corner needed to join a port anchor to the route, when libavoid's end * does not sit on the anchor's row. Empty when they already align. @@ -248,11 +287,13 @@ export async function routeOrthogonal( // invalidate, derive the ceiling from the anchors actually passed in. // Push each endpoint out along its port's side before routing; the stub back // to the true anchor is re-attached afterwards. - // Staggered where a port carries several edges, so they do not all turn on - // one x and blur together at the port. The rule is shared with the client's - // live router — see `portStubDistances` — because the two must agree or the - // edge visibly jumps the moment the mouse is released. - const stubs = portStubOffsets( + // + // With `sharedPortPaths` every edge on a port starts at the same point and + // libavoid keeps them on one path for as long as they go the same way. + // Otherwise they are staggered, so they do not all turn on one x, the rule + // the client's live router shares (`portStubOffsets`). + const base: PortStubOffset = { distance: requested.portStub, across: 0 }; + const stubs = requested.sharedPortPaths ? new Map() : portStubOffsets( connectors.flatMap(c => [ ...(c.sourceSide ? [{ id: c.id, end: 'source' as const, ...c.source, side: c.sourceSide, towardY: c.target.y }] @@ -264,7 +305,6 @@ export async function routeOrthogonal( obstacles, { portStub: requested.portStub, nudge: requested.idealNudgingDistance } ); - const base: PortStubOffset = { distance: requested.portStub, across: 0 }; const offsetOf = (p: RouterPoint, side: 'WEST' | 'EAST' | undefined, at: PortStubOffset): RouterPoint => side === 'EAST' ? { x: p.x + at.distance, y: p.y + at.across } : side === 'WEST' ? { x: p.x - at.distance, y: p.y + at.across } @@ -361,6 +401,10 @@ export async function routeOrthogonal( // So the join is an L where the lines differ: out along the // anchor's row first, then across to meet the route. Orthogonal, // and the nudging survives. + if (opts.sharedPortPaths) { + unkink(points, c.source, opts.idealNudgingDistance); + unkink(points, c.target, opts.idealNudgingDistance); + } const withStubs = [c.source, ...bridge(c.source, points[0]), ...points, ...bridge(c.target, points[points.length - 1]).reverse(), c.target]; routes.set(id, snapRouteEndpoints(withStubs, c.source, c.target)); diff --git a/packages/diagram-server/test/libavoid-router.test.ts b/packages/diagram-server/test/libavoid-router.test.ts index 6aee65d..0573ee8 100644 --- a/packages/diagram-server/test/libavoid-router.test.ts +++ b/packages/diagram-server/test/libavoid-router.test.ts @@ -103,6 +103,37 @@ describe('libavoid router', () => { expect(new Set(ys).size).toBe(4); }); + it('routes the edges on one port as one shared path, leaving the port straight', async () => { + // Two sources feeding one input port, the fan-in a layout draws as a + // trunk. Shared endpoints keep the two on one path from where they + // meet to the port; staggered endpoints made a comb of two channels. + const obstacles: RouterObstacle[] = [ + { id: 'a', x: 0, y: 0, width: 100, height: 40 }, + { id: 'b', x: 0, y: 200, width: 100, height: 40 }, + { id: 't', x: 400, y: 100, width: 100, height: 40 } + ]; + const into = (id: string, y: number): RouterConnector => ({ + id, source: { x: 100, y }, target: { x: 400, y: 120 }, sourceSide: 'EAST', targetSide: 'WEST' + }); + const connectors = [into('a-t', 20), into('b-t', 220)]; + + const shared = (await routeOrthogonal(obstacles, connectors))!; + const [ra, rb] = [shared.get('a-t')!, shared.get('b-t')!]; + // The last segment, into the port, lies on the port's row and the two + // edges arrive down the same vertical. + for (const r of [ra, rb]) { + expect(r[r.length - 1]).toEqual({ x: 400, y: 120 }); + expect(r[r.length - 2].y).toBe(120); + expect(r[0].y).toBe(r[1].y); + } + expect(ra[ra.length - 2].x).toBe(rb[rb.length - 2].x); + expect(ra[ra.length - 3].x).toBe(rb[rb.length - 3].x); + + const staggered = (await routeOrthogonal(obstacles, connectors, { sharedPortPaths: false }))!; + const [sa, sb] = [staggered.get('a-t')!, staggered.get('b-t')!]; + expect(sa[sa.length - 3].x).not.toBe(sb[sb.length - 3].x); + }); + it('returns an empty map for no connectors without touching the solver', async () => { const routes = await routeOrthogonal([{ id: 'a', x: 0, y: 0, width: 10, height: 10 }], []); expect(routes).toBeDefined(); @@ -354,88 +385,53 @@ describe('libavoid router — several edges leaving one port', () => { } }); - it('still separates two edges that share a port', async () => { + /** + * Two edges leaving one port go out together and part where their ways + * part, the way a layout draws a fan-out and a schematic draws a bus out + * of a pin: one shared stub, a junction, then each its own way. + * + * This replaces the staggered stub, which started each edge on a port at + * its own point so that libavoid would give each its own channel. That + * drew every fan-in and fan-out as a comb of parallel lines, which is the + * scramble a move of a few pixels made of the layout's trunks. The kink + * that the stagger was for (libavoid nudging two edges with one start + * half a nudging distance each way, an L back to the port 4px off its + * row) is taken out after routing instead: a step smaller than the + * nudging distance at a port goes back onto the port's row. + */ + it('takes two edges off one port on one stub, parting at a junction', async () => { const routes = (await routeOrthogonal(fx.nodes, connectors, opts))!; const shared = connectors.filter(c => c.id.includes('rob:Com')); expect(shared.length).toBe(2); + const [a, b] = shared.map(c => routes.get(c.id)!); - // They start at the same anchor, so they must diverge somewhere: the - // long horizontal runs need to sit on different rows. - const rows = shared.map(c => { - const r = routes.get(c.id)!; + // Both leave the port along its row, straight, with no step back onto it. + for (const r of [a, b]) { + expect(orthogonal(r)).toBe(true); + const firstOffRow = r.slice(1).find(p => Math.abs(p.y - r[0].y) > 0.5); + expect(Math.abs(firstOffRow!.y - r[0].y)).toBeGreaterThan(opts.idealNudgingDistance); + } + // They share the stub, at least the port stub long, then part. + const sharedUntilX = Math.min(a[1].x, b[1].x); + expect(Math.abs(sharedUntilX - a[0].x)).toBeGreaterThanOrEqual(opts.portStub); + expect(a[1].y).toBe(b[1].y); + // And they end up on different rows, each heading for its own target. + const longestRow = (r: typeof a): number => { let longest = { len: 0, y: 0 }; for (let i = 1; i < r.length; i++) { - if (Math.abs(r[i].y - r[i - 1].y) > 0.5) continue; + if (Math.abs(r[i].y - r[i - 1].y) > 0.5) { continue; } const len = Math.abs(r[i].x - r[i - 1].x); - if (len > longest.len) longest = { len, y: r[i].y }; + if (len > longest.len) { longest = { len, y: r[i].y }; } } return longest.y; - }); - expect(Math.abs(rows[0] - rows[1])).toBeGreaterThan(4); - }); - - /** - * The reported artifact, and the one that matters most: an edge on a shared - * port must leave it STRAIGHT, like an edge on any other port. - * - * Handed two edges with the same start, libavoid separates them the only way - * it can — half the nudging distance each, in opposite directions. Both then - * come back off the anchor's row, both need an L to get back to it, and a - * 4px step a few pixels off the port does not read as a branch. It reads as - * a kink in an edge that should have gone straight out, which is exactly - * what was reported at `rob.Com` (`Rb` and `Trap`, one edge each, leave - * perfectly straight right below it). - * - * So the starts are separated BEFORE routing instead: the first edge keeps - * the port's row and libavoid has no reason to move it. A "real" turn is one - * bigger than the nudging distance; anything at or below that is the kink. - */ - it('leaves the first edge on a shared port straight', async () => { - const routes = (await routeOrthogonal(fx.nodes, connectors, opts))!; - const shared = connectors - .filter(c => c.id.includes('rob:Com')) - .sort((a, b) => (a.id < b.id ? -1 : 1)); - expect(shared.length).toBe(2); - - /** How far the route first departs the anchor's row. */ - const firstTurn = (id: string): number => { - const r = routes.get(id)!; - const step = r.slice(1).find(p => Math.abs(p.y - r[0].y) > 0.5); - return step ? Math.abs(step.y - r[0].y) : Infinity; }; - - // The first edge goes straight out and only leaves the row to head for - // its target. - expect(firstTurn(shared[0].id)).toBeGreaterThan(opts.idealNudgingDistance); - // The second branches off it by a FULL nudge, not the half-nudge kink - // that libavoid produces when it has to separate them itself. - expect(firstTurn(shared[1].id)).toBeGreaterThanOrEqual(opts.idealNudgingDistance); + expect(a[a.length - 1].y).not.toBe(b[b.length - 1].y); + expect(longestRow(a) === longestRow(b) || a[a.length - 2].x !== b[b.length - 2].x).toBe(true); }); - /** - * ...and they must not TURN at the same place either, which is the third - * way this join has looked broken. - * - * Every end starts `portStub` off its port, so the L back to the anchor - * turned at exactly that distance for every edge — put two edges on one port - * and both corners landed on one x (444 here). The routes then ran - * superimposed for the whole stub and split there in a 4px step. At stroke - * width, with rounded joins, that is not a fan-out; it is a blob on the port, - * which is what was reported at `rob.Com`. - * - * libavoid separates such corners by itself when both edges turn and run a - * long way (three edges off one port in `fixtures-dense-core` come back - * 12px apart with no help). It cannot here, because the first move is only - * the 4px nudge — far too short for segment nudging to bite. Hence the - * staggered stub, and hence this test: the previous two assertions both pass - * with every corner piled on one x. - */ - it('does not turn two edges off one port at the same x', async () => { - const routes = (await routeOrthogonal(fx.nodes, connectors, opts))!; + it('staggers the starts on a port when shared paths are off', async () => { + const routes = (await routeOrthogonal(fx.nodes, connectors, { ...opts, sharedPortPaths: false }))!; const shared = connectors.filter(c => c.id.includes('rob:Com')); - expect(shared.length).toBe(2); - - // The corner is the last point still on the anchor's own row. const corners = shared.map(c => { const r = routes.get(c.id)!; let i = 1; diff --git a/packages/diagram-server/test/reroute-keeps-the-layouts-trunk.test.ts b/packages/diagram-server/test/reroute-keeps-the-layouts-trunk.test.ts index 39173d7..ba2dec8 100644 --- a/packages/diagram-server/test/reroute-keeps-the-layouts-trunk.test.ts +++ b/packages/diagram-server/test/reroute-keeps-the-layouts-trunk.test.ts @@ -4,7 +4,8 @@ * one port share a trunk; routing a moved node's edges anew, one at a time, * turned that trunk into a comb of parallel lines, and a move of a few * pixels scrambled the picture. The router runs only for an edge whose kept - * route would not do. + * route would not do, and that router is libavoid, which routes the edges + * on one port as one shared path. */ import 'reflect-metadata'; import { describe, expect, it } from 'vitest'; @@ -51,11 +52,14 @@ function graph(...elements: Array): { root: GGraph; handler: any; return { root, handler, find: id => index.find(id) }; } -async function moveAndReroute(g: ReturnType, nodeId: string, to: Point, edgeIds: string[]): Promise { +/** Moves the node, reroutes the edges, and says which router drew the routes the slide refused. */ +async function moveAndReroute(g: ReturnType, nodeId: string, to: Point, edgeIds: string[]): Promise { g.find(nodeId).position = to; + const timing = { router: 'none' }; await g.handler.executeOperation({ kind: 'dialogram.rerouteEdgesAvoidOverlaps', isOperation: true, elementIds: edgeIds, preview: false, source: 'changeBounds' - }); + }, timing); + return timing.router; } const routeOf = (g: ReturnType, id: string): Point[] => g.find(id).routingPoints; @@ -79,7 +83,7 @@ function fanIn(): ReturnType { describe('reroute after a move keeps the layout\'s routes', () => { it('slides the ends onto the ports and keeps the trunk, at any distance', async () => { const g = fanIn(); - await moveAndReroute(g, 't', { x: 400, y: 160 }, ['a-t', 'b-t']); + expect(await moveAndReroute(g, 't', { x: 400, y: 160 }, ['a-t', 'b-t'])).toBe('slide'); const movedIn = { x: 400, y: 189.5 }; expect(routeOf(g, 'a-t')).toEqual(trunk(A_OUT, movedIn)); expect(routeOf(g, 'b-t')).toEqual(trunk(B_OUT, movedIn)); @@ -106,7 +110,7 @@ describe('reroute after a move keeps the layout\'s routes', () => { edge('a-t', 'a', 't', trunk(A_OUT, T_IN)), edge('b-t', 'b', 't', trunk(B_OUT, T_IN)) ); - await moveAndReroute(g, 't', { x: 400, y: 250 }, ['a-t', 'b-t']); + expect(await moveAndReroute(g, 't', { x: 400, y: 250 }, ['a-t', 'b-t'])).toBe('libavoid'); const xBox = { id: 'x', x: 230, y: 240, r: 370, b: 300 }; for (const id of ['a-t', 'b-t']) { const route = routeOf(g, id); @@ -121,7 +125,7 @@ describe('reroute after a move keeps the layout\'s routes', () => { it('routes anew an edge whose stub would turn back through its node', async () => { const g = fanIn(); // t now stands left of the trunk: a kept route would enter it from behind. - await moveAndReroute(g, 't', { x: 200, y: 100 }, ['a-t', 'b-t']); + expect(await moveAndReroute(g, 't', { x: 200, y: 100 }, ['a-t', 'b-t'])).toBe('libavoid'); for (const id of ['a-t', 'b-t']) { const route = routeOf(g, id); expect(route[route.length - 1]).toEqual({ x: 200, y: 129.5 }); @@ -132,7 +136,7 @@ describe('reroute after a move keeps the layout\'s routes', () => { it('routes anew a straight edge, which has no bend to carry', async () => { const g = graph(node('c', 0, 0), node('d', 400, 0), edge('c-d', 'c', 'd', [{ x: 140, y: 29.5 }, { x: 400, y: 29.5 }])); - await moveAndReroute(g, 'd', { x: 400, y: 60 }, ['c-d']); + expect(await moveAndReroute(g, 'd', { x: 400, y: 60 }, ['c-d'])).toBe('libavoid'); const route = routeOf(g, 'c-d'); expect(route.length).toBeGreaterThanOrEqual(4); expect(isOrthogonalPolyline(route)).toBe(true);