Bezier curves, please

[andrewmackie]

Drawing and editing masks with complex curves is incredibly difficult in Darktable. This is because smooth nodes are symmetrical and, therefore, attempting to change a curve between two nodes will also alter the curves outside of those two nodes. Repairing this side effect creates more side effects, and endless iterations for the user.

A solution to this already exists - Bezier curves as implemented by Inkscape. These allow the vectors on either side of a node to be fully symmetrical (in line which other with the same amplitude), partly symmetrical (in line with each other with independent amplitude) or fully independent of each other (independent angle and amplitude). Complex curves can, therefore, be created easily, with no side effects to repair. I have spent hours attempting to create precise curves in Darktable which would have taken seconds in Inkscape.

Please implement bezier curves. It is likely that they can be adapted from Inkscape's code (at https://gitlab.com/inkscape/inkscape) although I am unable to verify this and help with the code, I'm sorry, as I do not know C.

[TurboGit]

A solution to this already exists - Bezier curves as implemented by Inkscape.

Note that darktable curves are Bezier curves just that only a single controlled point is exposed and handled symmetrically.

[TurboGit]

BTW, this could be a good first contribution. If you un-comment the some code in masks/path.c you can see the real control points.

diff --git a/src/develop/masks/path.c b/src/develop/masks/path.c
index b93e356d24..247d257bd6 100644
--- a/src/develop/masks/path.c
+++ b/src/develop/masks/path.c
@@ -2269,14 +2269,14 @@ static void _path_events_post_expose(cairo_t *cr,
   {
     const int k = gui->point_edited;
     // uncomment this part if you want to see "real" control points
-    /*
+
     cairo_move_to(cr, gpt->points[k*6+2], gpt->points[k*6+3]);
     cairo_line_to(cr, gpt->points[k*6], gpt->points[k*6+1]);
     cairo_stroke(cr);
     cairo_move_to(cr, gpt->points[k*6+2], gpt->points[k*6+3]);
     cairo_line_to(cr, gpt->points[k*6+4], gpt->points[k*6+5]);
     cairo_stroke(cr);
-    */
+

So the work to do:

• Discuss how to enable those enhanced control points
• Add control handle for those two points
• Ensure that those points can be dragged
• Discuss what to do when one change from the advanced editing mode to the simple current mode

I'm pretty sure this should not be too difficult, hence the tag as "good first contribution". Volunteer?

[McDonnellJoseph]

Hello, if no one is already on it I'd be happy to have a go

[TurboGit]

Hello, if no one is already on it I'd be happy to have a go

Go ahead, will be a nice addition. Thanks.

[TurboGit]

@McDonnellJoseph : Just a note that the feature freeze is mid-may. If you want to have this included into 4.4 it should be ready before. Just let me know and I'll add 4.4 milestone otherwise we'll re-discuss this during the next release cycle.

[McDonnellJoseph]

Hello I'm on it but I doubt I will be done before the freeze

[TurboGit]

@McDonnellJoseph : Thanks for the report, there is no urgency. We'll schedule that for 4.6 then. Thanks a lot !

[McDonnellJoseph]

Hello, I'm getting back to this issue, being new with C I think this might be a bit too much to chew for me. Do you think you could baby step a bit more this issue for me ? Thank you !

[TurboGit]

@McDonnellJoseph : Sorry for the delay, I've been working on the release.

Small steps? It really depends on your way of working... I would try:

1. Display the Bezier controls (see my previous message)
2. Change the code to be able to have the mouse over detect the two nodes
3. See where the nodes coordinates are stored
4. Look at the mouse_move events, (_path_events_mouse_moved in path.c) and see how it handles the different nodes ...

Hope this will help you. TIA.

[TurboGit]

And of course how we initiate a point dragging in _path_events_button_pressed.

[TurboGit]

@McDonnellJoseph : Have you started on this? Any more questions? Do you think you can have this for 4.6 at end of year? Do not hesitate to ask questions, if we can answer of course :) You can also make small progress and propose PR for review and questions.

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

[TurboGit]

@McDonnellJoseph : Any news? This issue will close itself if there is no more action on this area.

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

[superusercode]

still needed

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

[marc-fouquet]

I am not a "C"-Person, but I found this issue and have looked into this a little bit. It seems to be mostly a UI issue... except for when it isn't.

The curve is a Bézier spline. As @TurboGit pointed out, the Bézier control points can be shown by uncommenting lines in the code. It is relatively easy to remove the perpendicular control point and actually control the curve by those Bézier points. That far I can provide a patch if needed, though I don't know if I have broken compatibility with old masks, since I have changed a struct in masks.h .

This control is more intuitive anyway, as most people know Bézier curves from other software (Inkscape, Photoshop, GIMP,...), while no one expects a handle that is perpendicular to the curve and also easily confused with the feathering radius.

The data structure stores an anchor point of the curve together with a pair of control points. Currently there are two cases:

(1) The control points are symmetric before and behind the anchor point. In this case the curve is continuous. (2) All three points are collapsed to a single point, which is a corner.

The author of this issue wants more freedom and basically it is possible to move the control points to places that don't satisfy (1) or (2). However this breaks the calculation of the outer curve for feathering. This calculation assumes that the inner curve is differentiable for case 1 (which I think requires the controls to be symmetric). In case 2 it draws a circle arc with the anchor point in the center.

So there is a math problem left to solve.

[TurboGit]

Thanks for looking at this, I would say start a PR to keep the ball rolling :) We will see if someone want to have a deeper look at the remaining issues.

[marc-fouquet]

I have looked some more into the Bézier issues. The patch that I submitted works, but sometimes there is a visual glitch regarding the border of the feathering area. This border is drawn in a circle around a point, see the screenshot. The effective mask is OK, this is a UI issue.

In fact this screenshot was taken from an unmodified darktable 4.8.0, so this glitch was there before (though it is hard to reproduce). Currently I don't think that my Bézier patch breaks anything that wasn't broken. But with the new handles, concave corners are a much more common occurrence, so this glitch appears more often.

As far as I understand the code, rendering of the borders works like this:

0. _path_get_pts_border is the main function that controls the process.
1. For each path section _path_points_recurs is called, to draw the Bézier segment and its feathering border.
2. If the end points of two segments of the feathering border don't match, _path_points_recurs_border_gaps is called. This function draws the arcs. It happens whenever we don't have a completely smooth Bézier curve with symmetric control points. This means it does happen for all sharp corners, including the ones that were possible previously. It does not matter if the corners are convex (the useful case) or concave (a less useful case).
3. The function _path_find_self_intersection cuts all self-intersecting segments, so this is the place that normally eliminates unnecessary arcs. For this it creates a bitmap-like data structure and fills it with the curve points, to detect if a "pixel" is visited twice.

Step 3 sometimes fails and therefore some arcs are not removed.

I think that this block of code

              if((v[k] - i)
                 * ((int)dt_masks_dynbuf_get(inter, -2)
                    - (int)dt_masks_dynbuf_get(inter, -1)) > 0
                 && (int)dt_masks_dynbuf_get(inter, -2) >= v[k]
                 && (int)dt_masks_dynbuf_get(inter, -1) <= i)
              {
                // we find an self-intersection portion which include the last one
                // we just update it
                dt_masks_dynbuf_set(inter, -2, v[k]);
                dt_masks_dynbuf_set(inter, -1, i);
              }
              else
              {
                // we find a new self-intersection portion
                dt_masks_dynbuf_add_2(inter, v[k], i);
                inter_count++;
              }

does not merge intersections correctly. Therefore instead of one continuous intersection, many (i.e. 20) overlapping ones are added to inter.

When inter gets too large, the line

if(inter_count >= nb_corners * 4) break;

causes the function to exit early, before all self-intersections are removed. Increasing the constant here (i.e. from 4 to 16) fixes the UI issue, but does not solve the underlying problem.

I still don't fully understand the code, i.e. the reason for the expression (v[k] - i) * ((int)dt_masks_dynbuf_get(inter, -2) - (int)dt_masks_dynbuf_get(inter, -1)) > 0. This seems to make sure, that both "go in the same direction", but since the order in which we loop over the points is fixed, I don't see why this check should be necessary.

[marc-fouquet]

I have submitted a new PR that should work now.

[github-actions[bot]]

This issue has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

[superusercode]

PR is waiting for a response