Modernisation

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
berniev
Posts: 247
Joined: Wed Apr 13, 2022 10:45 pm
Location: Oz

Modernisation

Post by berniev »

I continue my journey learning C++ by quietly refactoring chunks of FreeCAD. Learn something new every day.

Often I find myself asking "Why is XX done this way?".

Quite often there is a clue in the copyright notice :

Code: Select all

Copyright (c) 2002 Jürgen Riegel <juergen.riegel@web.de> 
and what a great job they did.

Git history only goes back to 2011, but changes since are usually very minor. I am finding this is quite common in the C++ codebase. Big chunks of the core architechure are from turn of the century.

Many goodies of c++11, c++14, c++17, and more modern software engineering principles have not been applied. Of course that takes manpower/girlpower yadda yadda etc .... I hear ya.

How does one create an environment to achieve modernisation of a seriously legacy codebase? What do you think?
User avatar
doia
Posts: 251
Joined: Sat May 29, 2021 5:47 am
Location: Düsseldorf

Re: Modernisation

Post by doia »

I‘m trying to understand this code base myself, and often think what the heck, just rewrite from the ground up. But I know, this is hardly feasable.
So where to start to modularize to try a rewrite in more managable chunks? And to keep it maintainable for updates in the future?
User avatar
andrecaldas
Posts: 300
Joined: Fri Jan 27, 2023 8:45 pm
Contact:

Re: Modernisation

Post by andrecaldas »

berniev wrote: Thu Jun 08, 2023 7:39 am I continue my journey learning C++ by quietly refactoring chunks of FreeCAD. Learn something new every day.

[...]

How does one create an environment to achieve modernisation of a seriously legacy codebase? What do you think?
Me too! I do understand that we standing on the shoulders of giants. I am always afraid of offending those giants! :-)

Anyway, every new thing I try, I find it a little dis-motivating... everything looks messy. :-(
And, don't get me wrong... if I take a look on software I wrote a year ago, I find it messy. But FreeCAD is so huge and those problems are so impregnated in so many places...

I guess it would be nice to have a team of people that would choose one problem at a time and try to fix it. And, contrary to most projects I have seen around, I am not advocating for fixing "real world problems"!!! I am talking about conceptual problems. I am talking about problems that would get me mocked out from the forum with a:
What problem does this solve???
I think that a team like that could propose problems to solve, discuss solutions, discuss design... then, we could select one very specific and realistic problem to fix. Count on me! :-)

From the top of my head, things that bother me are basically:

Fake class polymorphism

FreeCAD is full of classes that have enums, and behave as a different thing depending on the enum value. Examples are: sketcher's constraints and ObjectIdentifier. I think the correct approach would be having real subclasses and an abstract base class.

Because of this fake polymorphism, the code is full of if/else switch/case. For example, check this nested switch/case/if with MORE THEN 250 LINES:
https://github.com/FreeCAD/FreeCAD/blob ... .cpp#L1813
Many more examples on the same file.

When you combine fake polymorphism with python, it is even messier, because you also have to guess/test the python function arguments.


Python impregnation

I think the scope of the python interpreted should be well discussed and specified. It is very annoying to have to deal with PyObject back and forth. Python objects are not typed... converting back and forth is quite messy. ObjectIdentifier uses python objects and python strings. And lots of if/else/switch/case.


The shape variable

There is the variable Shape that holds the OCCT generated shape of part objects... it should not be used for things that are not related to displaying data or computations that really depend on the final shape of the object.

In FreeCAD, this "shape" is referenced a lot!!! And (I think) this is the source of all those very famous "topological naming problems". To place something on a wall, you do not need the shape of the wall. You understand the wall conceptually. And conceptually it is build, for example, on top of a sketch. Each sketch element generates many referenceable objects. You do not need the OCCT shape. All you need is a coordinate system. So, the "wall" should provide a FIXED number of coordinate system, according to its structure (defined by its sketch and, possibly, its parameters). The wall would be responsible to name those elements. Each sketch element corresponds to a few coordinate elements: points, lines and planes. This is where other elements should be placed. Not in wall.Shape.Face4!!!

The sketcher could export elements that could be used to generate/specify a coordinate system. The wall shall use those elements to generate/specify other elements that could be used to specify a coordinate system. This would all be independent of the OCCT generated shapes.

Now, what we do now is to depend on the shape generated by OCCT... if OCCT merges faces, divide faces, disappear with faces, etc... you get TNP.


Non RAII

FreeCAD objects have a Restore non-static method. When reading a file, it seems to me that the objects are default initialized to a NON-CONSISTENT state. Then, the Restore method puts it in a (suposedly) consistent state.

Objects should be consistent all the time!!! Not doing this makes people have to check all the time... is it in a consistent state?

For example, "TopoDS_Shape" as a "IsNull()" method! Why do I want to care if a "shape" is or is not null? Why can a shape be null????? Conceptually, I understand that a shape can be empty... but null??? If you call certain methods on a "null" shape, you are in trouble. If you try to "fuse" an empty "TopoShape" with something else, an exception is thrown!!! :-(

We should not have to worry about objects being in an inconsistent state.
User avatar
doia
Posts: 251
Joined: Sat May 29, 2021 5:47 am
Location: Düsseldorf

Re: Modernisation

Post by doia »

andrecaldas wrote: Thu Jun 08, 2023 1:44 pm I am talking about conceptual problems. I am talking about problems that would get me mocked out from the forum
I hear you. See the first reply's to my forum post about trying approaches to transition to a QML based Ui.

So count me in to conceptual discussions for a modernization of FC. I am especially interested in using a Domain Driven approach in finding ways to restructure and improve.

How would we structure FC if we would start from scratch? Let's work out an idealized architecture and than move backwards by finding intermediate steps.

Python impregnation: -> why do C++ workbenches even need to be initialised with Python, even though these can only be internal?

@andrecaldas: What are your main points for an ideal architectural structure of a modernised FC.
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Modernisation

Post by adrianinsaval »

berniev wrote: Thu Jun 08, 2023 7:39 am Git history only goes back to 2011, but changes since are usually very minor. I am finding this is quite common in the C++ codebase. Big chunks of the core architechure are from turn of the century.
This is probably not the point of your post but you can look here for earlier history if you need it: https://github.com/berndhahnebach/All_FreeCAD

It is an import of the old svn history + the git history applied on top up until that date (june 2020)
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Modernisation

Post by adrianinsaval »

andrecaldas wrote: Thu Jun 08, 2023 1:44 pmI think the scope of the python interpreted should be well discussed and specified. It is very annoying to have to deal with PyObject back and forth. Python objects are not typed... converting back and forth is quite messy. ObjectIdentifier uses python objects and python strings. And lots of if/else/switch/case.
I'm no expert on this but please remember that python scripting is one of the main features of FreeCAD, if you can do these things you propose with no regression on the python side ok, otherwise...
In FreeCAD, this "shape" is referenced a lot!!! And (I think) this is the source of all those very famous "topological naming problems". To place something on a wall, you do not need the shape of the wall. You understand the wall conceptually. And conceptually it is build, for example, on top of a sketch. Each sketch element generates many referenceable objects. You do not need the OCCT shape. All you need is a coordinate system. So, the "wall" should provide a FIXED number of coordinate system, according to its structure (defined by its sketch and, possibly, its parameters). The wall would be responsible to name those elements. Each sketch element corresponds to a few coordinate elements: points, lines and planes. This is where other elements should be placed. Not in wall.Shape.Face4!!!
IMO this is not feasible, it's all nice an easy when we talk of simple shapes and simple relationships but as soon as you want to do more complex CAD this idea falls apart and becomes unreasonably complex.
The sketcher could export elements that could be used to generate/specify a coordinate system. The wall shall use those elements to generate/specify other elements that could be used to specify a coordinate system. This would all be independent of the OCCT generated shapes.
you want us to make our own geometric kernel?
Now, what we do now is to depend on the shape generated by OCCT... if OCCT merges faces, divide faces, disappear with faces, etc... you get TNP.
and what makes you think the same won't happen if we didn't depend on occt? TNP is a fundamental problem in all CAD, it's not specific to OCCT so we would need the TN algorithm anyways.
doia wrote: Thu Jun 08, 2023 2:40 pm Python impregnation: -> why do C++ workbenches even need to be initialised with Python, even though these can only be internal?
what do you mean with "can only be internal"?
berniev
Posts: 247
Joined: Wed Apr 13, 2022 10:45 pm
Location: Oz

Re: Modernisation

Post by berniev »

My workflow is roughly:

1. Apply Clang-Format.
2. Work through Clang-tidy warnings until just a few that are too hard or I don't properly understand are left.
3. Set IDE pragmas to ignore those last warnings.
4. Rename as appropriate: variables camelCase, Types to PascalCase. No leading underscores.
5. Fix basic stuff. eg use ternary to replace four lines etc.
6. Remove the many inane, unhelpful or simply incorrect comments! Rename things to make their purpose obvious.
7. Break up responsibilities using initially lambdas then new functions as well. Some untangling of spaghetti.
8. Change initialisations to use uniform initialisation {}. Much easier to differentiate from assignments or function calls.
9. Set default initialisers class variables at declaration. Simplify constructors accordingly.
10. Remove obvious duplications, typically using lambdas.
11. Simplify loops. eg Range based.

.. all the while being careful to not change functionality. I don't know or care what the code actually does. Make sure it still compiles.
Likely there are many hundreds of simple changes so far.

If that went in as a PR it would likely be rejected as too complex. But then you think "there are 5000 files, if each PR is tiny changes this will take a thousand years". I think it is fair to say that many here learnt their craft at FreeCAD, and have learnt old ways. So even most maintainers don't understand the pressing need for change.

At this point I have spent several hours and can start to reason about the file. It becomes immediately apparent that large files with multiple large classes with large functions is not the way to go!

SOLID principles are sadly lacking, as are DRY and others. It seems it is very easy to just create objects all over the place, use macros, and statics are nice :( . Single responsibility? Nah don't need that! Dependencies run amok. Turning this all around is no easy task.

Inheritance is abused (for code reuse rather than for structure). Back in the day inheritance was all the rage, but we know better now. Even the guy that invented it doesn't like it! Composition is much preferred. At the level of a couple of files I can understand DI (as in injection), but at large scale in C++, not. Could get messy?

I know little of cmake, but it is my understanding that much FC cmake uses old techniques.

At the moment anything calls anything else. To reduce the complexity of the problems, it might be an idea to modularise the whole thing into manageable chunks. Build chunks into libraries? But I'm running ahead of my current skills ...
berniev
Posts: 247
Joined: Wed Apr 13, 2022 10:45 pm
Location: Oz

Re: Modernisation

Post by berniev »

adrianinsaval wrote: Thu Jun 08, 2023 3:18 pm look here for earlier history
Thanks, nice to know.
User avatar
doia
Posts: 251
Joined: Sat May 29, 2021 5:47 am
Location: Düsseldorf

Re: Modernisation

Post by doia »

adrianinsaval wrote: Thu Jun 08, 2023 3:55 pm
doia wrote: Thu Jun 08, 2023 2:40 pm Python impregnation: -> why do C++ workbenches even need to be initialised with Python, even though these can only be internal?
what do you mean with "can only be internal"?
A workbench containing C++ code must be compiled together with the FreeCAD source, so it is by definition internal. An external workbench, which can be added via the Add-On manager can only contain Python code. So if a WB is written in pure C++, why does it need the detour of a Python initialization?
I know, it is conceptually done to make Initialization of either type of WB the same. But IMV it shouldn't. The C++ core should not rely on Python to run.
User avatar
andrecaldas
Posts: 300
Joined: Fri Jan 27, 2023 8:45 pm
Contact:

Re: Modernisation

Post by andrecaldas »

adrianinsaval wrote: Thu Jun 08, 2023 3:55 pm I'm no expert on this but please remember that python scripting is one of the main features of FreeCAD, if you can do these things you propose with no regression on the python side ok, otherwise...
Yes! I agree. And I love python. :-)
And the python console teaches me a lot! I actually want a better python integration, but I think it is very hard when no one seems to really understand how things work backstage. It is too complicated. Once I was experimenting with the sketcher's geometry. Everyone was telling me that it was not possible to change the geometries form python. Then, I figured it was not "design". Actually, it is possible, but you have to copy the whole geometries array and put it back the whole array, modified.

adrianinsaval wrote: Thu Jun 08, 2023 3:55 pm IMO this is not feasible [...]
I am a believer! :-)
I am not naive, though.

And I love the quote:
- They did not know it was impossible, so they did it.
adrianinsaval wrote: Thu Jun 08, 2023 3:55 pm you want us to make our own geometric kernel?
I just want to make it very clear that I do not want anyone to make anything. :-)
I mean, I am not demanding or saying that people have to do this or that... I am sharing an idea...

But if by "us" you mean "us, the believers"... then my answer is:

No. I think that a coordinate system object can be specified through the use of some simple (to be listed) parametrized objects. In the beginning, simple ones. I believe that each "thing" (wall, part, geometry, extrusion, etc) is able to "export" those things in a very stable manner.

When I say very stable, I mean that:
1. Things usually do not cease to exist conceptually just because of the shape.
2. When things do disappear, or when the counting changes, or any other topological invariant changes, the name resolution shall fail with the appropriate exception thrown, explaining the user what changed and what actions need to be taken.

I want to avoid TNP as much as possible, and alert the user when some problem happens. I want to do this in a very documented and uniform way. For example, suppose you have a "coordinate object" that specifies a point by means of the intersection of a one dimensional object L and a two dimensional object P. Some sort of "CoordinatePoint p(L,P)"...

Now, suppose this coordinate point can be reached through a path xxx.yyy.zzz. And suppose it exports the intersection points. It could export names like:
1. "n" - the n-th intersection point.
2. "n_q" - the n-th intersection point of "q" known to exist points.

Then, if you refer to "xxx.yyy.zzz.2" you will get:
1. A reference to the second intersection point when it exists.
2. An exception thrown when "recalculating/executing" the object.

If you refer to "xxx.yyy.zzz.2_3", you will get:
1. A reference to the second intersection point when it exists and there are exactly three intersection points.
2. An exception.

When the referencing object is informed of changes or when it recomputes, it shall advise the user of exception in those problematic references. I have started developing a naming resolution scheme that I think is capable of doing this. Take a look at the README.md here:
https://github.com/andre-caldas/FreeCAD ... e/Accessor

When not possible, people can use the shape... but they should be advised not to do so! :-)

To have a coordinate system one needs an xyz parametrization of the space. This can be specified in many ways using: points, vectors, parametrized surfaces, parametrized lines and other xyz parametrizations. A wall xxx, for example, can take a sketch (it has a normal vector), pick up an element yyy and export "xxx.yyy.top_plane" or "xxx.yyy.left_plane". The origin of the coordinate system can be specified as the intersection of two straight lines on the sketch... and they actually do not need to intersect topologically!!!

The document tree keeps the simple constructions of a part design... all structured. You do not need to refer to some "Face27"!!! You can refer to the bottom plane that was generated by the "pad operation". Part of the TNP can be solved when the user clicks on the face s/he wants to use.
adrianinsaval wrote: Thu Jun 08, 2023 3:55 pm and what makes you think the same won't happen if we didn't depend on occt?
Design. :-)

adrianinsaval wrote: Thu Jun 08, 2023 3:55 pm TNP is a fundamental problem in all CAD, it's not specific to OCCT so we would need the TN algorithm anyways.
Yes, it is a fundamental problem. No, we would not need "the" TN algorithm. And I state that even not knowing what you mean by "the" TN algorithm. :-)

We need to deal with TNP. And we shall design it to be easy... not magical.
Post Reply