Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Bézier handles to path masks, so the control points can be moved independently #17204

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marc-fouquet
Copy link

Patch for issue #14236 .
Replaces my previous PR #17044 .

Includes changes to the way the feathering border is drawn for paths. The border could already be bugged in rare cases before. With the added freedom of moving the control points individually, broken feathering borders were much too easy to produce.

Includes one changed UI tooltip in English, German, French, Italian.

…d independently

Includes changes to the way the feathering border is drawn for paths. The border could
already be bugged in rare cases before. With the added freedom of moving the control
points individually, broken feathering borders were much too easy to produce.

Includes one changed UI tooltip in English, German, French, Italian.
@victoryforce
Copy link
Collaborator

Includes one changed UI tooltip in English, German, French, Italian.

No, you shouldn't touch what is related to translations. POT file is updated separately, contributors do not have to do this in PR.

@ralfbrown ralfbrown added feature: enhancement current features to improve scope: UI user interface and interactions labels Jul 26, 2024
@marc-fouquet
Copy link
Author

OK, I removed my localization changes. Do I have to submit them somewhere else separately?

@TurboGit
Copy link
Member

@marc-fouquet : Yes, you can simply make another (or better multiple PR to eventually discuss the translation with people speaking the target language without holding the other PR for being merged) for the translation work. TIA.

@marc-fouquet
Copy link
Author

I have submitted one extra PR for this (mainly because of the .pot-file, which is the prerequisite for the localization change, so I was not sure what to do with it in PRs that are separated by language).

@TurboGit
Copy link
Member

TurboGit commented Jul 30, 2024

Two things, when using shift-click I can indeed change point independently. But if edited after without the shift modifier the lines are realigned. I would have expected them to keep the actual angle and moved together. I would only reset to standard aligned controlled point when change mode with a ctrl-click on the node.

Also I see a small glitch on the feather which gets a little "corner" as soon as the lines are not aligned. See the feather red line over the clouds:

image

@marc-fouquet
Copy link
Author

The deadline for DT 5.0 is December, right? I will look into this some more and try to find improvements, but I can't promise that I will be fast.

Regarding #1: I initially wanted to implement something that I think would have felt even more natural to me - basically the current behaviour, BUT with the added possibility to press or release the shift-key on the fly while already dragging the control point. I did not implement this, because in _path_events_mouse_moved I am missing the state variable which contains the state of the keyboard.

So far I wanted to keep my changes minimal. I did not yet research, where _path_events_mouse_moved is called from, but a change to add the extra information would of course require changing the function definition in all other shapes. What do you think?

Regarding #2: This is another behaviour that was present before, but gets amplified after my change. I will have a look if I can improve this.

One more thing that I don't like, but which was also present before: With weirder shapes, the feathering control points are not located on the feathering border and also not visually connected to their corresponding point on the regular border. Maybe I could just draw a line to connect the border point with the feather point, now that the perpendicular "Bézier handle" is gone.

Another thing that I thought about was refactoring the code. It is unnecessarily hard to understand, with unclear variable names, a lack of comments, functions that are 100s of lines of code, no unit tests. Besides time, the main problem here is that I still don't fully understand everything, so I am afraid of breaking things.

@TurboGit
Copy link
Member

The deadline for DT 5.0 is December, right?

Yes, no urgency.

Sorry, I just edited my first paragraph. I meant when edited after WITHOUT the shift modifier.

@TurboGit TurboGit added the wip pull request in making, tests and feedback needed label Jul 31, 2024
@marc-fouquet
Copy link
Author

I have worked on this some more. My current code is in need of some cleanup before it can be merged, but I won't have time in the next 2 weeks, so there will be a further delay.
The current state is here: https://github.com/marc-fouquet/darktable/tree/path_bezier_new_debug_2024-08-02

@TurboGit
Copy link
Member

Hi Marc, thanks for your work I check your branch and report.

@marc-fouquet
Copy link
Author

The current controls:

  • Shift drags handles independently
  • Ctrl enforces symmetry
  • Ctrl + Shift enforces both points to be on a straight line, but the distance can be different (not mentioned in tooltips)
  • No modifier preserves the angle and the ratio of distances

I have added an optimization step to make the connections between segments smoother. However, this can't be used on all connections.

I spent the most effort on ensuring that the feathering border behaves well, as long as the user acts somewhat reasonably. However, users have a lot of freedom to shoot themselves in the foot.

Especially large feathering borders, large changes in the feathering borders, and overlapping curves are cases that may break the algorithm. In the worst case, it is possible to build a curve where some feathering control points are no longer reachable with the mouse, so the user might have to delete the shape and start over. However, this was also possible before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: enhancement current features to improve scope: UI user interface and interactions wip pull request in making, tests and feedback needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants