-
-
Notifications
You must be signed in to change notification settings - Fork 385
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
Support for deleting points to break path #1593
Support for deleting points to break path #1593
Conversation
I'm very glad to see this, thank you! I'll review it in detail ASAP, although I'm behind on my reviews because I've been focusing on some things with short-term deadlines. Most likely this weekend though I'll be able to do do the remaining reviews. Thanks for your patience and continued work on these while you're waiting on them to get merged, I promise the PRs won't get abandoned! |
!build |
thanks, take your time. I'm still diving into the codebase and contributing some codes during the process 😆 |
|
Just some quick comments about the behavior: Eventually we'll have a menu, similar to Blender's X hotkey menu, that will let the user choose how to delete or dissolve the selection. But for now we'll rely on hotkeys. What I had in mind was that CtrlDelete would delete the selected point but also the adjacent segments. However the behavior you implemented, where it breaks it but keeps the adjacent segments, is still useful and I think assigning it to CtrlShiftDelete would be the best, until we eventually get around to building the X hotkey menu (which will require some frontend work and refactoring from me to build context menus). Also, when breaking these points, it'd be nice to have their other handle be removed (equivalent to selecting it and hitting Delete). So instead of this being what you see after breaking the point and separating them a bit: I'd like to see this instead: Lastly, it would be useful if it was also possible to break/delete an anchor that's part of an already open shape. Doing this would result in two separate subpaths. Hopefully those suggestions would be easy enough to tweak. This is all looking good and is very useful, so thank you for doing it! |
thanks for the detailed comments. I will try to change the hotkeys and delete handles first. For delete an anchor for open shape, I also thought about it. After deleting the anchor on an open curve, would we expect two break parts to be in one layer or two separate layers? I have tried in Inscape, they will create a new layer to draw the new break part. |
For the moment I haven't made up my mind which is likely to be better. The idea of subpaths is that a layer can contain multiple of them. They're used, for example, to create a donut shape where there's an outer circle as one subpath and an inner circle as the other subpath (and since they have opposite winding directions, the inner one is drawn as a hole). It's probably easier for you to make them subpaths so I'd suggest you go with that option. This also would make it possible for people to actually create subpaths which isn't possible right now through any other means. But it splitting into two layers would also be acceptable. We may also want to eventually add a menu action that can split subpaths into individual layers. Or perhaps another to combine multiple selected layers into subpaths of one layer. |
Please ping me when you've had the chance to complete each of the suggestions I made and I'll be ready to jump back into reviewing and merging this. Thanks! |
I had a initial try to handle breaking an open path yesterday, I chose to break them as subpaths in a layer. But met a weired issue that both subpaths are always moved together. I will spend more time today to see how to solve this and do some tests. Will let you know once done. |
Could you post a video of how both subpaths were being moved together? |
Screen.Recording.2024-02-04.at.09.44.14.mov |
Ah, yes, gotcha. Pay close attention to which anchor points are selected (when they're filled with blue), we have it so when you click on a shape with the Path tool, it becomes fully selected by default so it's easily draggable. But now that we are actually using multiple subpaths within a layer, we would like to update that behavior so it only selects all the anchors of the subpath you click on, not all anchors from all subpaths. However, if you prefer to work on that in a separate PR, that could be deferred (the choice is yours). |
There is one more bullet point to add to your list, by the way, if you don't mind implementing it too:
Thanks :) |
Oh, you give good hints on the issue. I could spend more time on this PR. And add a new to-do in the list 😄 .
|
@Keavon do we have an existing function to check if the mouse click on a subpath? |
I'll have to ask @0HyperCube if he knows, I'm not sure. |
@Keavon Break path related to-dos should have been finished, you could have a code review when you have time. Subpath selection issue I could open another PR to fix, let this PR stays on path break related changes. |
2e6b0c0
to
45dec05
Compare
!build |
|
1046cf3
to
ce131b2
Compare
This is a case has been broken after I refactor some code. Have been fixed. |
Closes #1550
Only handle the closed path case and support only delete one anchor right now.
Select a anchor, then Ctrl + Delete to break