diff --git a/packages/diagram-server/src/server/elk-layout-engine.ts b/packages/diagram-server/src/server/elk-layout-engine.ts index 98318f5..4c657d6 100644 --- a/packages/diagram-server/src/server/elk-layout-engine.ts +++ b/packages/diagram-server/src/server/elk-layout-engine.ts @@ -40,6 +40,23 @@ export const PORT_ORDER_FREE = 'free'; /** On a port the Free Ports pass has moved: the row the server gave it from the declaration. */ export const DECLARED_ROW_ARG = 'wf:declaredRowY'; +/** + * `elk.layered.priority.direction` of a wire that goes with the flow, in a + * diagram that has return wires. A handshake's `ready` answers backwards: it + * leaves an instance on the west, beside the `valid` it answers, and enters + * the queue on the east, so ELK sees a cycle for every channel and lays one + * of its wires against the flow. Its greedy cycle breaker counts wires, and a + * queue drives more than it takes (data, valid, size, ready, space against + * data, valid, ready), so every queue became a source and every actor a sink: + * a pipeline came out as a column of queues beside a column of actors. With + * the data wires at this priority and the return wires at zero, the return + * wires are the ones laid backwards; a real feedback loop still reverses a + * data wire, as it must. Measured on a ten-queue, six-actor netlist: two + * layers at priority 2 and below, eight from 5 up. A diagram without return + * wires gets no priority at all, so its layout is exactly what it was. + */ +export const FLOW_PRIORITY = '10'; + /** * Minimal CAL extension of the standard GLSP ELK layout engine. * @@ -55,6 +72,8 @@ export class WorkflowElkLayoutEngine extends GlspElkLayoutEngine { private readonly debugElkLevel: number = Number(process.env.WORKFLOW_DIAGRAM_DEBUG_ELK ?? '0') || 0; private debugDumped: boolean = false; private elkInputDumped: boolean = false; + /** Whether the graph being transformed has a wire that leaves a west port or enters an east port. */ + private hasReturnWires: boolean = false; constructor( elkFactory: ElkFactory, @@ -304,6 +323,7 @@ export class WorkflowElkLayoutEngine extends GlspElkLayoutEngine { * Layout options come from WorkflowLayoutConfigurator - we only add direction propagation here. */ protected override transformGraph(graph: GGraph): ElkNode { + this.hasReturnWires = this.findChildren(graph, GEdge).some(edge => this.isReturnWire(edge)); const elkGraph = super.transformGraph(graph) as ElkNode; // Forward any runtime-injected layoutOptions (e.g. interactive reroute tweaks). @@ -535,6 +555,15 @@ export class WorkflowElkLayoutEngine extends GlspElkLayoutEngine { } const elkEdge = super.transformEdge(edge); + // In a diagram with return wires, tell ELK which wires go with the flow + // (see FLOW_PRIORITY); the return wires are the ones to lay backwards. + if (this.hasReturnWires) { + (elkEdge as any).layoutOptions = { + ...(((elkEdge as any).layoutOptions ?? {}) as Record), + 'elk.layered.priority.direction': this.isReturnWire(edge) ? '0' : FLOW_PRIORITY + }; + } + // Forward runtime-injected edge.layoutOptions if present. const edgeLayoutOptions = this.stringifyLayoutOptions((edge as any)?.layoutOptions as Record | undefined); if (edgeLayoutOptions) { @@ -547,6 +576,24 @@ export class WorkflowElkLayoutEngine extends GlspElkLayoutEngine { return elkEdge; } + /** + * A wire drawn against the flow: it leaves a port its producer put on the + * west, or enters one put on the east. A handshake's `ready` is one; so is + * a wire from a boundary input placed in the east margin. + */ + private isReturnWire(edge: GEdge): boolean { + let source: GModelElement | undefined; + let target: GModelElement | undefined; + try { + source = this.modelState.index.get(edge.sourceId); + target = this.modelState.index.get(edge.targetId); + } catch { + return false; + } + return (source instanceof GPort && gportSide(source) === 'WEST') + || (target instanceof GPort && gportSide(target) === 'EAST'); + } + /** * For compound containers, accept ELK's computed size. * For boundary nodes, only apply position — never change their size. diff --git a/packages/diagram-server/test/elk-return-wires-lay-out-backwards.test.ts b/packages/diagram-server/test/elk-return-wires-lay-out-backwards.test.ts new file mode 100644 index 0000000..6431df8 --- /dev/null +++ b/packages/diagram-server/test/elk-return-wires-lay-out-backwards.test.ts @@ -0,0 +1,147 @@ +/** + * A handshake answers backwards: `ready` leaves an instance on the west, beside + * the `valid` it answers, and enters the queue on the east. ELK sees a cycle + * for every channel and has to lay one of its wires against the flow. Its + * greedy cycle breaker counts wires: a queue drives five (data, valid, size, + * ready, space) and takes three, so every queue became a source, every actor + * a sink, and a pipeline came out as a column of queues beside a column of + * actors. The engine now tells ELK which wires go with the flow: a wire that + * leaves a west port or enters an east port is the return wire and is the one + * to lay backwards. This pins that a chain producer → queue → consumer comes + * out in that order, on the layout the server really runs. + */ +import { describe, expect, it } from 'vitest'; +import ELK from 'elkjs/lib/elk.bundled.js'; +import { GGraph, GNode } from '@eclipse-glsp/server'; +import { DefaultElementFilter } from '@eclipse-glsp/layout-elk'; +import { WorkflowElkLayoutEngine } from '../src/server/elk-layout-engine'; +import { WorkflowLayoutConfigurator } from '../src/server/layout-configurator'; +import { GraphGModelSource, type PyGraphDocument, type PyGraphPort } from '../src/model/graph-gmodel-source'; +import { convertToGModelRoot } from '../src/server/gmodel-convert'; + +function find(element: any, id: string): any { + if (!element) { + return undefined; + } + if (element.id === id) { + return element; + } + for (const child of element.children ?? []) { + const hit = find(child, id); + if (hit) { + return hit; + } + } + return undefined; +} + +/** A handshake channel's ports on one node: the return wires sit beside the data. */ +function channel(node: string, name: string, dir: 'in' | 'out', extra: string[]): PyGraphPort[] { + const fwd = dir === 'out' ? 'out' : 'in'; + const back = dir === 'out' ? 'in' : 'out'; + const backSide = dir === 'out' ? 'east' : 'west'; + const port = (suffix: string, direction: 'in' | 'out', side?: string): PyGraphPort => + ({ id: `p:${node}:${name}${suffix}`, name: `${name}${suffix}`, direction, type: 'logic', group: name, ...(side ? { side } : {}) }); + return [ + port('', fwd), + port('_valid', fwd), + ...extra.filter(s => s === '_size').map(s => port(s, fwd)), + port('_ready', back, backSide), + ...extra.filter(s => s === '_space').map(s => port(s, back, backSide)) + ]; +} + +function wire(from: string, to: string): { id: string; from: string; to: string; scope: string } { + return { id: `${from}->${to}`, from, to, scope: 'scope:root' }; +} + +/** A channel between two nodes: data, valid and size one way, ready and space back. */ +function link(src: string, srcPort: string, dst: string, dstPort: string, extra: string[]) { + const data = ['', '_valid', ...extra.filter(s => s === '_size')]; + const back = ['_ready', ...extra.filter(s => s === '_space')]; + return [ + ...data.map(s => wire(`p:${src}:${srcPort}${s}`, `p:${dst}:${dstPort}${s}`)), + ...back.map(s => wire(`p:${dst}:${dstPort}${s}`, `p:${src}:${srcPort}${s}`)) + ]; +} + +/** + * producer → queue → consumer, each link a full handshake. The queue drives + * five wires and takes three, the shape that made it a source. + */ +const DOC: PyGraphDocument = { + version: '1.1', + graph: { + id: 'wf:core', + nodes: [ + { id: 'node:producer', kind: 'instance', label: 'producer', type: 'stage', scope: 'scope:root', + ports: channel('producer', 'out0', 'out', ['_size', '_space']) }, + { id: 'node:queue', kind: 'instance', label: 'queue', type: 'handshake_queue', scope: 'scope:root', + ports: [...channel('queue', 'ins', 'in', ['_space']), ...channel('queue', 'outs', 'out', ['_size'])] }, + { id: 'node:consumer', kind: 'instance', label: 'consumer', type: 'stage', scope: 'scope:root', + ports: channel('consumer', 'in0', 'in', ['_size']) } + ], + edges: [ + ...link('producer', 'out0', 'queue', 'ins', ['_space']), + ...link('queue', 'outs', 'consumer', 'in0', ['_size']) + ] + } +}; + +async function layout(doc: PyGraphDocument): Promise { + const root = convertToGModelRoot(new GraphGModelSource({} as any).transform(doc) as any) as unknown as GGraph; + const modelState: any = { + root, + index: { get: (id: string) => find(root, id), findByClass: (id: string) => find(root, id) } + }; + const engine = new WorkflowElkLayoutEngine( + () => new (ELK as any)({ algorithms: ['layered'] }), + new DefaultElementFilter(), + new WorkflowLayoutConfigurator(), + modelState + ); + await engine.layout(); + return root; +} + +function x(root: GGraph, id: string): number { + return (find(root, id) as GNode).position!.x; +} + +describe('WorkflowElkLayoutEngine lays return wires backwards', () => { + it('keeps a handshake pipeline in dataflow order, the queue between producer and consumer', async () => { + const root = await layout(DOC); + expect(x(root, 'producer')).toBeLessThan(x(root, 'queue')); + expect(x(root, 'queue')).toBeLessThan(x(root, 'consumer')); + }); + + it('leaves a graph without return wires exactly as before: no priority on any wire', async () => { + const plain: PyGraphDocument = { + version: '1.1', + graph: { + id: 'wf:plain', + nodes: [ + { id: 'node:a', kind: 'instance', label: 'a', type: 't', scope: 'scope:root', + ports: [{ id: 'p:a:d', name: 'd', direction: 'in', type: 'logic' }, { id: 'p:a:q', name: 'q', direction: 'out', type: 'logic' }] }, + { id: 'node:b', kind: 'instance', label: 'b', type: 't', scope: 'scope:root', + ports: [{ id: 'p:b:d', name: 'd', direction: 'in', type: 'logic' }, { id: 'p:b:q', name: 'q', direction: 'out', type: 'logic' }] } + ], + edges: [wire('p:a:q', 'p:b:d'), wire('p:b:q', 'p:a:d')] + } + }; + const root = convertToGModelRoot(new GraphGModelSource({} as any).transform(plain) as any) as unknown as GGraph; + const modelState: any = { root, index: { get: (id: string) => find(root, id), findByClass: (id: string) => find(root, id) } }; + const seen: Array | undefined> = []; + const engine = new WorkflowElkLayoutEngine( + () => ({ layout: async (g: any) => { for (const e of g.edges ?? []) { seen.push(e.layoutOptions); } return g; } }) as any, + new DefaultElementFilter(), + new WorkflowLayoutConfigurator(), + modelState + ); + await engine.layout(); + expect(seen.length).toBe(2); + for (const options of seen) { + expect(options?.['elk.layered.priority.direction']).toBeUndefined(); + } + }); +});