FreeCADHi!
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??
