bpmn-io / bpmn-auto-layout

Layout BPMN diagrams, generating missing DI information.
https://bpmn-io.github.io/bpmn-auto-layout/
50 stars 41 forks source link

Empty grid point behind task with boundary event #47

Closed till-stadtler closed 1 month ago

till-stadtler commented 2 months ago

Describe the Bug

When running the layouter, there is an unnecessary empty grid point behind a task with a boundary event: image

This empty grid point helps with the layout when there is a gateway behind the task, but only if there are not too many elements following the boundary event. So this "help" is rather limited. On the other side, this empty grid point makes the process longer than necessary and creates a lot of "empty space".

Steps to Reproduce

Layout a process with a task with a boundary event.

Expected Behavior

I expect there to be no empty grid point behind a task with a boundary event.

(In the best case, the layouter can understand how many element follow behind the boundary event, and react accordingly, either using such empty grid points, or shifting the elements behind the boundary event to lower grid points.)

Environment

till-stadtler commented 2 months ago

The empty space is gone when removing the following lines from attachersHandler.js:

  // Host has element directly after, add space
  if (grid.get(row, col + 1)) {
     grid.addAfter(host, null);
  }

I don't know if this has any other unwanted side effects that won't be fixed via the other bug issues.

abdul99ahad commented 2 months ago

The empty space is gone when removing the following lines from attachersHandler.js:

  // Host has element directly after, add space
  if (grid.get(row, col + 1)) {
     grid.addAfter(host, null);
  }

I don't know if this has any other unwanted side effects that won't be fixed via the other bug issues.

I have verified this, and it adds extra spaces after the element. @marstamm can confirm this further.

marstamm commented 2 months ago

This will not break anything as far as I can tell. For reference, this is the test case I added the "add space" for:

image

With the change, the joining looks a bit worse but I am fine with whatever call you make.

image

marstamm commented 2 months ago

You can run the tests and check the generated .bpmn files to check if it fundamentally breaks anything. Most things should be reflected there

till-stadtler commented 2 months ago

I am currently working on solving this issue by skipping layouting the merging gateway, and only layouting it when all incoming elements have been placed. (This has side effects, but it's looking good so far.)

philippfromme commented 2 months ago

@till-stadtler Are you planning to create pull request?

till-stadtler commented 2 months ago

@philippfromme pull request was created!

nikku commented 1 month ago

Still present in current main, cf. example:

image