Sketcher: Snap initial implementation.

[PaddleStroke]

• This PR creates a SnapManager which centralize the snapping features.
• Grid snap is moved to this SnapManager.
• Snap to object is added. A preference setting is added to toggle it.
• Snap at angle is added. A preference setting let user customize the angle. Snap at angle for arc and line only now.

The priority order is : snap at angle, then snap to object, then snap to grid. This is the order that makes sense in term of UX.

• Grid snap is activated by a toggle as before it didn't change.
• Snap to object is on by default and can be disabled in preferences, though it's not likely that users will need to disable it. This setting should perhaps be paired with auto-constraint?
• Snap at angle is activated by holding CTRL. The use of CTRL is software standard to snap movement along an axis/angle. It does not interfer with other uses of CTRL (ie camera panning/rotation, rubberband selection.)

Future improvement ideas :
1 - Enable snapping to object when dragging a point.
2 - Snap to inifite lines rather than line segment.
3 - Snap to bisecting angles between 2 lines.

https://forum.freecad.org/viewtopic.php?t=75856

Thank you for creating a pull request to contribute to FreeCAD! Place an "X" in between the brackets below to "check off" to confirm that you have satisfied the requirement, or ask for help in the FreeCAD forum if there is something you don't understand.

• Your Pull Request meets the requirements outlined in section 5 of CONTRIBUTING.md for a Valid PR

Please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 1.0 Changelog Forum Thread.

---

[freecadci]

for feature branch PR_8387. Pipeline 770384056 was triggered at 687cd34. All CI branches and pipelines.

[freecadci]

for feature branch PR_8387. Pipeline 770854641 was triggered at 33917ea. All CI branches and pipelines.

[freecadci]

for feature branch PR_8387. Pipeline 771022927 was triggered at 9a36a6e. All CI branches and pipelines.

[freecadci]

for feature branch PR_8387. Pipeline 771082108 was triggered at 7808136. All CI branches and pipelines.

[freecadci]

for feature branch PR_8387. Pipeline 771104399 was triggered at a6a990f. All CI branches and pipelines.

[freecadci]

for feature branch PR_8387. Pipeline 772640919 was triggered at e3a8f9b. All CI branches and pipelines.

[freecadci]

for feature branch PR_8387. Pipeline 772894226 was triggered at 3ff2b9b. All CI branches and pipelines.

[freecadci]

for feature branch PR_8387. Pipeline 772922230 was triggered at 8d91249. All CI branches and pipelines.

[freecadci]

for feature branch PR_8387. Pipeline 773634109 was triggered at 59cbb87. All CI branches and pipelines.

[freecadci]

for feature branch PR_8387. Pipeline 773688289 was triggered at 7497eaa. All CI branches and pipelines.

[freecadci]

for feature branch PR_8387. Pipeline 773727186 was triggered at 5b6f4f8. All CI branches and pipelines.

[abdullahtahiriyo]

I took a initial fast look to the code. I have not compiled yet. I did not go into the detail of the code.

However, it looks reasonably good.

There is one aspect that could be improved. The code is checking the parameter preferences on every function execution (hgrp + getint). This is unnecessary, as a parameter observer can update local variables used in the functions when they change. This leads to faster and more readable code.

You have similar code here:

FreeCAD/src/Mod/Sketcher/Gui/EditModeCoinManager.h

Line 106 in b6af6c9

class ParameterObserver : public ParameterGrp::ObserverType

FreeCAD/src/Mod/Sketcher/Gui/EditModeCoinManager.cpp

Line 76 in b6af6c9

void EditModeCoinManager::ParameterObserver::initParameters()

Basically, this class does the following:

1. Initialise the variables to the parameter value upon construction.
2. Subscribe to parameter changes (and unsubscribe upon object destruction).
3. Modify local variables used in the actual algorithm when the parameters change.

A nested class can access private members of the outer class. So if you make the observer as a nested class, you can directly change local private variables of the outer class (the SnapManager).

Do you feel like making this improvement?

[freecadci]

for feature branch PR_8387. Pipeline 775742197 was triggered at ff9b7f1. All CI branches and pipelines.

[PaddleStroke]

@abdullahtahiriyo yes it's ok. I will implement the parameter observer if it is helpful.

[abdullahtahiriyo]

@PaddleStroke

Thanks. It will be helpful and you will get a performance gain in sections where snapping gets called intensively, as you won't need to access parameters in each iteration.

[PaddleStroke]

@abdullahtahiriyo Do I keep the static map structure even though there're only 2 parameters and the code would be much simpler without?
On one side if we use the same structure then it's same as the coinmanager parameterobserver. And if we have to add more parameters it may scale better. But I don't know if SnapManager will need much more parameters.

Let me know what you prefer.

Edit: I used the same structure as coinmanager in the end. If you prefer otherwise let me know.

[freecadci]

for feature branch PR_8387. Pipeline 778796064 was triggered at dc0d470. All CI branches and pipelines.

[0penBrain]

End of first review. Code review only, didn't tried to compile yet (hopefully other will do :p).

I should say it was very pleasant. Could have been a bit more commented in places where there are some tricky maths, but this is how a PR should look.
I guess it's the maximum size for it to be "easy to reviewed". But the code is well structured, at all levels. It makes it easy to review, and beyond "easy to merge" because maintainer isn't feared what will happen if it introduces a bug,. It will be well contained, and probably easy to solve.

Congrats. Keep up the good work, and if you're looking for other bones to eat, tell me. ;)

[0penBrain]

Better to use same code structure as function just below.

[PaddleStroke]

What do you mean?
isNamingBoxChecked = acts[1]->isChecked(); is not needed because the value is not used anywhere in the widget.

slotElementsChanged(); is not needed either.

[0penBrain]

There are 2 functions that do same thing. Better if they do it the same way, whatever it is.

[abdullahtahiriyo]

@PaddleStroke

I missed your question from two days ago.

I prefer the map solution because of its scalability and uniform initilisation/change.

I have a strong preference to merge first the grid PR and then this. But if I do not manage to move forward the grid PR this weekend, then I will jump to this PR.

[abdullahtahiriyo]

Compilation throws a warning regarding the initialisation order. Take a look to the CI/Ubuntu 22-04 line 4367.

[abdullahtahiriyo]

We got some kind of messy history in the commits. I am rebasing to master and will force-push it as soon as I ensure it is building properly.

[abdullahtahiriyo]

Ok. Now take a look and let me know.

[PaddleStroke]

Got it I will check it out (this evening probably).

[PaddleStroke]

Looks good. Thanks for sharing the different commits separated it's very informative.

[luzpaz]

Fantastic effort!

[PaddleStroke]

Thanks guys for reviewing!