Solver/CGS architecture changes: Request for comment

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:

Solver/CGS architecture changes: Request for comment

Post by abdullah »

Hi!

DeepSOIC has done quite a remarkable work here:
viewtopic.php?f=10&t=8445&p=69546#p69546

We are trying to figure it out if some changes are acceptable or not and would like the feedback from the developers, not necessarily main developers (though you are always welcome), but also of others, specially if you are currently working (or have worked) in the solver/CGS.

FOR THE DEVELOPERS
=================
Lets discuss the main implementation issues. I would start from here:
https://github.com/abdullahtahiriyo/Fre ... 5f043b8dc8

Those are the first bunch of changes. I put that code because it is all squashed and you can see the whole at a time.

The first proposal of DeepSOIC is to modify the structure of GCS to use polymorphism, this enables to treat all the elements homogeneously, and at the same opens the door for seamless integration of specific calls. For example: CalculateNormal is in the base class a pure virtual function. So every new solver element shall implement such a function, and constraints requiring of a normal, may use generic code in combination with this specific code to build constraints (notably the constraints that he proposes in the post above).

src/Mod/Sketcher/App/freegcs/Geo.h

Code: Select all
- class Line
+ class Curve //a base class for all curve-based objects (line, circle/arc, ellipse/arc)
+ {
+ public:
+
+ //returns normal vector. The vector should point inward, indicating direction towards center of curvature.
+ //derivparam is a pointer to a curve parameter to compute the derivative for. if derivparam is nullptr,
+ //the actual normal vector is returned, otherwise a derivative of normal vector by *derivparam is returned
+ virtual Vector2D CalculateNormal(Point &p, double* derivparam = nullptr) = 0;
+
+ //adds curve's parameters to pvec (used by constraints)
+ virtual int PushOwnParams(VEC_pD &pvec) = 0;
+ //recunstruct curve's parameters reading them from pvec starting from index cnt.
+ //cnt will be incremented by the same value as returned by PushOwnParams()
+ virtual void ReconstructOnNewPvec (VEC_pD &pvec, int &cnt) = 0;
+ virtual Curve* Copy() = 0; //DeepSOIC: I haven't found a way to simply copy a curve object provided pointer to a curve object.
+ };
+
+ class Line: public Curve
{
public:
Line(){}
Point p1;
Point p2;
+ Vector2D CalculateNormal(Point &p, double* derivparam = nullptr);
+ virtual int PushOwnParams(VEC_pD &pvec);
+ virtual void ReconstructOnNewPvec (VEC_pD &pvec, int &cnt);
+ virtual Line* Copy();
};
- class Arc
+ class Circle: public Curve
{
public:
- Arc(){startAngle=0;endAngle=0;rad=0;}
- double *startAngle;
- double *endAngle;
- double *rad;
- Point start;
- Point end;
+ Circle(){rad = 0;}
Point center;
+ double *rad;
+ Vector2D CalculateNormal(Point &p, double* derivparam = nullptr);
+ virtual int PushOwnParams(VEC_pD &pvec);
+ virtual void ReconstructOnNewPvec (VEC_pD &pvec, int &cnt);
+ virtual Circle* Copy();
};
- class Circle
+ class Arc: public Circle
{
public:
- Circle(){rad = 0;}
- Point center;
- double *rad;
+ Arc(){startAngle=0;endAngle=0;rad=0;}
+ double *startAngle;
+ double *endAngle;
+ //double *rad;
+ Point start;
+ Point end;
+ //Point center;
+ virtual int PushOwnParams(VEC_pD &pvec);
+ virtual void ReconstructOnNewPvec (VEC_pD &pvec, int &cnt);
+ virtual Arc* Copy();
};

My impression of looking at the original code of GCS is that it is presented in a very lean structure, probably thinking about performance and the penalty in performance that may suppose complex objects.

IMO, using a virtual class "Curve" and deriving from it all the solver elements (e.g. line, circle), should not have a big performance loss if any. At the end, you are passing a pointer (not a copy of the object), and passing a pointer(reference) should not have any performance penalization. The dynamic determination of the type in execution time, should not bear much significance. Note: Other parts of the code, in the constraints, include a full copy of the object, but this is IMO another issue to discuss separately.

The presence of pure virtual functions obliges a developer adding a new element (hyperbola, parabola,...) to implement the function. I do not think that this is a major problem.

Does anybody know a reason why we should not adopt this hierarchy??
User avatar
yorik
Founder
Posts: 13640
Joined: Tue Feb 17, 2009 9:16 pm
Location: Brussels
Contact:

Re: Solver/CGS architecture changes: Request for comment

Post by yorik »

I think the freecgs library was entirely coded by logari81, but there is a long time he doesn't appear here... It might be interesting to ask him
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Solver/CGS architecture changes: Request for comment

Post by abdullah »

I would love to hear from logari. He appeared once in the sketcher ellipse thread a couple of months ago. But how can we draw his attention?

There are some questions like this one that we should discuss for the sake of the future of the solver...
jmaustpc
Veteran
Posts: 11207
Joined: Tue Jul 26, 2011 6:28 am
Location: Australia

Re: Solver/CGS architecture changes: Request for comment

Post by jmaustpc »

His last post is the one you are talking about and it was approximately a month ago.

If something is important enough, a member can be sent a PM. The Private Message system will send the member an email to their hidden real world Email address, informing them that a new private message is waiting for them. Not something you want to do to often, but if Yorik agrees, I think it would be reasonable to send him a PM regarding this matter.


Jim
User avatar
yorik
Founder
Posts: 13640
Joined: Tue Feb 17, 2009 9:16 pm
Location: Brussels
Contact:

Re: Solver/CGS architecture changes: Request for comment

Post by yorik »

jmaustpc wrote:I think it would be reasonable to send him a PM regarding this matter.
Yes of course! Nobody better than him to discuss changes to the freecgs lib
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Solver/CGS architecture changes: Request for comment

Post by abdullah »

Great suggestion Jim!! I have PM-ed him. Hopefully he will have some free time for us.
logari81
Posts: 658
Joined: Mon Jun 14, 2010 6:00 pm

Re: Solver/CGS architecture changes: Request for comment

Post by logari81 »

Hi, it seems that you have a quite established idea of what you are aiming at. So your questions appear to be rather rhetorical. In any case if you want my honest opinion:

1. the idea of each geometry element defining a normal direction is something useful that I would do. Using angle constraints is something that I have already exploited in point-to-point tangency and perpendicularity.

2. regarding the use of polymorphism, I wouldn't do. Geometrical constraints solving is a quite simple problem which shouldn't really require any great OO infrastructure and not that much code at all.

3. regarding your ellipse support, the internal alignment stuff is something that I would definitely avoid. In general for ellipses I would do it in a completely differen way, reusing the circle/arc geometry and constraints with an additional affine transformation constraint.

Since it seems that you have time and motivation for pushing this forward I suggest that you fork freegcs choosing a new name and implementing your changes. Please don't merge your changes before you do the renaming because it may cause confusion if I release the current version of freegcs as an independent library.

I would be glad if I would have the time to prove with code that my approach is preferable. Since I don't have the time for coding, it would be a waste of our time to try to explain it and convince you with words.

I wish you good luck with the rest.

P.S. if I was you, I would prefer to invest my free time in fixing the poor dof counting performance, which is easy to fix and affects users very much.
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: Solver/CGS architecture changes: Request for comment

Post by DeepSOIC »

logari81, thank you very much for your opinion.
I find that your points 1 and 2 are almost in contradiction, because the normal can be calculated in place if object type is known beforehand. 1 alone does make sense to avoid duplicating a lot of math though.
From the point 2, i.e. polymorphism... Well, it is aimed to simplify the introduction of new objects to sketcher, less extra coding is required. I'm very lazy! There are parabola and hyperbola in plans, as well as (probably) Beziers and B-splines. I can imagine, how long the combination lists will grow...
a=line, b=line
a=circle b=line
a=circle b=circle
a=arc b=line
a=arc b=circle
a=arc b=arc
a=ellipse b=line
a=ellipse b=circle... ugh. I'm already tired!
User avatar
DeepSOIC
Veteran
Posts: 7896
Joined: Fri Aug 29, 2014 12:45 am
Location: used to be Saint-Petersburg, Russia

Re: Solver/CGS architecture changes: Request for comment

Post by DeepSOIC »

logari81 wrote:Since it seems that you have time and motivation for pushing this forward I suggest that you fork freegcs choosing a new name and implementing your changes. Please don't merge your changes before you do the renaming because it may cause confusion if I release the current version of freegcs as an independent library.
A new name?.. hmm.. maybe.. DeepGCS 8-) 8-) or... SketchGCS? I'm open for suggestions!
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Solver/CGS architecture changes: Request for comment

Post by wmayer »

OpenGCS, LibreGCS, ...
Post Reply