[Sketcher] snapping discussion

Have some feature requests, feedback, cool stuff to share, or want to know where FreeCAD is going? This is the place.
Forum rules
Be nice to others! Read the FreeCAD code of conduct!
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

[Sketcher] snapping discussion

Post by paddle »

Hi there,

I am tasked to make the snapping feature in sketcher. This project is funded by Open Toolchain Foundation, a big thanks to them.

First what is snapping : It's complementary with auto-constraint. Say you are drawing a line, your mouse is close to a point. If you have auto constraint, a coincident will be created after the tool finish. Snapping is overriding the mouse coordinates to that point when you are close to it.

The first advantage is that it avoids the geometry from 'jumping around' when you drawing them as it is the case with auto-constraint alone.

The big advantage is when you use tools like rectangular array, offset, rotate and so on where you need the mouse position to be exactly on a point and not just "somewhere close".

Currently there is already 2 kind of snapping, 'grid snapping' and 'vertical/horizontal snapping' for the slot tool.

This project idea is to make a general snapping feature for the geometry tools.
Snapping would be activated by holding CTRL. Or if the toggle 'Snapping' is enabled in preferences (then holding CTRL deactivate it).
The snapping behaviour is as follow :
- Snap to objects : cursor would snap to points / line / circles etc. Additional interesting ideas are : snapping at line center. Snapping on the bisecting angle between 2 lines.
- Snap at 5 degree : For the tools where you select multiple points. Such as slot, line, arc, rotate, rectangular array... Cursor would snap such that the drawn line is a round multiple of 5degree (or a user definable value of degrees).

@abdullah I would like to ask you your opinion on how this should be done.
There're two possibilities, either I make a PR on current master, either I make a PR on your development branch "paddle-widget-testing" or "paddle-widget". I think doing this on current master would be really counter productive as it would generate a lot of conflicts. So if you agree I think it's best to do it on the development branch.

Secondly, can you please advise based on the initial draft of this project, where you want this to be implemented. I recalled we discussed it and you were still uncertain if it would be best in the DSH or in the VPSketch. I would appreciate if you could tell me where it would be the best so that I can do it accordingly.

I wish you a nice day.
Last edited by paddle on Tue Feb 07, 2023 12:09 pm, edited 1 time in total.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: [Sketcher] snapping discussion

Post by abdullah »

paddle wrote: Mon Feb 06, 2023 11:32 am I am tasked to make the snapping feature in sketcher. This project is funded by Open Toolchain Foundation, a big thanks to them.
What are your project intended start/end dates?
paddle wrote: Mon Feb 06, 2023 11:32 am First what is snapping : It's complementary with auto-constraint. Say you are drawing a line, your mouse is close to a point. If you have auto constraint, a coincident will be created after the tool finish. Snapping is overriding the mouse coordinates to that point when you are close to it.

The first advantage is that it avoids the geometry from 'jumping around' when you drawing them as it is the case with auto-constraint alone.

The big advantage is when you use tools like rectangular array, offset, rotate and so on where you need the mouse position to be exactly on a point and not just "somewhere close".

Currently there is already 2 kind of snapping, 'grid snapping' and 'vertical/horizontal snapping' for the slot tool.
Currently we have 'grid snapping' that is not related to any geometry tool, that overrides the mouse coordinates to force a snap to the grid. As you know very well, this is not part of DSH.

We have in several geometry creation tools (not only for the slot DSH) some kind of angle snapping. This operates as part of the specific DrawSketchHandler (DSH) of the tool.

We know that angle snapping is not centralised (which it should). In the past we have treated angle snapping as angle restriction and independently of grid snapping. However, I do not think that decision was a sound one. There may be a conflict between angle restriction and grid snapping, and thus this responsibility should be centralised, so that the centralised entity can resolve the conflict, should a conflict exist.
paddle wrote: Mon Feb 06, 2023 11:32 am This project idea is to make a general snapping feature for the geometry tools.
Snapping would be activated by holding CTRL. Or if the toggle 'Snapping' is enabled in sketcher-edit-mode toolbar (then holding CTRL deactivate it).
The snapping behaviour is as follow :
- Snap to objects : cursor would snap to points / line / circles etc. Additional interesting ideas are : snapping at line center. Snapping on the bisecting angle between 2 lines.
- Snap at 5 degree : For the tools where you select multiple points. Such as slot, line, arc, rotate, rectangular array... Cursor would snap such that the drawn line is a round multiple of 5degree (or a user definable value of degrees).
I understand that in your project requirements, snapping needs to be available for the geometry tools. I see that your project requirements only define "snap to objects" and "snap at a number of degrees" (it omits reference to grid snapping).

However, moving snapping down to the tools (DSH) will interfere with grid snapping (which works also when drag and dropping endpoints of geometries). Interference with existing functionality is a red line.

For this reason, it appears to me that you need to redefine the scope of the project. Instead of restricting to "general snapping for the geometry tools" (so when a DSH is active), it appears to me that the project should rather encompass the sketcher in general (i.e. while in edit mode). This will also allow to drag and drop to other objects. Perhaps, it makes sense to drag and drop following an angle with respect to a reference (or not, I did not think it through, just making the point on the scope).

This is an important consideration because it defines which entity will have the responsibility for the "snapping" functionality. Today, the responsibility for grid snapping lies in ViewProviderSketch. I think ViewProviderSketch should delegate the handling of all types of snapping to a delegate class (SnapManager). Then the functions in ViewProviderSketch that today call the grid snap function, should call an appropriate function of this class.

In SnapManager you may need to access some restricted functionality of ViewProviderSketch (call functions of ViewProviderSketch which are not public). If you do, you will be tempted to pass a VPS pointer to the delegate and change the access from protected/private to public or create new public functions. If you arrive to this point, stop right away. Instead use a client-attorney pattern (you have several examples in ViewProvidersketch, one with CoinManager, another with DrawSketchHandler,...). Yes, it is more code, but it helps keep the encapsulation of ViewProviderSketch and keeps the code growth under control.

For angle snapping, you will likely need to provide a reference from which the angle should be counted. It is acceptable for DSH to ask for a reference or pointer to the snapmanager, so that the DSH can directly communicate with the SnapManager through its public interface (and among others, pass the reference or obtain other information). I think ViewProviderSketch should keep control of which entities can change the snapping. For this reason, this reference or pointer should not be public in VPSketch, but rather private (no public function returning it on VPSketch please). DrawSketchHandler attorney can be leveraged to get this reference for the DSHs.

DrawSketchHandler class should have code that is common to all DSHs that use the functionality. This would minimise the changes to individual DSHs and promote code reuse (and help a lot with maintainability).

As you mention the toolbar for selecting snapping, bear in mind that the UI element that allows snapping should probably somehow integrate the grid snapping functionality. At the end, it may be odd to have the snapping functionality spread over different icons. You already know how this Community loves to engage in UI and toolbar space management. I trust you will find some consensus. Here you may come in conflict with your PR grid (toolbar UI part). Please make sure to let me know if you will conflict with it. We will find a way forward if that happens.

A note goes for your intended means to operate snapping (CTRL), I think this is used elsewhere in the code (rubberband selection?). If there is a conflict of functionalities and keyboard, this needs to be addressed (Community consultation).

Let me know if something does not add up.
paddle wrote: Mon Feb 06, 2023 11:32 am @abdullah I would like to ask you your opinion on how this should be done.
There're two possibilities, either I make a PR on current master, either I make a PR on your development branch "paddle-widget-testing" or "paddle-widget". I think doing this on current master would be really counter productive as it would generate a lot of conflicts. So if you agree I think it's best to do it on the development branch.
Please, do a PR for current master.

If you agree to what I explained above, you would not need to substantially change the DSHs (or at least I do not foresee it at this moment).

Building on the other PR is a risk for you. You want to make a clean feature that will be easily merged (repeat this several times as it is indeed important: I hate to be frustrated, I will address only one functionality at a time.).

The other PR will have to adapt where necessary to this PR. If the impact on the particular DSHs is small, so it will be the conflicts with the other PR.
paddle wrote: Mon Feb 06, 2023 11:32 am Secondly, can you please advise based on the initial draft of this project, where you want this to be implemented. I recalled we discussed it and you were still uncertain if it would be best in the DSH or in the VPSketch. I would appreciate if you could tell me where it would be the best so that I can do it accordingly.
I have sketched how this would work in my head above. Designing an architecture for software is inherently based on incomplete information. Neither I know everything that you want to include in your class, nor may you be sure at this moment of everything you will need. If something does not add up, please seek advise when you become aware of any shortcoming.
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: [Sketcher] snapping discussion

Post by paddle »

abdullah wrote: Mon Feb 06, 2023 5:24 pm What are your project intended start/end dates?
The PR has to be ready by the 15th of March. But there is apparantly no deadline for the merging. This is why I said it could potentially be on the dev branch.

You are correct, in vpsketch it enables to use snap with dragging, which would be very convenient. So it is a better solution.

Thanks for your detailed description. I will follow it and will seek feedback in case something doesn't add up.

- Regarding CTRL shortcut: This is the shortcut that is currently used by Slot tool. Which is why I used it in the initial draft. It didn't seemed to conflict, but I will have a closer look.

- Regarding grid snapping, I don't think it should be toggled with the same button as general snapping. The reason is that grid snapping is very specific and it will probably be very annoying if it is activated when not needed. I seem to recall that in Catia for instance it has 2 separate buttons for this reason. So my plan was to leave the UI toggle for grid snapping untouched. In my opinion it make sense for grid snapping to be in the grid comp because it's very specific to grid. So if someone is using the grid, he will see the grid snapping button. If it's not there he will have to find it somewhere else.
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: [Sketcher] snapping discussion

Post by paddle »

Regarding CTRL key. It'd be great to have some community feedback. I see several possibilities :

- CTRL trigger 'snap to object' + 'snap 5 degree'
- The preference trigger both 'snap to object' + 'snap 5 degree' and reverse CTRL behaviour.

or

- CTRL trigger 'snap 5 degree' only
- The preference trigger only 'snap to object'

The second one might be the most intuitive after thinking about it.

Regarding the idea of a tool in toolbar to toggle, after rethinking about it, I think it's not worth it, just a preference setting is enough for snap to object. For snap to angle the best is to have a key to do that.
Last edited by paddle on Tue Feb 07, 2023 12:43 pm, edited 3 times in total.
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: [Sketcher] snapping discussion

Post by paddle »

abdullah wrote: Mon Feb 06, 2023 5:24 pmIf something does not add up, please seek advise when you become aware of any shortcoming.
So I could implement following the architecture you wanted.

There is one problem I'm not sure how to handle: When I drag a point, the snap does not occur because when dragging a point, the preselection is not updated. See in mouseMove :

Code: Select all

bool ViewProviderSketch::mouseMove(const SbVec2s &cursorPos, Gui::View3DInventorViewer *viewer)
{
    ...
    bool preselectChanged = false;
    if (Mode != STATUS_SELECT_Point &&
        Mode != STATUS_SELECT_Edge &&
        Mode != STATUS_SELECT_Constraint &&
        /*Mode != STATUS_SKETCH_DragPoint &&*/
        Mode != STATUS_SKETCH_DragCurve &&
        Mode != STATUS_SKETCH_DragConstraint &&
        Mode != STATUS_SKETCH_UseRubberBand) {

        std::unique_ptr<SoPickedPoint> Point(this->getPointOnRay(cursorPos, viewer));

        preselectChanged = detectAndShowPreselection(Point.get(), cursorPos);
    }
    ...
}
If I remove Mode != STATUS_SKETCH_DragPoint, then it does snap, but not correctly. Because 50% of time the mouse move select the currently dragged point as the preselected point.
This seems a bit touchy because it gets inside getPointOnRay. And I seem to recall that selection when multiple object overlap is cause to some issues (cf what you said on the rendering order widget).
I see 2 possibilities :

- I leave the snapping when dragging out of current PR.
- I try to modify getPointOnRay such that it returns all the points and try to exclude the dragged point.
User avatar
-alex-
Veteran
Posts: 1856
Joined: Wed Feb 13, 2019 9:42 pm
Location: France

Re: [Sketcher] snapping discussion

Post by -alex- »

Nice project here :-)
paddle wrote: Tue Feb 07, 2023 10:43 am Regarding CTRL key. It'd be great to have some community feedback.
Please do not use CTRL key, neither Shift key because both are used in CAD navigation style. Isn't it?
CAD navigation style with CTRL and shift keys avoids carpal tunnel syndrom. It's a very ergonomic navigation style.

BTW, in sketcher editing mode there is already a bit of confict between CAD nav. style +CTRL key (to pan) and RMB contextual menu.
IMHO a condition in the code is missing, something like:
If CTRL key enabled, then do not trigger sketcher RMB contextual menu.

Thanks for your attention.
User avatar
onekk
Veteran
Posts: 6144
Joined: Sat Jan 17, 2015 7:48 am
Contact:

Re: [Sketcher] snapping discussion

Post by onekk »

There are also ALT keys and Windows Keys, but they are used also by DE or WM.

Probably even on MacOS there is something like Win Key.

but probably beven a simple key will work as example A even if is not on the first lower row.

Sadly international keyboards have mixed position for keys so probably comma (,) or dot (.) could be an option if they retain position at least on US and western country localized Keyboard (sorry I don't want to exclude other countries, but I don't know nothing about oriental keyboards layout)

Regards

Carlo D.
GitHub page: https://github.com/onekk/freecad-doc.
- In deep articles on FreeCAD.
- Learning how to model with scripting.
- Various other stuffs.

Blog: https://okkmkblog.wordpress.com/
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: [Sketcher] snapping discussion

Post by abdullah »

paddle wrote: Tue Feb 07, 2023 8:53 am - Regarding CTRL shortcut: This is the shortcut that is currently used by Slot tool. Which is why I used it in the initial draft. It didn't seemed to conflict, but I will have a closer look.
I think rubberband uses CTRL outside DSH. I am not sure there is interference.

Please give it a thought to Alex' indication of interference (even of current implementation) with navigation control. I do not use navigation style (much) and somehow I have not noticed what he means. He may be able to put forward a detailed example.
paddle wrote: Tue Feb 07, 2023 8:53 am - Regarding grid snapping, I don't think it should be toggled with the same button as general snapping. The reason is that grid snapping is very specific and it will probably be very annoying if it is activated when not needed. I seem to recall that in Catia for instance it has 2 separate buttons for this reason. So my plan was to leave the UI toggle for grid snapping untouched. In my opinion it make sense for grid snapping to be in the grid comp because it's very specific to grid. So if someone is using the grid, he will see the grid snapping button. If it's not there he will have to find it somewhere else.
paddle wrote: Tue Feb 07, 2023 10:43 am Regarding CTRL key. It'd be great to have some community feedback. I see several possibilities :
paddle wrote: Tue Feb 07, 2023 10:43 am Regarding the idea of a tool in toolbar to toggle, after rethinking about it, I think it's not worth it, just a preference setting is enough for snap to object. For snap to angle the best is to have a key to do that.
You have one CTRL key. You have several possible snapping modes and combinations. You may have conflicts between snapping modes. You will have users that do not want to have any snapping at all. The will be users that want to change from one mode (or combination of snapping functionalities) to another. I think it makes sense to have a place where one can select which kind of snapping is activated or how conflicts will be resolved, and will help the user switch from a certain behaviour of the CTRL key (or whatever key we use, or even a button in fancy multibutton mouses) to another.

In this context is where I mentioned also the snapping to grid feature. There will simply be times where a user is using snapping to grid and may want to (temporarily) consider other snapping options, maybe following some preference or priority.

It may not be the core of the feature, but it would make sense that today you think about it, because the code that you write will be different if you consider the possibility of multiple different configurations and conflict resolution.

Such a place may be handy also for future features. I have not thought it through, but I imagine an autoconstraining on snap feature, that may need to be enabled or disabled. A place appears necessary to have this configuration relating to snapping. Give it a deeper thought.
paddle wrote: Tue Feb 07, 2023 11:48 am There is one problem I'm not sure how to handle: When I drag a point, the snap does not occur because when dragging a point, the preselection is not updated.
I am not sure I am understanding the whole issue. If I activate "snap to grid" and I grab then endpoint of a line this point is preselected and when I move the mouse by dragging, it will move with the mouse. Within a certain distance to the grid, it will snap to the grid.

I fear I do not understand what you are trying to convey with "Because 50% of time the mouse move select the currently dragged point as the preselected point". But isn't it always that the preselected point is the point being dragged?

Are you referring to snap to object, and the object is what you want as preselection? If this is the case, there is only one element pre-selection in the sketcher. Is pre-selection of this other object necessary? If it is, you may need to extend getPointOnRay or create another function for an extended usage. It may be possible to retrieve all objects within a radius as you say, or implement an exclusion list, so that one result is disregarded when deciding the best match (the closest point)... actually that could be useful for other features. In these ideas, the preselection should not be updated, as that will interfere with dragging. But you could extend the framework to get a snapTarget (this could be one data member in your SnapManager class).

However, I do not wish to extend your PR beyond boundaries. You take into account that this may be added later on when designing the rest. Focus on the rest, then you may come back to it. Prioritise while not shutting off doors.
onekk wrote: Tue Feb 07, 2023 1:20 pm There are also ALT keys and Windows Keys, but they are used also by DE or WM.

Probably even on MacOS there is something like Win Key.
Yep, thinking MacOS or Chromebook puts some limitations.

SHIFT is used to select constraint selectivity in the Sketcher. I do not know about Alt key. WM = Window Manager, DE = ?
User avatar
onekk
Veteran
Posts: 6144
Joined: Sat Jan 17, 2015 7:48 am
Contact:

Re: [Sketcher] snapping discussion

Post by onekk »

abdullah wrote: Tue Feb 07, 2023 1:51 pm ...
WM = Window Manager, DE = ?
WM window manager like OpenBox

DE desktop environment Gnome, KDE, XFCE.

WM is simpler DE usually have a set of application like File Managers And similar things like pager with uniform appearances and hopefully common shortcuts for simular operations.

I use a WM as it is more responsive and permit a better customization when chosing applications like pager taskbars and similar things.

But is as usual a matter of taste.

Regards

Carlo D.
GitHub page: https://github.com/onekk/freecad-doc.
- In deep articles on FreeCAD.
- Learning how to model with scripting.
- Various other stuffs.

Blog: https://okkmkblog.wordpress.com/
User avatar
-alex-
Veteran
Posts: 1856
Joined: Wed Feb 13, 2019 9:42 pm
Location: France

Re: [Sketcher] snapping discussion

Post by -alex- »

abdullah wrote: Tue Feb 07, 2023 1:51 pm Please give it a thought to Alex' indication of interference (even of current implementation) with navigation control. I do not use navigation style (much) and somehow I have not noticed what he means. He may be able to put forward a detailed example.
Sure, how to reproduce:

- select CAD navigation style
- edit a sketch
- press and hold CTRL hey
- then click (and hold) RMB in order to pan the view (the pan cursuor is displayed)

At this stage you have 2 choices, while CTRL key still held:
1- hold RMB pressed while moving the mouse (not ergonomic, with tension in tendons of hand)
2- release RMB then move the mouse peacfully :-D

In the 2nd situation, when you release the RMB, instead of pan cursor you get the trigger of contextual sketcher menu.
Then it's tricky to get back to pan cursor.

Similar process if you want to tilt, but by holding Shift key. But you should notice there is no conflict in the tilt case, the contextual menu is not triggered. This how it should work.
Post Reply