[Merged] Refactoring ViewProviderSketch

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Refactoring ViewProviderSketch

Post by abdullah »

I have pushed to master some preparation commits. These preparation commits are some general purpose classes and minor refactors in preparation for the actual refactor.

What should happen?

You should not see any difference at all. Most of the new code is not used at all. The refactors are minor and should introduce no issues.

What next?

1. I will let these commits rest during a couple of days. If any issue gets reported, I will fixed it ASAP.

2. Then, I will create a PR for the actual VP refactor. This I will ask for volunteers to work on it before merging. I have taken quite a lot of effort not to break anything. However, the VP refactor is rather big in size (though it only expands Sketcher WB) and I would feel better merging code after some testing phase.
User avatar
-alex-
Veteran
Posts: 1849
Joined: Wed Feb 13, 2019 9:42 pm
Location: France

Re: Refactoring ViewProviderSketch

Post by -alex- »

abdullah wrote: Tue Dec 07, 2021 3:04 pm As for CPU consuming, carbon copy is not CPU consuming
I trust you, I said it is CPU consuming because my FC slow down a lot if I us carbon copy (cc) on medium sketches. However I use FC exclusively on Raspberry PI4 single board, hence my point of view is certainly too much CPU sensitive.
BTW cc is a great feature, no doubt about that.

I do not let any user behind ;)
yes I know that :-) much appreciated
+1
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Refactoring ViewProviderSketch

Post by abdullah »

Update.

I have decided to merge some other unrelated commits before merging the ViewProviderSketch refactor. Now in master.

You should not detect any change in behaviour.

One user compiling in MAC, left this comment:
https://github.com/FreeCAD/FreeCAD/comm ... t-61588367

Can anyone confirm that current master compiles properly in MAC?
User avatar
jonasb
Posts: 162
Joined: Tue Dec 22, 2020 7:57 pm

Re: Refactoring ViewProviderSketch

Post by jonasb »

abdullah wrote: Sat Dec 11, 2021 4:21 pm One user compiling in MAC, left this comment:
https://github.com/FreeCAD/FreeCAD/comm ... t-61588367

Can anyone confirm that current master compiles properly in MAC?
I compiled Today from commit 56d86df5bb20b83a86f1d326acd05674736f3084, that has the b88dad5 in his ancestor chain. No issues here on macOS 10.14 using Apple clang version 11.0.0 (clang-1100.0.33.17)
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Refactoring ViewProviderSketch

Post by abdullah »

jonasb wrote: Sat Dec 11, 2021 10:19 pm
abdullah wrote: Sat Dec 11, 2021 4:21 pm One user compiling in MAC, left this comment:
https://github.com/FreeCAD/FreeCAD/comm ... t-61588367

Can anyone confirm that current master compiles properly in MAC?
I compiled Today from commit 56d86df5bb20b83a86f1d326acd05674736f3084, that has the b88dad5 in his ancestor chain. No issues here on macOS 10.14 using Apple clang version 11.0.0 (clang-1100.0.33.17)
Thank you.
User avatar
Gift
Posts: 769
Joined: Tue Aug 18, 2015 10:08 am
Location: Germany, Sauerland

Re: Refactoring ViewProviderSketch

Post by Gift »

Thanks for your feed back. For whatever reason, I used C++ 14 std. :-D Since I set it to 17 all works fine.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Refactoring ViewProviderSketch

Post by abdullah »

Gift wrote: Sun Dec 12, 2021 3:00 pm Thanks for your feed back. For whatever reason, I used C++ 14 std. :-D Since I set it to 17 all works fine.
I am happy that it is sorted out. :D :D :D

My life has improved substantially with the upgrade to c++17. I think I will be even happier when we raise the bar to c++20. :)
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Refactoring ViewProviderSketch

Post by abdullah »

The VP refactoring is structured as a single commit (Note I have not published the refactor yet, I will do it during the next week).

Architecture Description

Encapsulation and collaboration - restricting friendship - reducing public interface

Summary:
- DrawSketchHandler to ViewProviderSketch friendship regulated via attorney.
- ShortcutListener to ViewProviderSketch friendship regulated via attorney.
- EditModeCoinManager (new class) to ViewProviderSketch friendship regulated via attorney.
- ViewProviderSketch public interface is heavily reduced.

In further detail:
While access from ViewProviderSketch to other classes is regulated via their public interface, DrawSketchHandler, ShortcutListener and EditCoinManager (new class) access to ViewProviderSketch non-public interface via attorneys. Previously, it was an unrestricted access (friend classes). Now this interface is restricted and regulated via attorneys. This increases the encapsulation of ViewProviderSketch, reduces the coupling between classes and promotes an ordered growth. This I call the "collaboration interface".

At the same time, ViewProviderSketch substantially reduces its public interface. Access from Command draw handlers (deriving from DrawSketchHandler) is intended to be restricted to the functionality exposed by DrawSketchHandler to its derived classes. However, this is still only partly enforced to keep the refactoring within limits. A further refactoring of DrawSketchHandler and derivatives is for future discussion.

Complexity and delegation

Summary:
- Complexity of coin node management is dealt with by delegation to helper classes and specialised objects.

In further detail:

ViewProviderSketch is halved in terms of code size. Higher level ViewProviderSketch functions remain

Automatic update of parameters - Parameter observer nested classes

Summary:
- ViewProviderSketch and CoinManager get their own observer nested classes to monitor the parameters relevant to them and automatically update on change.

The split enables that each class deals only with parameters within their own responsibilities, effectively isolating the specifics and decoupling the implementations. It is more convenient as there is no need to leave edit mode to update parameters. It is more compact as it leverages core code.

How to review

Following this order leads, IMO, to the lowest energy path into reviewing the code (should anyone want to):

I. DrawSketchHandler related changes

1. DrawSketchHandler.h and cpp
-> New Class ViewProviderSketchDrawSketchHandlerAttorney, which restricts, defines and regulates access from DrawSketchHandler to ViewProviderSketch (Attorney-Client idiom).
-> DrawSketchHandler provides unified access via inheritance for common operations of drawing edit curves and markers and getting preselected points, curves and axes.

2. CommandConstraints.cpp and CommandCreateGeo.cpp
-> Changes are exclusively for using functionality provided by DrawSketchHandler instead of accessing directly the viewprovider. This enables to have unified behaviour for all commands and to reduce the public API of ViewProviderSketch.

3. CommandSketcherBSpline.cpp
-> ViewProviderSketch and CoinManager are now subscribed to monitor changes on parameters relevant to them. This means it is no longer necessary to notify the ViewProvider explicitly.

4. CommandSketcherTools.cpp
-> The ability to draw a temporal curve (drawEdit) is now provided by DrawSketchHandler (ancestor in inheritance). It hides from derived classes who is responsible for the actual drawing. DrawSketchHandler redirects this request to ViewProviderSketch via the private API (through attorney). ViewProviderSketch delegates this function to EditModeCoinManager. This provides increased encapsulation for DrawSketchHandler inheritors and to ViewProviderSketch which sees its public API reduced.

II. Shortcut listener

This is a mini-refactoring just to promote ordered growth.

1. ShortcutListener.h and cpp
-> Attorney

III. EditCoinManager and ViewProviderSketch

We start with an overview of the parameters, which should be helpful to better understand the split later on:

1. EditModeCoinManagerParameters.h
-> This file comprises all the structures and classes that regulate the drawing of the coin nodes. Part of it is what previously was EditData struct. However, the different parameters are now structured into topics.
-> DrawingParameters
This struct contains every single parameter defining how to draw coin layers, geometry, constraint and information layer.
-> GeometryLayerNode
This struct contains references to the scenegraph nodes necessary to draw the Geometry Layers.
-> MultiFieldId
Convenience class to store a field index (of a MF coin class) and the coin layer in which it is represented.
-> GeometryLayerParameters
Provides mapping between logical layers (GeometryFacade) and the coin layer. Different coin layers are necessary for example for different drawing styles. Geometries in different logical layers may be rendered in a same coin layer if the drawing style is the same.
-> AnalysisResults
Struct to store analysis results obtained during geometry creation, to be used for resizing the grid, for the information overlay layer,...
-> OverlayParameters
Struct to store parameters necessary for drawing the Overlay information layer
-> ConstraintParameters
Struct to store parameters necessary for drawing the constraints
-> EditModeScenegraphNodes
Struct containing pointers to the Coin nodes defining the whole edit mode scenegraph
-> CoinMapping
Struct containing index conversions between {GeoId, PointPos} and MF indices per layer, and VertexId and MF indices per layer. These are updated with every draw of the scenegraph.

Next we move to see what is gone, what remains and what is added to ViewProviderSketch:

2. ViewProviderSketch.h, then ViewProviderSketch.cpp
-> EditData is gone. A minimal part remains within ViewProvidersketch - see ViewProviderParameters nested class. Most of it is now in EditModeCoinManager.
-> Observer functionality has been moved to ParameterObserver nested class. This has expanded to taken Single Responsibility to initialise, monitor and update all ViewProviderSketch relevant parameters.
-> Loose members have been grouped into structs by topic - Dragging, selecting and preselecting, DoubleClick, ViewProviderParameters (part of it coming from all EditData)
-> Public API has been substantially reduced. This means several functions have moved from public to private. All functionality dealing with coin nodes is moved to EditModeCoinManager.
-> Most of the necessary hack to deal with OCC changing the weights of B-Splines from normalized to unnormalized depending on whether the BSpline is rational or no is encapsulated in a single private member function.
-> All the specific members for drawing constraints are going (To EditModeConstraintManager).
-> All colors are gone (to EditModeCoinManagerParameters)
-> Naked pointers for sketchHandler and rubberband are made into smart pointers.
-> All macros and magic numbers are gone (in functionality to EditModeConstraintManager and other helper classes, into enums)
-> Where interface still remains (public or private for friend classes), all this interface is delegated to EditModeCoinManager.
-> SEL_PARAMS macro is removed in favour of specialised member functions that make clear which parameters are necessary for the selection.
-> ViewProviderSketch no longer relies on Solver code for PointId/VertexId calculations. This coupling is fully removed.

Next we move to see where is gone, what is gone from ViewProviderSketch:

3. EditModeCoinManager.h, the EditModeCoinManager.cpp
-> The principle is to reuse code as much as possible and to try not to convert an architectural refactor into a functionality improvement.
-> EditModeCoinManager defines the time span in which ViewProviderSketch is in edit mode.
-> EditModeCoinManager holds objects to the structs defined in EditModeCoinManagerParameters. Helper classes get a reference of the structs they need.
-> EditModeCoinManager has a nested class, ParameterObserver, which inits, monitors and updates all parameters relevant to coin node management. This way, EditModeCoinManager and its helpers always work with up-to-date parameters.
-> There are two main helpers, EditModeConstraintCoinManager and EditModeGeometryCoinManager.
-> There is substantial documentation which should help understand most design decisions, if not all.

4. EditModeInformationOverlayCoinConverter
-> A converter is a specialised object for node creation. This one is for Information Overlay nodes. The intended scope of a converter is that of the function where it is created and called. It is a temporary on each draw().
-> There are a similar converter for geometry. There is not (yet) a similar converter for constraints.
-> There is substantial documentation which should help understand most design decisions, if not all.

5. EditModeGeometryCoinManager.h and .cpp
-> Takes Single Responsibility for creating the edit mode scenegraph for geometry, for converting sketcher geometry in coin information and for updating the geometry colors.
-> It uses a converter class EditModeGeometryCoinConverter

6. EditModeGeometryCoinConverter.h and .cpp
-> This converter is a substantial refactor.
-> It creates and updates actual coin nodes and calculates analysis while processing the geometries. This Analysis is necessary for later on constructing the Overlay Information Layer.
-> It creates the mappings (CoinMapping in EditModeCoinParameters, member of EditModeCoinManager) between indices of MF fields and coin layers and GeoIds and VertexIds.
-> Geometry drawing calculations no longer are done in place, but OCC is leveraged. This leads to a substantially lower footprint and lesser and clearer code to maintain.
-> There is substantial documentation which should help understand most design decisions, if not all.

7. EditModeConstraintCoinManager.h and .cpp
-> Constraint node creation is the least refactored code. Not that it does not need it, but the code itself did not interfere in the architectural refactor. This means that most of the code comes almost directly from ViewProviderSketch. The fact that it does not have a converter helper rather emphasises this point. It is also significant the length of the file by comparison to its geometry counterpart (more that four times longer).
-> This is good in that it does not make this refactor even longer and should lead to a lower amount of introduced defects. It should also help a lot into a future focused refactor of this class, as the scope is heavily limited by the present refactor.

Help testing

While I have taken a lot of precautions (and often restrained myself into refactoring deeper) so that functionality is not affected, every modified line may introduce a new bug.

When I publish the commit, I will ask for volunteers to compile it and give it a good run. You should see nothing new. You should experience no drawback. I think this testing is necessary not to compromise the stability of master. I will try to be responsive in fixing anything that is reported.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Refactoring ViewProviderSketch

Post by abdullah »

It is published:
https://github.com/FreeCAD/FreeCAD/pull/5257

I still wanted to improve the refactoring of the selection/preselection, but now I am overwhelmed with work by my real work. I have decided to tackle it later.

Please, test this code and report here any misbehaviour.

Thanks!:)
wmayer
Founder
Posts: 20202
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Refactoring ViewProviderSketch

Post by wmayer »

The function EditModeConstraintCoinManager::processConstraints is still by far too long. It counts around 1000 lines of code and thus should be heavily re-factored. In a first step all code blocks underneath the switch labels should go to a separate functions of a helper class. This way we may recognize some code duplication that can be reduced.
Post Reply