Closed MartinMay closed 4 years ago
There is one minor issue where adding expanded subprocesses causes padding after the subprocesses. I'll work on it as a separate branch/PR.
As it stands, this PR fixes more then it breaks
Could you quickly walk me through the changes you propose?
What do they fix, why is adding the ECOTree thing a good addition?
It is not clear to me from the commit message (and the inlined comment) what you've actually fixed.
@nikku not a problem.
The main reason for this change is that AutoLayout using breadth first search only creates unreadable gateways. The problem being is that all elements at a particular depth are drawn together. Visually, each gateway branch is not drawn separately and given enough room to appear seperate from other silblings or paths.
I came to the conclusion that a bpmn model is just a tree that at it's leaf nodes closes in on itself; treating each bpmn element as a node and treating then entire bpmn as a tree allow us to abstract away from bpmn-moddel with regards to layout and positioning. (I actually use a tree to create my original bpmn)
basically, this PR does the following: 1.) uses the incoming bpmn to create a node based tree 2.) an abstract root node receives all process start nodes as children, thereby handling multiple start events 3.) padding nodes are also added to ensure connections do not run into sibling branches 4.) uses a tree node positioning algorithm to position each node bases on parents 5.) when nodes are positioned, uses bpmn-moddel breath first to work through the bpmn creating bpmnDi elements using the tree node positions
Visually, the changes are as follows: 1.) I created a bpmn that uses gateways using the bpmn modeller app
2.) using bpmn-moddle-auto-layout master to layout the bpmn will result in ...
3.) using this branch will provide the following results
To reiterate the case for using a tree; I believe there needs to be a layer of abstraction that allows each bpmn element to be positioned in relation to its:
at the very least, the tree root node abstraction handles multiple start events
I'll give this a try with a number of test diagrams. Happy to merge this if I see there is no unwanted consequences (or rather an overall improvement).
Could you remove standard and ESlint configuration from your PR?
I'll add eslint
, a basic number or tests and the lint rules we commonly use in our sphere.
@nikku if you can setup the linting style that you require, I can make the required linting changes.
Futher, if we can get a testing framework setup with some basic tests, it'll make identifying and fixing issues easier.
I like your commit messages by the way. Makes for a :cool: changelog when being published :heart:.
Heads-up: I did a lot of clean-up, added basic visual test coverage, integrated CI. Plus, I've renamed the repository to bpmn-auto-layout
.
A lot of very basic test cases fail, still (cf. AutoLayoutSpec
-> it.skip
).
As part of the tests a test/generated/test.html
file is created. This one contains before -> after visualizations.
As an example for something that simply blows up the layouter right now (simple.bpmn
):
@nikku thanks, I'll review the changes and pick it up
feature(tree): Tree node position algorithm used to position bpmn elements
fix(gateways): nested gateways no longer overlap
fix(start events): multiple start events supported (issue #4)
feature(linting): added eslint, made linting changes chore(DiFactoryUtils): split out DiFactoryUtils into seperate file