Add preliminary support for Cubic Bézier curve edge #245

kaisalmen · 2021-10-04T10:04:58Z

This PR introduces preliminary support for cubic Bézier curve edges.

What has been achieved:

Support one or many Bezier curves
Allow interactive manipulation of curves
Interactively add and remove new curves on an edge
Smooth interpolation on curve junctions

Issues that should be fixed before PR is ready

Add +/- to green/red circles for add/remove curve
New Curve should be created on "click" and not "move" of add/remove
Visual connections (lines) between control points and curve endings

What is still missing (reason for being preliminary). To be discussed if done in scope of this PR or later:

Add unit tests
Support Text-Layout
Line jumps according curve

The classdiagram has been enhanced to show the new capabilities.

gitpod-io · 2021-10-04T10:05:00Z

spoenemann

Thanks for this huge contribution!

The approach of using green and red handles for adding / deleting sections is interesting and probably a good match for handling Bezier edge sections. I'd propose to add + / - icons to those handles. However, a single click on those handles has no effect, but you need to move the mouse before anything happens; this is probably unintuitive.

Interesting: Bezier control points are handled as regular line segments for the computation of line jumps, see below. This is not critical, but should be noted in a separate issue once we merge this.

Keep up the good work!

spoenemann · 2021-10-11T07:27:38Z

src/features/routing/bezier-anchors.ts

@@ -0,0 +1,44 @@
+/********************************************************************************
+ * Copyright (c) 2019 TypeFox and others.


Please use an up-to-date-year for new files.

Yes, I will. Thanks.

kaisalmen · 2021-10-18T07:48:59Z

The approach of using green and red handles for adding / deleting sections is interesting and probably a good match for handling Bezier edge sections. I'd propose to add + / - icons to those handles. However, a single click on those handles has no effect, but you need to move the mouse before anything happens; this is probably unintuitive.

+/- icons, yes definitely. Colors-only was a short-cut.
Handle control is identical to the others edges, but this is not good here.
At least these two things should be corrected before this PR is ready, I think. I will add WIP to the title and add the list these things to the open points in the description.

Interesting: Bezier control points are handled as regular line segments for the computation of line jumps, see below. This is not critical, but should be noted in a separate issue once we merge this.

Fixing this behaviour is likely not easy (needs investigation). Actually the curve should define the line jumps and not the handles.

spoenemann

A few things left to fix, otherwise it's looking great!

Clicking on + / - still doesn't work unless I move the mouse a bit. We should listen on the mouseUp event, not mouseMove.
When moving a split point between two Bezier segments, I'd expect the connected control points to move by the same amount.
The source and target anchors seem to be determined by the position of the other node instead of the nearest control point. For example, moving around the "Ada" node affects the anchor position on the Foo node.

spoenemann · 2021-10-26T06:06:41Z

src/features/routing/bezier-edge-router.ts

+import { LinearEdgeRouter } from './linear-edge-router';
+
+@injectable()
+export abstract class BezierEdgeRouter extends LinearEdgeRouter {


It looks strange to inherit from LinearEdgeRouter. Shall we rename that to "AbstractEdgeRouter", or rather copy some code to avoid the inheritance?

Renaming it sounds better. I need to check how much code is really required from the inherited class. I will come back with a proposal.

I go for rename, otherwise we have to copy too much code.

spoenemann · 2021-10-26T06:09:40Z

src/features/routing/bezier-edge-router.ts

+            x: relH2.x + BezierEdgeRouter.DEFAULT_BEZIER_HANDLE_OFFSET,
+            y: relH2.y
+        };
+        return { h1: h1, h2: h2 };


Hint: you can write this as { h1, h2 }. But in this case I'd propose to return a tuple instead: [h1, h2] of type [Point, Point].

Ok, will do so.

Only one segment for now Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

Allow to add but not yet remove additional new curve segment Classpath example now features to curves Control handle movement is mirrored on control handle opposite to bezier-junction Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

Add and remove is done via new red and green handles available at source or junction. If only one curve is available, then there is no delete Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

Add separate handle view for add/remove of bezier curves Add SBezierControlHandleView for handle visualization Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

kaisalmen · 2021-10-28T13:03:54Z

Only thing left now is the +/- button behaviour. I fixed some issue with the mirrored control point alignment (when model is initially loaded) as well.

- Renamed LinearEdgeRouter to AbstractEdgeRouter - Bezier control points move along with source, target and bezier-junctions - Fixed eslint issues - Fixed source and target anchors when moving control points and attached objects Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

- Add MouseListener that reacts on mouseDown on bezier-add and bezier-remove elements. It triggers an Action and Command that adds or removes a curve segment - Adjust class diagram layout Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

kaisalmen · 2021-11-02T21:40:27Z

Ready for review. The last open point is now resolved: Add MouseListener that reacts on mouseDown on bezier-add and bezier-remove handles. The event riggers a new Action and Command that adds or removes a curve segment.

spoenemann

Thanks! I have a few more remarks, but I'll merge it anyway so we can start the work on #243. @kaisalmen please create a new PR to address the remaining comments when the restructuring is done (will submit a PR for that today).

spoenemann · 2021-11-03T06:51:41Z

src/features/routing/bezier-edge-router.ts

+
+        const result = [];
+        let router;
+        const element = target.root.index.getById(target.id);


I think this will just point to the same element, so target and element are the same?

True. I will simplify it.

spoenemann · 2021-11-03T06:55:00Z

src/features/routing/bezier-edge-router.ts

+    edgeId: string;
+    actionTask: BezierActionTask;
+
+    constructor(@inject(TYPES.IEdgeRouter) protected readonly router: BezierEdgeRouter,


Actions should be serializable, so please remove the router and fetch it in the BezierCurveCommand instead.

Ok, the action will only contain the targetId and a value from the enum in the future.

spoenemann · 2021-11-03T06:56:13Z

src/features/routing/bezier-edge-router.ts

+        this.edgeId = edgeId;
+    }
+
+    getRouter(): BezierEdgeRouter {


We don't need these getters because the properties are public.

✅ It was a bit late yesterday evening.

spoenemann · 2021-11-03T06:57:23Z

src/features/routing/bezier-edge-router.ts

+};
+
+@injectable()
+export class BezierCurveCommand extends Command {


I think the command name should be more specific, like AddRemoveBezierSegmentCommand.

Yes, good idea. I will rename the action accordingly

spoenemann reviewed Oct 11, 2021

View reviewed changes

kaisalmen changed the title ~~Add preliminary support for Cubic Bézier curve edge~~ WIP: Add preliminary support for Cubic Bézier curve edge Oct 18, 2021

kaisalmen force-pushed the ks-bezier-edge branch from 5b78de3 to c95cea4 Compare October 18, 2021 12:31

spoenemann mentioned this pull request Oct 21, 2021

Consider refactoring actions to interface`s instead of classes #233

Closed

tortmayr mentioned this pull request Oct 25, 2021

Node.js support and restructuring to monorepo #243

Closed

spoenemann reviewed Oct 26, 2021

View reviewed changes

kaisalmen added 4 commits October 27, 2021 22:17

Initial version of cubic bezier edges

7c5ff10

Only one segment for now Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

Support multiple splines

c9cbf32

Allow to add but not yet remove additional new curve segment Classpath example now features to curves Control handle movement is mirrored on control handle opposite to bezier-junction Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

Added support for removal of curves

6de4a27

Add and remove is done via new red and green handles available at source or junction. If only one curve is available, then there is no delete Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

Improve bezier curve handling

de37871

Add separate handle view for add/remove of bezier curves Add SBezierControlHandleView for handle visualization Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

kaisalmen force-pushed the ks-bezier-edge branch 2 times, most recently from 509694d to e97073f Compare October 28, 2021 13:02

kaisalmen force-pushed the ks-bezier-edge branch from e97073f to a56cd88 Compare October 28, 2021 13:11

BezierEdgeRouter directly reacts on add/remove

c15a155

- Add MouseListener that reacts on mouseDown on bezier-add and bezier-remove elements. It triggers an Action and Command that adds or removes a curve segment - Adjust class diagram layout Signed-off-by: Kai Salmen <kai.salmen@typefox.io>

kaisalmen changed the title ~~WIP: Add preliminary support for Cubic Bézier curve edge~~ Add preliminary support for Cubic Bézier curve edge Nov 2, 2021

spoenemann approved these changes Nov 3, 2021

View reviewed changes

spoenemann merged commit 5b094df into eclipse-sprotty:master Nov 3, 2021

kaisalmen deleted the ks-bezier-edge branch November 3, 2021 07:36

spoenemann added this to the v0.11.0 milestone Nov 30, 2021

spoenemann mentioned this pull request Apr 27, 2022

Line jump calculation does not work correctly for bezier edges #287

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preliminary support for Cubic Bézier curve edge #245

Add preliminary support for Cubic Bézier curve edge #245

kaisalmen commented Oct 4, 2021 •

edited

Loading

gitpod-io bot commented Oct 4, 2021

spoenemann left a comment

spoenemann Oct 11, 2021

kaisalmen Oct 18, 2021

kaisalmen commented Oct 18, 2021 •

edited

Loading

spoenemann left a comment

spoenemann Oct 26, 2021

kaisalmen Oct 26, 2021 •

edited

Loading

kaisalmen Oct 27, 2021

spoenemann Oct 26, 2021

kaisalmen Oct 26, 2021

kaisalmen commented Oct 28, 2021 •

edited

Loading

kaisalmen commented Nov 2, 2021

spoenemann left a comment

spoenemann Nov 3, 2021

kaisalmen Nov 3, 2021

spoenemann Nov 3, 2021

kaisalmen Nov 3, 2021

spoenemann Nov 3, 2021

kaisalmen Nov 3, 2021

spoenemann Nov 3, 2021

kaisalmen Nov 3, 2021

		@@ -0,0 +1,44 @@
		/********************************************************************************
		* Copyright (c) 2019 TypeFox and others.

Add preliminary support for Cubic Bézier curve edge #245

Add preliminary support for Cubic Bézier curve edge #245

Conversation

kaisalmen commented Oct 4, 2021 • edited Loading

gitpod-io bot commented Oct 4, 2021

spoenemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaisalmen commented Oct 18, 2021 • edited Loading

spoenemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaisalmen Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaisalmen commented Oct 28, 2021 • edited Loading

kaisalmen commented Nov 2, 2021

spoenemann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaisalmen commented Oct 4, 2021 •

edited

Loading

kaisalmen commented Oct 18, 2021 •

edited

Loading

kaisalmen Oct 26, 2021 •

edited

Loading

kaisalmen commented Oct 28, 2021 •

edited

Loading