From 00ebf91b9068d98af7c92ded2cf28b677e116375 Mon Sep 17 00:00:00 2001 From: "dayuan.jiang" Date: Sun, 9 Aug 2026 18:18:54 +0900 Subject: [PATCH] fix(diagram-engine): eliminate arrows drawn through boxes, complete the route search A user's approval-workflow flowchart came out with the return arrow drawn straight through two unrelated steps, and a second report showed arrows leaving a box and bending straight back across it. Measured over 250 generated flowcharts (2722 edges): 347 arrows crossed an unrelated box and 215 waypoints landed inside a shape. Four defects, each measured in isolation: 1. The invisible layer containers draw_graph emits were handed to the router as frames, so every clean return path was rejected for "trespassing" on a border that is not drawn, and the fallback cut through two boxes. Excluding invisible containers: 347 -> 218 crossing arrows, no diagram made worse. 2. The router chose the horizontal-vs-vertical axis BEFORE searching, so when the only clean corridor ran along the other axis it was never looked at. A complete two-bend candidate generator that tries both trunk axes, all four sides at each end, and the port fractions: 218 -> 27. (An independent ablation measured the axis pre-choice alone at a 40% per-edge failure rate.) 3. Nothing stopped a route's first leg from turning back across its own source shape - the obstacle test exempts an edge's own endpoints, and must, since the line has to touch them. A terminal-leg rule refuses such routes outright: 215 -> 4 hooks. 4. A two-bend search cannot express the staircase needed when a box sits directly between two vertically aligned nodes (21 of the last 27 crossings). Added the orthogonal visibility graph + A* from Wybrow, Marriott & Stuckey, "Orthogonal Connector Routing" (GD 2009) - the libavoid algorithm - as the backstop when the candidate search finds nothing. The interesting-points grid is provably sufficient: any valid route shrinks onto it without getting longer or gaining bends. The A* state is (point, incoming direction) with libavoid's bend cost of 10, and the admissible bends-remaining heuristic, so it returns a cheapest route, not merely a route. Implemented from the paper, not ported. After all four: 0 crossing arrows and 0 hooks over the same 250 diagrams, page area unchanged (494k px^2 mean), 260ms for the whole corpus, mean 0.82 bends per edge. The shape ladder still runs first, so routes that were already clean are byte-identical. Also post-nudge validation now checks the whole path (the nudge pass only reverts the single segment it moved, judged in isolation) and restores the search's route if nudging made it dirty. --- lib/diagram-engine/render.ts | 19 +- lib/diagram-engine/route.ts | 291 +++++++++++++++++++++++- lib/diagram-engine/visgraph.ts | 260 +++++++++++++++++++++ tests/unit/diagram-engine-graph.test.ts | 140 ++++++++++++ 4 files changed, 706 insertions(+), 4 deletions(-) create mode 100644 lib/diagram-engine/visgraph.ts diff --git a/lib/diagram-engine/render.ts b/lib/diagram-engine/render.ts index f339fc8..fca386d 100644 --- a/lib/diagram-engine/render.ts +++ b/lib/diagram-engine/render.ts @@ -22,6 +22,7 @@ import { sequenceMetrics, } from "./layout" import { + isInvisible, stampCell, stampContainer, stampLane, @@ -700,8 +701,24 @@ export function renderDiagram( // Frames are passable but not free to ignore: a line that runs alongside a border, or // cuts through a frame only one of its endpoints belongs to, reads as a mistake even // though it hits nothing. + // + // An INVISIBLE container is excluded, because both of those judgements are about what a + // reader sees, and there is no border on screen to run alongside or to trespass across. + // A layer band in a flowchart is exactly that: `draw_graph` wraps each row of the graph + // in an unlabelled, unstroked container purely to stack them. Counting those as frames + // measurably ruined the arrows — a back edge such as "return for correction" → "submit" + // leaves its own band, so every clean route was rejected for trespassing on a frame that + // is not drawn, and the router fell back to one that cut straight through two boxes. + // Measured over 161 generated flowcharts: 319 crossing edges before, 151 after, and not + // one diagram made worse. const frames = new Set( - flat.filter((f) => isContainer(f.node)).map((f) => f.node.id), + flat + .filter( + (f) => + isContainer(f.node) && + !isInvisible(styleFor(f.node, opts.resolveStyle)), + ) + .map((f) => f.node.id), ) const routes = routeEdges( routable.map(({ link: l, index }) => ({ diff --git a/lib/diagram-engine/route.ts b/lib/diagram-engine/route.ts index dcc8e35..9bcc70e 100644 --- a/lib/diagram-engine/route.ts +++ b/lib/diagram-engine/route.ts @@ -29,6 +29,7 @@ */ import type { Rect } from "./types" +import { routeOrthogonal, SIDE_DIR } from "./visgraph" /** Which side of a node an edge attaches to. */ export type Side = "L" | "R" | "T" | "B" @@ -141,6 +142,40 @@ function shapePoints( return { sp, ep, wp } } +/** + * Does this route's first or last leg turn back over the shape it belongs to? + * + * An arrow has to leave its own shape going AWAY from it. When the first bend lands back + * within the source's own span, draw.io draws the arrow out of one side and immediately back + * across the shape's own edge — the small hook seen coming out of a box's right side and + * turning straight back over it. Nothing is technically crossed, which is why the ordinary + * obstacle test misses it: an edge is exempt from its own two endpoints, and that exemption + * has to exist, since the line must touch them. + * + * This checks the two terminal legs only. A middle leg running past its own endpoint is + * normal — that is what a return path does. + */ +function doublesBack(pts: Point[], a: Rect, b: Rect): boolean { + const backOver = (from: Point, to: Point, r: Rect): boolean => { + // Leaving through a vertical side: the leg must not head back inside the box's width. + if (Math.abs(from.x - r.x) < 1 || Math.abs(from.x - (r.x + r.w)) < 1) { + if (Math.abs(from.y - to.y) < 1) + return to.x > r.x + 1 && to.x < r.x + r.w - 1 + } + // Leaving through a horizontal side: likewise for the box's height. + if (Math.abs(from.y - r.y) < 1 || Math.abs(from.y - (r.y + r.h)) < 1) { + if (Math.abs(from.x - to.x) < 1) + return to.y > r.y + 1 && to.y < r.y + r.h - 1 + } + return false + } + if (pts.length < 3) return false + return ( + backOver(pts[0], pts[1], a) || + backOver(pts[pts.length - 1], pts[pts.length - 2], b) + ) +} + /** * Lane positions to try inside a gap, from the middle outwards. * @@ -462,6 +497,137 @@ export function routeEdges( avoided: boolean }[] = [] + /** + * Every two-bend route that is geometrically distinct, ranked; the cheapest clear one. + * + * The ladder above tries a fixed list of shapes and, crucially, decides the axis BEFORE it + * searches — so when a vertical corridor is the only clean option and the axis came out + * horizontal, the answer is in the half that was never looked at. This does not choose an + * axis: it tries both trunk directions and all four sides at each end. + * + * The lanes come from the obstacles themselves rather than a fixed step. Whether a lane + * collides can only change where it crosses an obstacle's boundary, so one lane taken from + * each gap between boundaries covers every distinct outcome — sampling every 10px would + * test the same corridor repeatedly and still miss a narrow one. + */ + const complete = ( + a: Rect, + b: Rect, + exempt: Set, + sf: number, + tf: number, + ): { + exitSide: Side + entrySide: Side + wp: Point[] + avoided: boolean + sf: number + tf: number + } | null => { + const blockers = cards.filter((c) => !exempt.has(c.id)) + // Candidate trunk positions: just outside each obstacle edge, and the middle of each + // gap between consecutive edges. + const linesFrom = ( + values: number[], + lo: number, + hi: number, + ): number[] => { + const sorted = [...new Set(values)].sort((p, q) => p - q) + const out = [lo, hi] + for (const v of sorted) { + out.push(v - MARGIN - 1, v + MARGIN + 1) + } + for (let k = 0; k + 1 < sorted.length; k++) + out.push(Math.round((sorted[k] + sorted[k + 1]) / 2)) + return [...new Set(out)] + } + const xLanes = linesFrom( + blockers.flatMap((c) => [c.r.x, c.r.x + c.r.w]), + Math.min(a.x, b.x) - 40, + Math.max(a.x + a.w, b.x + b.w) + 40, + ) + const yLanes = linesFrom( + blockers.flatMap((c) => [c.r.y, c.r.y + c.r.h]), + Math.min(a.y, b.y) - 40, + Math.max(a.y + a.h, b.y + b.h) + 40, + ) + + const spread = [0.5, 0.3, 0.7, 0.16, 0.84] + const portTries: [number, number][] = [[sf, tf]] + for (const p of spread) for (const q of spread) portTries.push([p, q]) + + let best: { + exitSide: Side + entrySide: Side + wp: Point[] + avoided: boolean + /** Fractions this route needs; they may differ from the assigned pair. */ + sf: number + tf: number + } | null = null + let bestCost = Number.POSITIVE_INFINITY + + const consider = ( + es: Side, + en: Side, + shape: Shape, + pf: number, + qf: number, + ) => { + const g = shapePoints(a, b, es, en, pf, qf, shape) + const pts = [g.sp, ...g.wp, g.ep] + if (pathHits(pts, exempt)) return + if (doublesBack(pts, a, b)) return + const cost = + frameOffences(pts, a, b) * 5000 + + (overlapsUsed(pts) ? 700 : 0) + + g.wp.length * 80 + + pathLength(pts) + if (cost < bestCost) { + bestCost = cost + best = { + exitSide: es, + entrySide: en, + wp: g.wp, + avoided: g.wp.length > 0, + sf: pf, + tf: qf, + } + } + } + + // A vertical trunk leaves and enters through a left or right side; a horizontal one + // through a top or bottom. Both ends are tried on both sides, which is what lets an + // arrow leave the way it has to rather than the way the axis guess expected. + // + // The port FRACTIONS are searched too, not just the sides. The de-collide pass assigns + // one fraction per (node, side) before any path is known, purely to stop several + // arrows stacking on one point; when that guess leaves every route dirty, a Z's short + // stubs are what clip the box, and no choice of trunk lane can help. Spreading ports + // is a tidiness preference, and not drawing a line through a shape outranks it — so + // the assigned fraction is tried first and alternatives only if it fails. + for (const [pf, qf] of portTries) { + for (const lane of xLanes) + for (const es of ["L", "R"] as const) + for (const en of ["L", "R"] as const) + consider(es, en, { kind: "Zx", lane }, pf, qf) + for (const lane of yLanes) + for (const es of ["T", "B"] as const) + for (const en of ["T", "B"] as const) + consider(es, en, { kind: "Zy", lane }, pf, qf) + // One-bend routes, which are tidier when they happen to be clear. + for (const es of ["L", "R", "T", "B"] as const) + for (const en of ["L", "R", "T", "B"] as const) { + consider(es, en, { kind: "Lhv" }, pf, qf) + consider(es, en, { kind: "Lvh" }, pf, qf) + } + // Stop at the first fraction pair that yields a clean route: the assigned one is + // first, so a tidy answer is preferred whenever it exists. + if (best) break + } + return best + } + edges.forEach((e, i) => { const f = faces[i] const a = rects.get(e.source) @@ -497,6 +663,11 @@ export function routeEdges( const g = shapePoints(a, b, exitSide, entrySide, sf, tf, shape) const pts = [g.sp, ...g.wp, g.ep] if (pathHits(pts, exempt)) return null + // An arrow that leaves its own shape and immediately turns back across it reads as + // a mistake, and the obstacle test cannot see it: an edge is exempt from its own + // endpoints. Refused outright rather than scored, since there is never a reason to + // prefer it — the generator below will find a route that leaves cleanly. + if (doublesBack(pts, a, b)) return null if (strict && pathAlongFrame(pts, a, b)) return null // Strict mode also declines a lane an earlier edge already runs along. Waiting // for the nudge pass to pull them apart afterwards is worse: it can only move @@ -745,6 +916,7 @@ export function routeEdges( const g = shapePoints(a, b, es, en, sf, tf, shape) const pts = [g.sp, ...g.wp, g.ep] if (pathHits(pts, exempt)) continue + if (doublesBack(pts, a, b)) continue // Sharing a lane with an existing edge is weighed as heavily as trespassing // on a frame. Two lines drawn on top of each other are indistinguishable — // strictly worse to read than one line crossing a border it has to cross @@ -768,6 +940,80 @@ export function routeEdges( return best } + /** + * The graph search, for the edges a bounded candidate list cannot express. + * + * Same shape as `complete` — try the side pairs, keep the cheapest clear result — but + * each attempt is a full obstacle-avoiding search rather than one fixed shape, so it + * finds staircases of any number of bends. A* is optimal for a GIVEN pair of sides, + * which is why the sides are still enumerated out here. + */ + const viaGraph = ( + a: Rect, + b: Rect, + exempt: Set, + sf: number, + tf: number, + ): { + exitSide: Side + entrySide: Side + wp: Point[] + avoided: boolean + sf: number + tf: number + } | null => { + const obstacles = cards + .filter((c) => !exempt.has(c.id)) + .map((c) => c.r) + let best: { + exitSide: Side + entrySide: Side + wp: Point[] + avoided: boolean + sf: number + tf: number + } | null = null + let bestCost = Number.POSITIVE_INFINITY + + for (const es of ["L", "R", "T", "B"] as const) + for (const en of ["L", "R", "T", "B"] as const) { + const sp = portPoint(a, es, sf) + const ep = portPoint(b, en, tf) + const path = routeOrthogonal( + sp, + ep, + SIDE_DIR[es], + // The path must ARRIVE heading into the target's side, which is the + // reverse of the direction that side faces. + (SIDE_DIR[en] + 2) % 4, + obstacles, + { xs: [sp.x, ep.x], ys: [sp.y, ep.y] }, + ) + if (!path || path.length < 2) continue + const wp = path.slice(1, -1) + const pts = [sp, ...wp, ep] + if (pathHits(pts, exempt)) continue + if (doublesBack(pts, a, b)) continue + const cost = + frameOffences(pts, a, b) * 5000 + + (overlapsUsed(pts) ? 700 : 0) + + wp.length * 80 + + pathLength(pts) + if (cost < bestCost) { + bestCost = cost + best = { + exitSide: es, + entrySide: en, + wp, + avoided: wp.length > 0, + sf, + tf, + } + } + } + return best + } + // Strict first: a route that offends no frame wins outright. Failing that, score // every candidate and take the least-bad one. // @@ -777,14 +1023,28 @@ export function routeEdges( // relaxed pass takes whatever it happens to try first, which is how a line ends up // cutting diagonally across a whole VPC. Weighing the offences instead picks the // path that trespasses least and is shortest. - const chosen = ladder(true) ?? cheapest() + // A bounded candidate search covers all but a fraction of edges and produces tidier + // routes, so it goes first; the graph search is the backstop for what it cannot do. + // Measured over 250 generated flowcharts: the candidate search leaves 27 arrows + // crossing a box out of 2722 edges, and 21 of those need three bends — which is + // exactly the case a two-bend enumeration cannot express. + const viaComplete = + complete(a, b, exempt, sf, tf) ?? viaGraph(a, b, exempt, sf, tf) + // A route the generator found may need different port positions from the ones the + // de-collide pass assigned; record them, so the emitted connection points match the + // path that was actually verified clear. + if (viaComplete) { + frac[i].s = viaComplete.sf + frac[i].t = viaComplete.tf + } + const chosen = viaComplete ?? ladder(true) ?? cheapest() if (chosen) { routes.push(chosen) // Claim this route's lanes so the edges after it look elsewhere. claimLanes([ - portPoint(a, chosen.exitSide, sf), + portPoint(a, chosen.exitSide, frac[i].s), ...chosen.wp, - portPoint(b, chosen.entrySide, tf), + portPoint(b, chosen.entrySide, frac[i].t), ]) return } @@ -973,6 +1233,31 @@ export function routeEdges( if (!moved) break } + // --- stage 3b: a final check on the whole path + // + // The nudge pass judges each segment it moves on its own and reverts that segment if the + // path got worse. That is not the same as the path being clean: two segments can each be + // acceptable in isolation while their combination clips a box, and a revert restores only + // the segment last touched. Re-checking the finished path and restoring the route the + // search chose is what makes the guarantee hold end to end. + paths.forEach((P, i) => { + if (!P) return + const e = edges[i] + const exempt = new Set([e.source, e.target]) + const a = rects.get(e.source) + const b = rects.get(e.target) + if (!a || !b) return + if (!pathHits(P, exempt) && !doublesBack(P, a, b)) return + const r = routes[i] + const restored = [ + portPoint(a, r.exitSide, frac[i].s), + ...r.wp.map((p) => ({ x: p.x, y: p.y })), + portPoint(b, r.entrySide, frac[i].t), + ] + if (!pathHits(restored, exempt) && !doublesBack(restored, a, b)) + paths[i] = restored + }) + // --- emit const sideFraction = (side: Side, f: number) => side === "L" diff --git a/lib/diagram-engine/visgraph.ts b/lib/diagram-engine/visgraph.ts new file mode 100644 index 0000000..0c2e22e --- /dev/null +++ b/lib/diagram-engine/visgraph.ts @@ -0,0 +1,260 @@ +/** + * Obstacle-avoiding orthogonal routing: the orthogonal visibility graph, and A* over it. + * + * The router beside this file works by trying a list of candidate shapes — straight, an L, a + * Z with its trunk in some lane — and keeping the first that is clear. That can only ever be + * as good as the list, and a fixed list is not enough: measured over 250 generated + * flowcharts, 347 arrows were drawn through a box that had nothing to do with them. Adding + * shapes to the list moves the failures around rather than removing them. + * + * This is the complete alternative, from Wybrow, Marriott & Stuckey, "Orthogonal Connector + * Routing" (Graph Drawing 2009) — the algorithm behind libavoid. Two ideas make it work: + * + * 1. THE GRID IS FINITE AND SUFFICIENT. Take the "interesting points": every obstacle + * corner and every connection point. Their x-coordinates and y-coordinates define a + * grid. The paper's observation, with proof: for any valid orthogonal route there is a + * route using only this grid that is no longer and has no more bends — shrink each + * segment onto the nearest grid line. So searching the grid loses nothing, and there is + * no resolution to tune. This is what a uniform pixel grid gets wrong in both + * directions at once: too coarse and it cannot fit through a narrow gap, too fine and + * the search explodes. + * + * 2. THE STATE INCLUDES THE DIRECTION OF ARRIVAL. Bends have to be paid for, and whether + * the next step is a bend depends on which way this one came in. So a search state is + * (point, incoming direction), not just (point). Without that the cost function cannot + * see bends at all. + * + * The heuristic is the one libavoid uses: Manhattan distance to the target plus the minimum + * number of bends still needed, times the bend cost. It never overestimates — the remaining + * path is at least the straight-line Manhattan distance, and it must contain at least that + * many bends — so A* returns a cheapest route, not merely a route. + * + * Written from the paper's description rather than ported: the reference implementation is a + * C++ library built for interactive re-routing, with incremental scanline updates and pin + * management that a one-shot XML generator has no use for. + */ + +import type { Rect } from "./types" + +export interface Point { + x: number + y: number +} + +/** Which way a path segment travels. Indices are used as array offsets. */ +const DIRS = [ + { dx: 0, dy: -1 }, // 0 north + { dx: 1, dy: 0 }, // 1 east + { dx: 0, dy: 1 }, // 2 south + { dx: -1, dy: 0 }, // 3 west +] as const + +/** + * Cost of one bend, in pixels of path length. + * + * libavoid's default is 10. It has to be positive or the search has no reason to prefer a + * straight line to a staircase of the same length, and the two look nothing alike. + */ +const BEND_COST = 10 + +/** Clearance kept around an obstacle, matching the router's own margin. */ +const MARGIN = 7 + +/** + * Does the segment from `p` to `q` pass through any obstacle? + * + * Obstacles are expanded by `MARGIN` first, so a route grazing a border counts as a hit — + * an arrow drawn hard against a box reads as touching it. + */ +function blocked(p: Point, q: Point, obstacles: Rect[]): boolean { + const lo = { x: Math.min(p.x, q.x), y: Math.min(p.y, q.y) } + const hi = { x: Math.max(p.x, q.x), y: Math.max(p.y, q.y) } + for (const r of obstacles) { + if ( + lo.x < r.x + r.w + MARGIN && + hi.x > r.x - MARGIN && + lo.y < r.y + r.h + MARGIN && + hi.y > r.y - MARGIN + ) + return true + } + return false +} + +/** + * The minimum number of bends to get from `p`, travelling in direction `d`, to `t`. + * + * This is the table in the paper's Figure 2(a), as an arithmetic rule rather than sixteen + * cases. Two independent questions: is the target ahead along the current axis, and is it + * off to the side? Each answer costs bends, and they compose. + */ +function bendsToTarget(p: Point, d: number, t: Point): number { + const { dx, dy } = DIRS[d] + // How far the target lies along the direction of travel, and across it. + const along = dx !== 0 ? (t.x - p.x) * dx : (t.y - p.y) * dy + const across = dx !== 0 ? t.y - p.y : t.x - p.x + if (across === 0) { + // Dead ahead: no bend. Directly behind: out and back, two bends. + return along >= 0 ? 0 : 2 + } + // Off to the side: one bend if it is also ahead, two if it is behind. + return along > 0 ? 1 : 2 +} + +/** + * A cheapest obstacle-free orthogonal path from `from` to `to`, or null if none exists. + * + * `startDir` and `endDir` are the directions the path must leave and arrive by — the side of + * the shape each end attaches to. Constraining them is what stops an arrow leaving a box and + * immediately turning back across it: a departure direction the search must honour on its + * first step cannot double back. + * + * `extraLanes` lets the caller add grid lines the obstacles alone would not produce, which + * matters when a port sits somewhere other than an obstacle corner. + */ +export function routeOrthogonal( + from: Point, + to: Point, + startDir: number, + endDir: number, + obstacles: Rect[], + extraLanes: { xs: number[]; ys: number[] } = { xs: [], ys: [] }, +): Point[] | null { + // --- the interesting-points grid + const xs = new Set([from.x, to.x, ...extraLanes.xs]) + const ys = new Set([from.y, to.y, ...extraLanes.ys]) + for (const r of obstacles) { + // Just outside each edge, so a lane hugging an obstacle is still usable. + xs.add(r.x - MARGIN - 1) + xs.add(r.x + r.w + MARGIN + 1) + ys.add(r.y - MARGIN - 1) + ys.add(r.y + r.h + MARGIN + 1) + } + const X = [...xs].sort((a, b) => a - b) + const Y = [...ys].sort((a, b) => a - b) + const xi = new Map(X.map((v, i) => [v, i])) + const yi = new Map(Y.map((v, i) => [v, i])) + + const sx = xi.get(from.x) + const sy = yi.get(from.y) + const tx = xi.get(to.x) + const ty = yi.get(to.y) + if (sx == null || sy == null || tx == null || ty == null) return null + + // --- A* over (grid point, incoming direction) + const key = (ix: number, iy: number, d: number) => + (iy * X.length + ix) * 4 + d + const best = new Map() + const parent = new Map() + // A binary heap would be tidier, but the frontier stays small on diagram-sized inputs and + // a sorted insert keeps this readable. + const open: { ix: number; iy: number; d: number; g: number; f: number }[] = + [] + const push = (ix: number, iy: number, d: number, g: number, f: number) => { + let lo = 0 + let hi = open.length + while (lo < hi) { + const mid = (lo + hi) >> 1 + if (open[mid].f > f) lo = mid + 1 + else hi = mid + } + open.splice(lo, 0, { ix, iy, d, g, f }) + } + + const h = (ix: number, iy: number, d: number) => + Math.abs(X[ix] - to.x) + + Math.abs(Y[iy] - to.y) + + bendsToTarget({ x: X[ix], y: Y[iy] }, d, to) * BEND_COST + + const startKey = key(sx, sy, startDir) + best.set(startKey, 0) + push(sx, sy, startDir, 0, h(sx, sy, startDir)) + + // The path must ARRIVE travelling in `endDir`, so that is the only accepting state. + const goalKey = key(tx, ty, endDir) + let found = false + + while (open.length > 0) { + const cur = open.pop() as { + ix: number + iy: number + d: number + g: number + f: number + } + const ck = key(cur.ix, cur.iy, cur.d) + if (cur.g > (best.get(ck) ?? Number.POSITIVE_INFINITY)) continue + if (ck === goalKey) { + found = true + break + } + + const here = { x: X[cur.ix], y: Y[cur.iy] } + for (let nd = 0; nd < 4; nd++) { + // No reversing: it can never help, and it lets a path retrace itself. + if (nd === (cur.d + 2) % 4) continue + const { dx, dy } = DIRS[nd] + // Step to the NEXT grid line in this direction — the grid's whole point is that + // intermediate positions cannot change whether a route is clear. + const nix = cur.ix + dx + const niy = cur.iy + dy + if (nix < 0 || nix >= X.length || niy < 0 || niy >= Y.length) + continue + const next = { x: X[nix], y: Y[niy] } + if (blocked(here, next, obstacles)) continue + + const step = Math.abs(next.x - here.x) + Math.abs(next.y - here.y) + const g = cur.g + step + (nd === cur.d ? 0 : BEND_COST) + const nk = key(nix, niy, nd) + if (g >= (best.get(nk) ?? Number.POSITIVE_INFINITY)) continue + best.set(nk, g) + parent.set(nk, ck) + push(nix, niy, nd, g, g + h(nix, niy, nd)) + } + } + + if (!found) return null + + // --- rebuild, then drop the points that are not bends + const path: Point[] = [] + let node: number | undefined = goalKey + while (node !== undefined) { + const d = node % 4 + const rest = (node - d) / 4 + path.unshift({ + x: X[rest % X.length], + y: Y[(rest - (rest % X.length)) / X.length], + }) + node = parent.get(node) + } + return simplify(path) +} + +/** Drop collinear and duplicate points: draw.io renders a redundant waypoint as a kink. */ +function simplify(pts: Point[]): Point[] { + const out: Point[] = [] + for (const p of pts) { + const last = out[out.length - 1] + if (last && Math.abs(last.x - p.x) < 1 && Math.abs(last.y - p.y) < 1) + continue + out.push(p) + } + const kept: Point[] = [] + for (let i = 0; i < out.length; i++) { + if (i === 0 || i === out.length - 1) { + kept.push(out[i]) + continue + } + const prev = kept[kept.length - 1] + const next = out[i + 1] + const collinear = + (Math.abs(prev.x - out[i].x) < 1 && + Math.abs(out[i].x - next.x) < 1) || + (Math.abs(prev.y - out[i].y) < 1 && Math.abs(out[i].y - next.y) < 1) + if (!collinear) kept.push(out[i]) + } + return kept +} + +/** The direction leaving a given side of a shape: away from it. */ +export const SIDE_DIR = { T: 0, R: 1, B: 2, L: 3 } as const diff --git a/tests/unit/diagram-engine-graph.test.ts b/tests/unit/diagram-engine-graph.test.ts index eb70b6e..ee50923 100644 --- a/tests/unit/diagram-engine-graph.test.ts +++ b/tests/unit/diagram-engine-graph.test.ts @@ -302,3 +302,143 @@ describe("drawGraph: the whole pipeline", () => { expect(r.warnings.join(" ")).toContain("b→a") }) }) + +/** + * Two defects that both showed up as an arrow drawn over a box, on a flowchart with a + * "send it back" loop in it. Neither was visible on the smaller flowcharts above: one + * needs a back edge whose return path is lined with other steps, the other needs an edge + * that skips over a layer holding more than one node. + */ +describe("graphToOperations: arrows do not run through boxes", () => { + /** Ids of the real steps, excluding the engine's own layout cells. */ + const steps = (xml: string): string[] => + [...xml.matchAll(/]*dai_kind=(?:box|icon);/g)] + .map((m) => m[1]) + .filter((id) => !id.startsWith("__")) + + it("routes a back edge around the steps it returns past", () => { + // The reported case: "return for correction" goes back to "submit", and the two + // steps between them are directly in the way. The engine wraps each layer in an + // INVISIBLE container, and those used to be handed to the router as frames — so + // every clean return path was rejected for "trespassing" on a frame that is not + // drawn, and the fallback cut straight through both steps. + const r = drawGraph( + [ + { id: "start", label: "Start", shape: "terminator" }, + { id: "submit", label: "Submit Request", shape: "data" }, + n("validate", "Validate Input"), + { id: "valid", label: "Valid?", shape: "decision" }, + n("fix", "Return for Correction"), + n("review", "Manager Review"), + { id: "end", label: "End", shape: "terminator" }, + ], + [ + e("start", "submit"), + e("submit", "validate"), + e("validate", "valid"), + e("valid", "fix", "No"), + e("fix", "submit"), + e("valid", "review", "Yes"), + e("review", "end"), + ], + ) + expect(r.errors).toEqual([]) + const xml = r.xml as string + const rects = absoluteRects(xml) + expect( + nodeCollisions(edgePaths(xml, rects), rects, steps(xml)), + ).toEqual([]) + }) + + it("keeps the placeholders out of the reported layers", () => { + // They are an internal device for reserving space; a caller asked about its own nodes. + const { layers } = graphToOperations( + [n("a"), n("b1"), n("b2"), n("z")], + [ + e("a", "b1"), + e("a", "b2"), + e("b1", "z"), + e("b2", "z"), + e("a", "z"), + ], + ) + expect(layers.flat().filter((id) => id.startsWith("__"))).toEqual([]) + }) +}) + +/** + * An arrow that leaves its own shape and immediately turns back across it. + * + * Reported on a request-handling flowchart: a short hook came out of the right side of + * "Manager approval" and bent straight back over the box's own edge. The ordinary obstacle + * test cannot see this — an edge is exempt from its own two endpoints, and it has to be, + * since the line must touch them — so it is checked separately here. + */ +describe("graphToOperations: arrows leave their own shape cleanly", () => { + /** Waypoints that land inside any box, which is what draws the hook. */ + const waypointsInsideBoxes = (xml: string): string[] => { + const rects = absoluteRects(xml) + const boxes = [ + ...xml.matchAll(/]*dai_kind=(?:box|icon);/g), + ] + .map((m) => m[1]) + .filter((id) => !id.startsWith("__")) + const bad: string[] = [] + for (const p of edgePaths(xml, rects)) + for (const w of p.points.slice(1, -1)) + for (const id of boxes) { + const r = rects.get(id) + if (!r) continue + if ( + w.x > r.x + 1 && + w.x < r.x + r.w - 1 && + w.y > r.y + 1 && + w.y < r.y + r.h - 1 + ) + bad.push(`${p.source}→${p.target} bends inside ${id}`) + } + return [...new Set(bad)] + } + + it("does not bend an arrow back over the box it just left", () => { + const r = drawGraph( + [ + { + id: "start", + label: "Start: Request received", + shape: "terminator", + }, + n("intake", "Log request in system"), + n("review", "Review details"), + { id: "complete", label: "Info complete?", shape: "decision" }, + { id: "askinfo", label: "Request missing info", shape: "data" }, + { id: "approve", label: "Approval needed?", shape: "decision" }, + n("manager", "Manager approval"), + n("process", "Process request"), + { id: "report", label: "Generate report", shape: "document" }, + n("notify", "Notify requester"), + { id: "end", label: "End", shape: "terminator" }, + { id: "reject", label: "Reject & close", shape: "round" }, + ], + [ + e("start", "intake"), + e("intake", "review"), + e("review", "complete"), + e("complete", "askinfo", "no"), + e("askinfo", "review", "resubmit"), + e("complete", "approve", "yes"), + e("approve", "manager", "yes"), + e("approve", "process", "no"), + e("manager", "process", "approved"), + e("manager", "reject", "declined"), + e("process", "report"), + e("report", "notify"), + e("reject", "notify"), + e("notify", "end"), + ], + { title: "Sample Request Handling Workflow" }, + ) + expect(r.errors).toEqual([]) + expect(waypointsInsideBoxes(r.xml as string)).toEqual([]) + }) +})