kie-issues#451: Implement autolayout on the new React-based DMN Editor for DMN files without Diagram information (part 1) #2130
Conversation
Much better... Tiny refactorings big big big refactor More changes.. Fixed external dependencies problem for computed values Best version so far Minor improvements Simplify code Removed duplicated code from Autolayout and improved disptach and computed mechanisms renames Comment loggings EOD Memoize edges Better autolayout when Decision Sevices are involved Fix emtpy or semi-empty DS growing with every autolayout click Minor updates restore uniqueness validation to nodes Improving last commit
There was a problem hiding this comment.
@tiagobento thank you for this important PR! I tried to focus on the autolayout feature and the 'ELK' configuration as you asked in the private chat. I tried different kinds of graphs, so I will try to summarize them one by one.
01 Complete binary tree
In your PR it seems to be aligned left [1]. I was able to center it using one of these properties:
"elk.layered.nodePlacement.bk.fixedAlignment": "BALANCED","elk.radial.centerOnRoot": "true",
Then the layout was like [2].
02 Complete ternary tree
Situation pretty similar as for 01. Using one of bellow:
"elk.layered.nodePlacement.bk.fixedAlignment": "BALANCED","elk.radial.centerOnRoot": "true",
centered the tree.
03 Non complete trees
It means binary and ternary trees with some nodes missing. See my used models.
here it started to be better "elk.layered.nodePlacement.bk.fixedAlignment": "BALANCED", over "elk.radial.centerOnRoot": "true", however the main problem I see, is that still the tree leafs are always in the most bottom layer [3]. vs. what I think would be more readable [4] (created manually).
04 Diamond style models
In models where ich node is connected with almost every other node, the layered autolayout isn't ideal. I tried to play a little bit with these properties, but no effect achieved:
elk.layered.cycleBreaking.strategy....crossing...
Still [5] resulted into [6]. Here is model I used:
05 Different node sizes
Seems to work fine in combination with mentioned BALANCED [7]. Used model:
06 Each node type
Seems to work fine. [8]. Model I used:
07 trees connected with single edge
Results into not ideal from my point of view, but again, was not able to improve. See [9] and [10].
Model I used:
[1]
[2]
[3]
[4]
[5]
[6]
[7]
[8]
[9]
[10]
| export const ELK_OPTIONS = { | ||
| "elk.algorithm": "layered", | ||
| "elk.direction": "UP", | ||
| // By making width a lot bigger than height, we make sure disjoint graph components are placed horizontally, never vertically | ||
| "elk.aspectRatio": "9999999999", | ||
| // spacing | ||
| "elk.spacing.nodeNode": "60", | ||
| "elk.spacing.componentComponent": "200", | ||
| "layered.spacing.edgeEdgeBetweenLayers": "0", | ||
| "layered.spacing.edgeNodeBetweenLayers": "0", | ||
| "layered.spacing.nodeNodeBetweenLayers": "100", | ||
| // edges | ||
| "elk.edgeRouting": "ORTHOGONAL", | ||
| "elk.layered.mergeEdges": "true", // we need this to make sure space is consistent between layers. | ||
| "elk.layered.mergeHierarchyEdges": "true", | ||
| // positioning | ||
| "elk.partitioning.activate": "true", | ||
| "elk.nodePlacement.favorStraightEdges": "true", | ||
| "elk.nodePlacement.bk.fixedAlignment": "LEFTDOWN", | ||
| "elk.nodePlacement.bk.edgeStraightening": "IMPROVE_STRAIGHTNESS", | ||
| // | ||
| "considerModelOrder.strategy": "PREFER_NODES", | ||
| "crossingMinimization.forceNodeModelOrder": "true", | ||
| "layering.strategy": "LONGEST_PATH_SOURCE", | ||
| }; |
There was a problem hiding this comment.
it is minor comment, however for readability and clarity I think we could unify naming of elk options to all start as elk. what do you think?
I think it would make easier to understand if all options are some elk option or some are custom introduced by ourselves.
I think similar change would be applicable also on the line 152
There was a problem hiding this comment.
I'm afraid not all properties work with elk at the start, I remember I tried it... :/
|
@jomarko Thanks for the review! I intentionally made it left-aligned, as I didn't want to risk formatting the diagram and having the most important nodes out of the frame.... I guess it looks odd because of the edges, I guess.. and ELK seems to make some weird choices in regards to ordering the nodes on a same layer.. I'm not sure how to improve that either.. Maybe @danielzhe could help? :D |
There was a problem hiding this comment.
Awesome PR @tiagobento . A lot of improvements on the diagram memoization. I've made just some general comments regarding some leftovers and TODOs/FIXMEs without issues.
| // TODO: Decide what to do with Decision Services and multiple DRDs. | ||
| // | ||
| // When adding a Decision Service to a DRD, we need to bring all its encapsulated and output Decisions with it, already formatted with autolayout. | ||
| // | ||
| // if (drgElement.__$$element === "decisionService") { | ||
| // const defaultDecisionDimensions = DEFAULT_NODE_SIZES[NODE_TYPES.decision](state.diagram.snapGrid); | ||
|
|
||
| // let i = 0; | ||
| // for (const decision of [ | ||
| // ...(drgElement.outputDecision ?? []), | ||
| // ...(drgElement.encapsulatedDecision ?? []), | ||
| // ]) { | ||
| // i++; | ||
|
|
||
| // const decisionHref = parseXmlHref(decision["@_href"]); | ||
|
|
||
| // const decisionQNamePrefix = decisionHref.namespace | ||
| // ? getXmlNamespaceDeclarationName({ | ||
| // model: state.dmn.model.definitions, | ||
| // namespace: decisionHref.namespace, | ||
| // }) | ||
| // : undefined; | ||
|
|
||
| // // If there's a shape for this Decision in the DRD already, we don't need to add anything. | ||
| // // If not, we add a new shape for it. | ||
| // if (!state.computed(state).indexes().dmnShapesByHref.has(decision["@_href"])) { | ||
| // addShape({ | ||
| // definitions: state.dmn.model.definitions, | ||
| // drdIndex: state.diagram.drdIndex, | ||
| // nodeType, | ||
| // shape: { | ||
| // "@_dmnElementRef": buildXmlQName({ | ||
| // type: "xml-qname", | ||
| // localPart: decisionHref.id, | ||
| // prefix: decisionQNamePrefix, | ||
| // }), | ||
| // "dc:Bounds": { | ||
| // "@_x": dropPoint.x + 40 * i, | ||
| // "@_y": dropPoint.y + 40 * i, | ||
| // "@_width": defaultDecisionDimensions["@_width"], | ||
| // "@_height": defaultDecisionDimensions["@_height"], | ||
| // }, | ||
| // }, | ||
| // }); | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Maybe creating an issue? I don't think it's a good idea to keep it commented.
There was a problem hiding this comment.
Good catch. I'll create an issue and remove the commented code.
There was a problem hiding this comment.
Doesn't finish apache/incubator-kie-issues#451 yet...
This PR introduces a new button that automatically positions nodes on a DRD.
More than that, this PR fixed some bugs related to multi-DRD editing, and also introduces a brand new mechanism to cache nodes and allow big DMNs to maintain a good FPS while editing diagrams. The main change here is the creation of the
computedstate property, which replaces the DerivedState context, allowing Zustand selectors to be used for efficient caching, together with React.memo andfast-deep-equal.For part 2:
For the future: