[Merged] Split Edge function for Sketcher

[Kunda1]

pinged by pinger macro

@abdullah care to weigh in on this new feature?
FYI, PR is https://github.com/FreeCAD/FreeCAD/pull/4420

[abdullah]

pinged by pinger macro

@abdullah care to weigh in on this new feature?
FYI, PR is https://github.com/FreeCAD/FreeCAD/pull/4420

Yes. Believe it or not, I am wanting to look at this PR for over one month (maybe even two months). There is always something getting in the middle. Hopefully this weekend I will be able to. Thanks for reminding me

[Kunda1]

Sorry for being pushy. Totally understand that you're busy. Thanks for all you availability!

[abdullah]

Sorry for being pushy. Totally understand that you're busy. Thanks for all you availability!

No problem. You are not being pushy . I am on it now.

[Kunda1]

git commit 4d6b1f3
Thanks to everyone involved!

Can we get a wiki documentation page as well ?

[abdullah]

For this feature I have created a pull request https://github.com/FreeCAD/FreeCAD/pull/4420. Please feel free to test it, comment or report any bugs.

First at all, my deepest apology for the unacceptably long delay in merging this PR. Thanks for your patience.

After the PR merge, I have pushed a second commit with some changes. Most of it is just about renaming to make it easier to understand for others what the functions do. In one instance (swapInvolvedGeometry), I have moved the constraint specific functionality to the Constraint class. This way it is accessible to any client code.

I have not touched the split() function. I think it is possible to implement this function in more generic terms to avoid to have to write geometry specific code for every single geometry (the current trim() functionality may show the way). I am not a fan of the "do while(false)" idiom in c++, I think it is misleading from a code readability point of view (but I know that many others are of a different opinion, specially those coming from a C or Fortran background). In this case it is used to avoid "if" evaluations of the group and break from the loop at any time (through continue). However, addGeometry in practice never actually returns a negative value. The changes of properties could use move syntax instead of having to delete (this is also how trim works). As the code works and I do not have time at this moment, I decided not to touch the internals of the split function, which is something that can be done in the future if judged necessary or appropriate.

There is an issue I detected in my tests, which should be fixed. If an arc having a radius constraint is split, then the radius constraint is copied, while at the same time making the center point coincident and one endpoint. This leads to redundant constraints. My expectation is that this happens for diameter too. Nothing that cannot be solved by a direct deletion (or even by doing nothing at all if the auto-remove redundants mode is enabled).

Overall I think it works nicely and it will be a time saver in some common situations. Thank you !!

[Kunda1]

git commit 4d6b1f3
Thanks to everyone involved!

Can we get a wiki documentation page as well ?

I started a placeholder page at Sketcher Split. Can someone fill in the rest?
Also where should Split be in the hierarchy of Sketcher Workbench?

[tpavlicek]

Hello,

After the PR merge, I have pushed a second commit with some changes. Most of it is just about renaming to make it easier to understand for others what the functions do. In one instance (swapInvolvedGeometry), I have moved the constraint specific functionality to the Constraint class. This way it is accessible to any client code.

I have not touched the split() function. I think it is possible to implement this function in more generic terms to avoid to have to write geometry specific code for every single geometry (the current trim() functionality may show the way). I am not a fan of the "do while(false)" idiom in c++, I think it is misleading from a code readability point of view (but I know that many others are of a different opinion, specially those coming from a C or Fortran background). In this case it is used to avoid "if" evaluations of the group and break from the loop at any time (through continue). However, addGeometry in practice never actually returns a negative value. The changes of properties could use move syntax instead of having to delete (this is also how trim works). As the code works and I do not have time at this moment, I decided not to touch the internals of the split function, which is something that can be done in the future if judged necessary or appropriate.

There is an issue I detected in my tests, which should be fixed. If an arc having a radius constraint is split, then the radius constraint is copied, while at the same time making the center point coincident and one endpoint. This leads to redundant constraints. My expectation is that this happens for diameter too. Nothing that cannot be solved by a direct deletion (or even by doing nothing at all if the auto-remove redundants mode is enabled).

Abdullah, thank You for the review a valuable comments. Definitely I have no objections against the changes You have made, I am pretty much sure they are heading in the right way.

As for the comments, I was thinking about adding split() functionality also to the rest of conics (ellipse, parabola, hyperbola), so once I get to it, I will look how trim() is now implemented and refactor the code according to Your suggestions.

Once again thanks, kind regards,


Tomas

[abdullah]

Abdullah, thank You for the review a valuable comments. Definitely I have no objections against the changes You have made, I am pretty much sure they are heading in the right way.

As for the comments, I was thinking about adding split() functionality also to the rest of conics (ellipse, parabola, hyperbola), so once I get to it, I will look how trim() is now implemented and refactor the code according to Your suggestions.

Once again thanks, kind regards,

Tomas

Thanks!

It is really so nice to hear you are willing to extend the functionality and take a look at trim. I will take a look at your PR when it is ready. Hopefully it will not take that long for me to review this time

[Kunda1]

git commit 4d6b1f3
Thanks to everyone involved!

Can we get a wiki documentation page as well ?

I started a placeholder page at Sketcher Split. Can someone fill in the rest?
Also where should Split be in the hierarchy of Sketcher Workbench?

I've added mention this to the 0.20 release page but the documentation is still incomplete. I know this feature is still a work in progress nevertheless we should have some documentation explaining how to use it. Someone please step up, thank you!