Skip to content

Commit 2c928bc

Browse files
fix(intent): lay out generated BPMN/EDM diagrams so they read cleanly (#6299)
The intent generators emitted diagram interchange (bpmndi for .bpmn, mxGraphModel for .edm) with naive placement, so branching processes and larger entity models opened badly formatted and had to be reordered by hand. BPMN (BpmnIntentGenerator): replace the single-line, declaration-order placement with a deterministic layered (Sugiyama-style) layout. A node's column (X) is its longest-path distance from the start event, so a gateway's then/else branches share a column and the flow reads strictly left-to-right; its lane (Y) is assigned per column by the barycentre of its predecessors' lanes and spread symmetrically around a centre lane, so branches fan out above and below instead of piling onto the two fixed lanes (140/300) the old code used. Edges are routed orthogonally (right-angle L/Z) when their endpoints are on different lanes, straight when aligned. The obsolete secondaryBranchTargets single-drop heuristic is removed. EDM (EdmIntentGenerator.appendMxGraphModel): place entities in a relationship-aware order (breadth-first over the FK graph so connected entities cluster) and pack each into the currently shortest column instead of a blind index % 3, so columns stay balanced regardless of card height and FK-linked entities land near each other. Cells are still emitted in declaration order, keeping regeneration diffs stable. Both layouts are pure functions of the model, so output stays byte-stable; the modeler re-routes on first manual edit. Existing engine-intent unit tests and IntentEngineIT/IntentEmissionCoverageIT assertions key on element/flow presence, not coordinates, so they are unaffected. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent d6566e1 commit 2c928bc

2 files changed

Lines changed: 250 additions & 94 deletions

File tree

‎components/engine/engine-intent/src/main/java/org/eclipse/dirigible/components/intent/generator/bpmn/BpmnIntentGenerator.java‎

Lines changed: 152 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,11 @@
99
*/
1010
package org.eclipse.dirigible.components.intent.generator.bpmn;
1111

12-
import java.util.ArrayDeque;
1312
import java.util.ArrayList;
14-
import java.util.Deque;
1513
import java.util.HashMap;
1614
import java.util.HashSet;
1715
import java.util.LinkedHashMap;
16+
import java.util.LinkedHashSet;
1817
import java.util.List;
1918
import java.util.Locale;
2019
import java.util.Map;
@@ -67,8 +66,10 @@
6766
* <p>
6867
* The <b>{@code bpmndi:BPMNDiagram}</b> block IS emitted (see {@link #appendBpmnDiagram}): the
6968
* Flowable/Oryx modeler renders the canvas only from the diagram interchange, so a process with no
70-
* shapes opens empty. Nodes are laid out left-to-right on a fixed lane for deterministic,
71-
* byte-stable output; the modeler re-routes on first manual edit.
69+
* shapes opens empty. Nodes are laid out with a deterministic layered (Sugiyama-style) placement -
70+
* column by longest-path distance from the start event, lane by predecessor barycentre - with
71+
* orthogonally-routed edges, so branching processes read cleanly without manual reordering; the
72+
* output stays byte-stable and the modeler re-routes on first manual edit.
7273
*
7374
* <p>
7475
* <b>{@code flowable:formKey} is the form page URL.</b> The Inbox / Process perspective opens a
@@ -771,42 +772,34 @@ private static StepIntent decisionOf(String stepId, List<StepIntent> steps) {
771772

772773
// ----- BPMN diagram interchange (bpmndi) ---------------------------------------------------
773774

774-
private static final int LANE_Y = 140;
775-
/**
776-
* The lane a decision's secondary (default / {@code else}) branch target sits on, dropped below the
777-
* main lane so the gateway's two outgoing flows diverge visibly instead of overlapping on one line.
778-
*/
779-
private static final int SECONDARY_LANE_Y = 300;
780-
private static final int NODE_SPACING = 160;
775+
/** X of the first (start) column's centre. */
781776
private static final int FIRST_NODE_CENTER_X = 100;
777+
/** Horizontal distance between the centres of adjacent rank columns (task width 100 + gap). */
778+
private static final int COLUMN_PITCH = 180;
779+
/** Y of the centre lane (lane offset 0); branches fan out above and below it. */
780+
private static final int CENTER_Y = 260;
781+
/** Vertical distance between adjacent lanes. */
782+
private static final int LANE_HEIGHT = 130;
782783

783784
/**
784785
* Append the {@code bpmndi:BPMNDiagram} block. The Flowable/Oryx modeler renders the canvas
785786
* <b>only</b> from this diagram interchange (a process with no {@code BPMNShape}s opens empty), so
786-
* it is mandatory. Nodes are laid out left-to-right along the linear chain at a fixed lane, except
787-
* a decision's secondary ({@code else}) branch target, which drops to a lower lane so the gateway's
788-
* two outgoing flows are visibly distinct rather than overlapping on one line; edges connect the
789-
* right edge of the source to the left edge of the target. The layout is deterministic so
790-
* re-generation is byte-stable; the modeler re-routes on first manual edit.
787+
* it is mandatory.
788+
*
789+
* <p>
790+
* The layout is a deterministic <b>layered (Sugiyama-style) placement</b> rather than a single
791+
* line: each node's <b>column</b> (X) is its longest-path distance from the start event, so
792+
* parallel branches of a gateway share a column and the flow always reads left-to-right; each
793+
* node's <b>lane</b> (Y) is assigned per column by the barycentre of its predecessors' lanes and
794+
* centred on {@link #CENTER_Y}, so branches fan out above and below the main line instead of piling
795+
* onto one of two fixed lanes. Edges are routed <b>orthogonally</b> (an L/Z of right-angle
796+
* segments) when the endpoints are on different lanes, and as a straight segment when they line up
797+
* - the same shape a modeler draws by hand. The whole computation is a pure function of the step
798+
* graph, so re-generation stays byte-stable; the modeler re-routes on first manual edit.
791799
*/
792800
private static void appendBpmnDiagram(StringBuilder sb, String processId, List<String> effectiveIds, List<SequenceFlow> flows,
793801
List<StepIntent> steps) {
794-
// A decision's secondary branch target (the default / `else` flow's target) is dropped to a lower
795-
// lane so the gateway's two outgoing flows are visibly distinct instead of running along one line.
796-
Set<String> secondaryNodes = secondaryBranchTargets(flows);
797-
Map<String, int[]> bounds = new java.util.LinkedHashMap<>();
798-
for (int i = 0; i < effectiveIds.size(); i++) {
799-
String id = effectiveIds.get(i);
800-
if (bounds.containsKey(id)) {
801-
continue;
802-
}
803-
int[] size = nodeSize(id, steps);
804-
int centerX = FIRST_NODE_CENTER_X + i * NODE_SPACING;
805-
int laneY = secondaryNodes.contains(id) ? SECONDARY_LANE_Y : LANE_Y;
806-
int x = centerX - size[0] / 2;
807-
int y = laneY - size[1] / 2;
808-
bounds.put(id, new int[] {x, y, size[0], size[1]});
809-
}
802+
Map<String, int[]> bounds = layout(effectiveIds, flows, steps);
810803

811804
sb.append(" <bpmndi:BPMNDiagram id=\"BPMNDiagram_")
812805
.append(escapeXmlAttribute(processId))
@@ -838,75 +831,151 @@ private static void appendBpmnDiagram(StringBuilder sb, String processId, List<S
838831
if (source == null || target == null) {
839832
continue;
840833
}
841-
int x1 = source[0] + source[2];
842-
int y1 = source[1] + source[3] / 2;
843-
int x2 = target[0];
844-
int y2 = target[1] + target[3] / 2;
845834
sb.append(" <bpmndi:BPMNEdge bpmnElement=\"")
846835
.append(escapeXmlAttribute(flow.id()))
847836
.append("\" id=\"BPMNEdge_")
848837
.append(escapeXmlAttribute(flow.id()))
849-
.append("\">\n <omgdi:waypoint x=\"")
850-
.append(x1)
851-
.append("\" y=\"")
852-
.append(y1)
853-
.append("\"/>\n <omgdi:waypoint x=\"")
854-
.append(x2)
855-
.append("\" y=\"")
856-
.append(y2)
857-
.append("\"/>\n </bpmndi:BPMNEdge>\n");
838+
.append("\">\n");
839+
for (int[] point : edgeWaypoints(source, target)) {
840+
sb.append(" <omgdi:waypoint x=\"")
841+
.append(point[0])
842+
.append("\" y=\"")
843+
.append(point[1])
844+
.append("\"/>\n");
845+
}
846+
sb.append(" </bpmndi:BPMNEdge>\n");
858847
}
859848
sb.append(" </bpmndi:BPMNPlane>\n");
860849
sb.append(" </bpmndi:BPMNDiagram>\n");
861850
}
862851

863852
/**
864-
* The nodes of each decision's default ({@code else}) branch - its "second option" - placed on the
865-
* lower lane so the gateway's flows diverge and a later main-lane edge (e.g. a {@code next: end}
866-
* jump) does not visually cross them. Starts from each default-flow target and walks the branch
867-
* forward along the sequence flows, collecting every node until it reaches {@code end} or rejoins
868-
* the main path (a node entered by a conditioned {@code then} flow). The end event is never
869-
* dropped.
870-
* <p>
871-
* Without the full walk, only the immediate target dropped: a multi-node reject branch (e.g.
872-
* {@code else -> cancel} followed by more steps) left those steps on the main lane between
873-
* {@code send} and {@code end}, so the {@code send -> end} edge ran straight through them and
874-
* looked like {@code send -> cancel}.
853+
* Compute the {@code [x, y, width, height]} bounds of every node with a layered layout: column by
854+
* longest-path rank from the start event, lane by predecessor-barycentre within the column. The
855+
* returned map preserves {@code effectiveIds} order so the emitted shapes are byte-stable.
875856
*/
876-
private static Set<String> secondaryBranchTargets(List<SequenceFlow> flows) {
877-
Map<String, List<String>> adjacency = new HashMap<>();
878-
Set<String> thenTargets = new HashSet<>();
879-
Deque<String> frontier = new ArrayDeque<>();
857+
private static Map<String, int[]> layout(List<String> effectiveIds, List<SequenceFlow> flows, List<StepIntent> steps) {
858+
// Forward adjacency and predecessor lists, restricted to nodes that actually have a shape.
859+
Set<String> nodes = new LinkedHashSet<>(effectiveIds);
860+
Map<String, List<String>> predecessors = new LinkedHashMap<>();
861+
for (String id : effectiveIds) {
862+
predecessors.put(id, new ArrayList<>());
863+
}
880864
for (SequenceFlow flow : flows) {
881-
adjacency.computeIfAbsent(flow.source(), k -> new ArrayList<>())
882-
.add(flow.target());
883-
if (flow.id() == null) {
884-
continue;
865+
if (nodes.contains(flow.source()) && nodes.contains(flow.target()) && !flow.source()
866+
.equals(flow.target())) {
867+
predecessors.get(flow.target())
868+
.add(flow.source());
869+
}
870+
}
871+
872+
Map<String, Integer> rank = rankByLongestPath(effectiveIds, flows, nodes);
873+
// Group nodes by rank in a stable (effectiveIds) order, then compress ranks to contiguous
874+
// columns so an empty rank leaves no visual gap.
875+
Map<Integer, List<String>> byRank = new java.util.TreeMap<>();
876+
for (String id : effectiveIds) {
877+
byRank.computeIfAbsent(rank.get(id), k -> new ArrayList<>())
878+
.add(id);
879+
}
880+
Map<Integer, Integer> columnOf = new HashMap<>();
881+
int column = 0;
882+
for (Integer r : byRank.keySet()) {
883+
columnOf.put(r, column++);
884+
}
885+
886+
// Lane assignment, one column at a time in rank order: sort a column's nodes by the average
887+
// lane of their already-placed predecessors (barycentre - the classic crossing-reduction
888+
// heuristic), then spread them symmetrically around the centre lane (offset 0).
889+
Map<String, Double> laneOf = new HashMap<>();
890+
for (List<String> columnNodes : byRank.values()) {
891+
columnNodes.sort(java.util.Comparator.<String>comparingDouble(id -> barycentre(id, predecessors, laneOf))
892+
.thenComparingInt(effectiveIds::indexOf));
893+
double first = -(columnNodes.size() - 1) / 2.0;
894+
for (int i = 0; i < columnNodes.size(); i++) {
895+
laneOf.put(columnNodes.get(i), first + i);
885896
}
886-
if (flow.id()
887-
.endsWith("_then")) {
888-
thenTargets.add(flow.target());
889-
} else if (flow.id()
890-
.endsWith("_default")
891-
&& !END_ID.equals(flow.target())) {
892-
frontier.add(flow.target());
893-
}
894-
}
895-
// Forward walk of the else branch(es). Stop at the end event and where the branch rejoins the
896-
// main path (a `then` target), so a shared convergence/end node stays on the main lane.
897-
Set<String> secondary = new HashSet<>();
898-
while (!frontier.isEmpty()) {
899-
String node = frontier.poll();
900-
if (node == null || END_ID.equals(node) || thenTargets.contains(node) || !secondary.add(node)) {
897+
}
898+
899+
Map<String, int[]> bounds = new LinkedHashMap<>();
900+
for (String id : effectiveIds) {
901+
if (bounds.containsKey(id)) {
901902
continue;
902903
}
903-
for (String next : adjacency.getOrDefault(node, List.of())) {
904-
if (!END_ID.equals(next) && !thenTargets.contains(next)) {
905-
frontier.add(next);
904+
int[] size = nodeSize(id, steps);
905+
int centerX = FIRST_NODE_CENTER_X + columnOf.get(rank.get(id)) * COLUMN_PITCH;
906+
int centerY = CENTER_Y + (int) Math.round(laneOf.get(id) * LANE_HEIGHT);
907+
bounds.put(id, new int[] {centerX - size[0] / 2, centerY - size[1] / 2, size[0], size[1]});
908+
}
909+
return bounds;
910+
}
911+
912+
/**
913+
* Longest-path rank of each node from {@link #START_ID}, computed by Bellman-Ford-style relaxation
914+
* (bounded to {@code |nodes|} passes so a stray back edge cannot loop forever). The end event is
915+
* pinned to the deepest rank so nothing sits to its right.
916+
*/
917+
private static Map<String, Integer> rankByLongestPath(List<String> effectiveIds, List<SequenceFlow> flows, Set<String> nodes) {
918+
Map<String, Integer> rank = new HashMap<>();
919+
for (String id : effectiveIds) {
920+
rank.put(id, 0);
921+
}
922+
for (int pass = 0; pass < nodes.size(); pass++) {
923+
boolean changed = false;
924+
for (SequenceFlow flow : flows) {
925+
Integer source = rank.get(flow.source());
926+
Integer target = rank.get(flow.target());
927+
if (source == null || target == null || flow.source()
928+
.equals(flow.target())) {
929+
continue;
930+
}
931+
if (target < source + 1) {
932+
rank.put(flow.target(), source + 1);
933+
changed = true;
906934
}
907935
}
936+
if (!changed) {
937+
break;
938+
}
939+
}
940+
int deepest = rank.values()
941+
.stream()
942+
.mapToInt(Integer::intValue)
943+
.max()
944+
.orElse(0);
945+
rank.put(END_ID, deepest);
946+
return rank;
947+
}
948+
949+
/** Average lane of a node's already-placed predecessors, or {@code 0} when none are placed yet. */
950+
private static double barycentre(String id, Map<String, List<String>> predecessors, Map<String, Double> laneOf) {
951+
double sum = 0;
952+
int count = 0;
953+
for (String predecessor : predecessors.getOrDefault(id, List.of())) {
954+
Double lane = laneOf.get(predecessor);
955+
if (lane != null) {
956+
sum += lane;
957+
count++;
958+
}
959+
}
960+
return count == 0 ? 0 : sum / count;
961+
}
962+
963+
/**
964+
* Waypoints for an edge from the {@code source} bounds to the {@code target} bounds: a straight
965+
* right-to-left segment when the two shapes share a lane, otherwise an orthogonal L/Z stepping out
966+
* of the source's right side, across to the midpoint column, up or down to the target's lane, and
967+
* into the target's left side.
968+
*/
969+
private static List<int[]> edgeWaypoints(int[] source, int[] target) {
970+
int x1 = source[0] + source[2];
971+
int y1 = source[1] + source[3] / 2;
972+
int x2 = target[0];
973+
int y2 = target[1] + target[3] / 2;
974+
if (y1 == y2) {
975+
return List.of(new int[] {x1, y1}, new int[] {x2, y2});
908976
}
909-
return secondary;
977+
int midX = (x1 + x2) / 2;
978+
return List.of(new int[] {x1, y1}, new int[] {midX, y1}, new int[] {midX, y2}, new int[] {x2, y2});
910979
}
911980

912981
/** Width/height of a node's shape by its element id / step kind. */

0 commit comments

Comments
 (0)