[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:

[Merged] Refactoring ViewProviderSketch

Post by abdullah »

Hi!

This is an announcement that I am initiating a substantial refactoring of ViewProviderSketch.

As it is currently, trying to incorporate Geometry Layers will only result in a bigger chaos. The class has grown so large that keeping track of the different parts is a daunting task and incorporating new functionality in the current state only contributes to a larger class.

My (current) main target is ::draw().

Wish me luck ;)
User avatar
chennes
Veteran
Posts: 3878
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Refactoring ViewProviderSketch

Post by chennes »

Good luck, and thanks for the heads-up!
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Refactoring ViewProviderSketch

Post by openBrain »

Good luck. I already got a tendinitis just scrolling this file top to bottom with the mouse wheel a single time. :P
User avatar
M4x
Veteran
Posts: 1472
Joined: Sat Mar 11, 2017 9:23 am
Location: Germany

Re: Refactoring ViewProviderSketch

Post by M4x »

Good luck@ :mrgreen:
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Refactoring ViewProviderSketch

Post by abdullah »

openBrain wrote: Wed Oct 20, 2021 6:42 am Good luck. I already got a tendinitis just scrolling this file top to bottom with the mouse wheel a single time. :P
Oh! You are a strong man. Normal people can get a tendinitis just by scrolling with the mouse wheel the ::draw() function :lol:

I am offloading all the Coin related drawing on a new class CoinManager.

- I have refactored all the filling of coin nodes for the geometry. This is already in quite a nice state. I have systematically unloaded the responsibility for providing the coordinates of points and curves on OCC and have managed to substantially reduce the codesize.
- I have refactored the creation of the information layer out of ViewProviderSketch and into CoinManager. This I still need to rework it to remove code repetitions and reduce the codesize. Now it is just "hidden" somewhere else.
- Then after that is the drawing of constraints which was about half the original draw() function.

I think I might be able to run the creation of the geometry nodes and the constraint nodes in parallel.

When extending that to different layers, I might be able to run the creation of the geometry of the different layers in parallel too. Then join the tasks, and merge the geometry analysis of each layer (so that all BSplines use the same scaling of comb, poles, etc...). Then create the information layers of each geometry layer in parallel too. And all this in parallel with the constraint nodes. Joining at the end of the function. Then of course, some profiling may be needed to see if it actually makes sense to run all this in parallel or not (or from which size of sketch onward it makes sense). But I think that for big sketches it should help.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Refactoring ViewProviderSketch

Post by abdullah »

An update is due, so that other developers can keep track of this refactor effort (at least so that there is awareness it is still being undertaken).

The task to refactor ViewProviderSketch::draw() has become quite a daunting effort. Mainly because it has ended up in becoming a major refactor of about every single aspect of ViewProviderSketch where coin is involved. This is at least 70% of the original weight (and it was heavy).

Now I am going through preselection and I estimate I have done around 60% of the total effort.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Refactoring ViewProviderSketch

Post by abdullah »


Summary: Refactoring extends to DrawSketchHandler.


Extra! Extra!

ViewProviderSketch is not longer getting along with DrawSketchHandler. It is filing for divorce. It has already unfriend him, they no longer talk to each other and now they communicate via attorney.

Well, that was just to say that I am extending the refactor to DrawSketchHandler. The reason is that I am cleaning the public interface of ViewProviderSketch and this friendship was very odd. They were friends, but ViewProviderSketch would expose many functions public, so that the inheritors of DrawSketchHandler could access it, which were a violation of the encapsulation of ViewProviderSketch itself (anybody with a pointer to the viewprovider could draw an arbitrary line on the 3D view).

Then DrawSketchHandler had access to all the guts of ViewProviderSketch. Violation of privacy aside, this was promoting an unordered grow of the codebase. The attorney in the middle seems to have calm down the situation, and now the attorney defines the interface and would promote a more loose coupling (they still need to colaborate).

Now ViewProviderSketch.h is 231 lines long and ViewProviderSketch.cpp is only 3558 lines long (coming from 498 lines and 7367 lines respectively). There is much more documentation and diet has not ended yet. There is still quite some work to be done on the place where this difference of implementation lines went though.
chrisb
Veteran
Posts: 53922
Joined: Tue Mar 17, 2015 9:14 am

Re: Refactoring ViewProviderSketch

Post by chrisb »

Nice metaphor :D . Thanks for keeping us updated.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
User avatar
M4x
Veteran
Posts: 1472
Joined: Sat Mar 11, 2017 9:23 am
Location: Germany

Re: Refactoring ViewProviderSketch

Post by M4x »

Awesome effort and nice story :D
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Refactoring ViewProviderSketch

Post by adrianinsaval »

abdullah wrote: Sun Nov 14, 2021 7:25 am
Now ViewProviderSketch.h is 231 lines long and ViewProviderSketch.cpp is only 3558 lines long (coming from 498 lines and 7367 lines respectively)

🤯
You're a beast. The divorce is really taking a big toll on them.
Post Reply