[POC] Alternative terminal point implementation for 'no orthogonal segments'

[tbouffard]

⚠️ THIS IS A POC so it is not intended to be merged, please don't rebase nor force push. Tests are failing and won't be fixed ⚠️

Only use the implementation involving the perimeter projection when there is no waypoint. Otherwise, use the original first and last waypoints.
This avoids side effects when terminal waypoints are inside the shapes and - in some cases - outside shapes.

Drawbacks: as the terminal waypoints can be inside or outsides shapes, the rendering of such edge cases may be less pretty than before.

covers #295
covers #349
covers #715
covers #994
covers #1236
covers #1368

Details and notes

Benefits: this implementation avoids all side effects described in #1779 (comment)
It also fixes #715 and #1368 (see visual non regression test screenshots updated here)

About the created drawbacks: we could later implement a new floating point computation. Get the intersection of the perimeter with the existing last segment instead of doing a projection. That way, the point on the perimeter would be always aligned with the last existing segment and wouldn't create glitches with arrows like #351 (comment) (we have visual non regression test to detect such problems and fixes)
If we go on with this implementation, I will create a specific issue about the new behavior.

On this topic, bpmn-js manages inside terminal waypoints by always putting the shape on top of the edge and use opacity with the shape fill color. mxGraph provides a way to always put shapes on top of edges
See the documentation about keepEdgesInBackground and keepEdgesInForeground.

Notes:

• e2e tests are failing as all screenshots haven't been updated nor the thresholds
• some screenshots in particular those for the BPMN rendering tests have been updated. They exhibit the drawbacks for terminal waypoints inside/outside shapes, and demonstrate all improvements related to edges
• the changes has very limited impacts on fit/zoom visual tests
• also tested with [email protected]
  • rendering: no impact, except labels
  • overlays et theme: ok
• the implementation is tested in the examples repository, see [POC] Test the alternative terminal point implementation for 'no orthogonal segments' bpmn-visualization-examples#306

Suggestion: if implementing the changes proposed here, we should recreate most of the screenshots and re-configure thresholds. This also applies for any implementation removing the orthogonal segments. Orthogonal segments slightly moved the edges which is no longer the case.

Initial implementation is not supporting collapsed elements

There is an impact of the implementation on the example collapsing/expanding the pools

• issue with b042a98: see [POC] Test the alternative terminal point implementation for 'no orthogonal segments' bpmn-visualization-examples#306 (comment)
• reproduced here with specific diagram by collapsing sub-processes/pools in dedicated visual tests

Notice that is not an officially supported feature, it is just used in example. So we could decide to skip fixing the problem.

BPMN diagram of a non regression visual test to review

The diagram with.outside.flows used in fit test has non orthogonal segments and a terminal waypoints inside a shape (confirmed with the diagram xml content and display with bpmn-js).
I don't know if we wan to keep this: it may be interesting if we want to test the effect of the zoom/fit on segments and terminal waypoints computation. In this case, we may also add a terminal waypoint outside a shape.
If we keep it, snapshot changes will be required if we manage to fix such waypoints.

[github-actions]

🎊 PR Preview b042a98 has been successfully built and deployed to https://process-analytics-bpmn-visualization-js-demo_preview-pr-1863.surge.sh

🕐 Build time: 53.379s

_{🤖 By surge-preview}

[github-actions]

🎊 PR Preview b042a98 has been successfully built and deployed to https://process-analytics-bpmn-visualization-js-doc_preview-pr-1863.surge.sh

🕐 Build time: 46.272s

_{🤖 By surge-preview}

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

90.6% Coverage
0.0% Duplication

[tbouffard]

Decisions taken with @csouchet based on commit 991ab47

• create a PR about the visual tests for collapsed elements: for integration on master to ensure non regression [TEST] Add visual tests for edges targeting collapsed elements #1867
• start implementing the 'no orthogonal segment' with a new PR as it will take time to update snapshots and thresholds for visual tests and update the with.outside.flows diagram to also check outside terminal waypoints [FIX] Do not force usage of orthogonal edge segments #1868
• in the meantime keep this PR open to fix the issue with collapsed elements (this is not mandatory, as this is not a supported feature - see PR description). This is not possible for now, see [POC] Alternative terminal point implementation for 'no orthogonal segments' #1863 (comment). So create an issue ([FEAT] Restore the experimental pools/subprocess live collapsing #1871) and remove the availability of collapsing pools in the examples ([FEAT] Disable pool collapsing bpmn-visualization-examples#307).
• create an issue to restore the feature that put the inside and outside terminal waypoints on the perimeter of the shape. We should do it in a more clever way than the mxGraph implementation to avoid the side effects on the arrow direction. See [FEAT] Ensure that the terminal waypoints are on the shape perimeter #1870

[tbouffard]

Final status of "Initial implementation is not supporting collapsed elements"

This is not something we can fix right away. Just removing the Segment Connector without the alternative implementation proposed in this PR shows that the waypoints inside the collapsed pools and sub-processes are still there. We would like to detect such waypoints from the list and remove them. Remember that the alternative implementation proposed by this PR is just about the terminal waypoints computation.
Everything acts as if the Segment Connector implementation is cleaver enough to remove waypoints inside the collapsed elements. To fix the issue, we need to do the same which is out of the scope of the POC.

Screenshots of the collapsed pools used in visual tests when using the new implementation (without Segment connector)

collapsing pools on the left of the diagram	collapsing pools on the right of the diagram

[tbouffard]

Issues created, so closing.