Removing BaseGeom

Discussions about the development of the TechDraw workbench
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
bensay
Posts: 202
Joined: Wed Dec 22, 2021 8:14 pm
Location: Danmark
Contact:

Removing BaseGeom

Post by bensay »

@wandererfan It's very difficult to get completely rid of BaseGeom for multiple reasons. For instance all the member functions as getStartPoint, getEndPoint etc and member variables like edgeClass. This can be moved to a junk drawer like DrawUtil. TopoDS_Edge could perhaphs be extended to support it natively. BaseGeom itself could also be kept so only all the child classes (like Circle, AOC, Ellipse, AOE) are nerfed. Any suggestions?
User avatar
wandererfan
Veteran
Posts: 6268
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: Removing BaseGeom

Post by wandererfan »

bensay wrote: Mon May 01, 2023 11:25 am It's very difficult to get completely rid of BaseGeom for multiple reasons.
Yep. BaseGeom has been around since TechDraw was Drawing2.
User avatar
bensay
Posts: 202
Joined: Wed Dec 22, 2021 8:14 pm
Location: Danmark
Contact:

Re: Removing BaseGeom

Post by bensay »

@wandererfan Yeahyeah, but what are your suggestions for all these member functions that are located on BaseGeom? Some of them are pretty essential.
User avatar
wandererfan
Veteran
Posts: 6268
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: Removing BaseGeom

Post by wandererfan »

bensay wrote: Tue May 02, 2023 2:38 pm @wandererfan Yeahyeah, but what are your suggestions for all these member functions that are located on BaseGeom? Some of them are pretty essential.
I really need to sit down and think about this.

A lot of those functions should be changed to use the equivalent OCC methods working on the TopoDS_Shape that is inside the BaseGeom. There are also attributes in BaseGeom that OCC doesn't know about, but we need to know - hard/outline/seam/smooth, visible/hidden. Some can be calculated from the OCC geometry (CW/CCW arcs, start/end angles).

Not sure we can avoid having a geometry object of some sort.
User avatar
bensay
Posts: 202
Joined: Wed Dec 22, 2021 8:14 pm
Location: Danmark
Contact:

Re: Removing BaseGeom

Post by bensay »

I don't think it's possible avoiding an geometry object of some sort. But I do believe a lot can be done from skinnying it down. For instance, many of the BaeGeom helper functions that rely on the OCC edge can be moved to an extension of OCC edge. While some results can be calculated without storing the information, those calculations need to be held in a member functions for making it practical to use.

There are also many child classes of BaseGeom that helps providing data to other functions when making QPainterPaths or dimensions. In my opinion, many of these are obsolete as for instance AOC and Circle are very similar. It also makes for a lot of boiler plate code converting an OCC edge to one of these child classes. Therefore, it is probably easier to store geometry as BaseGeom and only converted to child classes when needed. A big reason to all the boiler plate code is that OCC edge curve types cannot simple correspond to a BaseGeom child class; just look at how both OCC Bsplines, bezier and circles can be a TechDraw::AOC.

BaseGeom as a code smell can be categorized as a coupler and a message chain.

This https://github.com/FreeCAD/FreeCAD/pull ... 1530410947 can be called coupled classes, classes that are forced together without actually having any structural relation (CosmeticEdge, GeomFormat and Centerline). And the propertylists can be categorized as classes with different interfaces: they essentially all do the same, but have different interfaces on whether you are dealing with CosmeticEdge, GeomFormat or CenterLine. I really suggest merging all these AppPropertyLists together and giving CosmeticEdge, GeomFormat and Centerline a common parent class as it can help get rid of a lot of bloated/repeated code.

Another issue here is how the OCC edges are delivered. That a TopoDS_Edge can be everything from splines to ellipses and polylines and only their types can be derived from calculating it with BRepSomth is very impractical. I don't know much about OCC or the code behind it, but to me it kinda seems like this class hacking itself, where everything is forced to fit into a specific curvetype. While a TopoDS_Edge can be converted to specific curve types, some information is lost in the process, such as if it's reversed. This also adds to making skinnying BaseGeom a very difficult task, as the alternative is either an OCC class that all curve types are forced into or different classes made for the specific curve type, but where seperate information constantly have to be passed down the message chain together with it.

Firsts steps could be to mainly rely on BaseGeom rather than all it's child classes. And then converted to TechDraw geometry classes when needed. There are also other middle mans that are easy to get rid off: for instance using gp_Pnt when these can be directly harvested from TopoDS_Edge instead of using Base::Vector3d as middle man even though they hold the same information for what we need.

Perhaps, making a UML diagram can help us out. I believe they can be auto generated by doxygen.
User avatar
bensay
Posts: 202
Joined: Wed Dec 22, 2021 8:14 pm
Location: Danmark
Contact:

Re: Removing BaseGeom

Post by bensay »

@wandererfan What is the difference between GeometryObject.edgeGeom and PropertyCosmeticList._lValueList? They both hold cosmeticedge data? So what's the difference??? And are they linked?

It seems like the formatting of the BaseGeom is saved in PropertyCosmeticList._lValueList... and therefore, data constantly have to be looked up both locations to draw on the GUI. But why aren't these data not just kept together? Why are they seperated into BaseGeom/GeometryObject.edgeGeom GeomFormat/PropertyCosmeticList._lValueList just to constantly be looked up both locations
User avatar
wandererfan
Veteran
Posts: 6268
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: Removing BaseGeom

Post by wandererfan »

bensay wrote: Wed Jun 07, 2023 8:39 pm @wandererfan What is the difference between GeometryObject.edgeGeom and PropertyCosmeticList._lValueList? They both hold cosmeticedge data? So what's the difference??? And are they linked?

It seems like the formatting of the BaseGeom is saved in PropertyCosmeticList._lValueList... and therefore, data constantly have to be looked up both locations to draw on the GUI. But why aren't these data not just kept together? Why are they seperated into BaseGeom/GeometryObject.edgeGeom GeomFormat/PropertyCosmeticList._lValueList just to constantly be looked up both locations
GeometryObject::edgeGeom is a vector containing all the edges in the view. Some come from the projection of the shape geometry and some are cosmetic edges.

BaseGeom existed for years before CosmeticEdges and formatting for individual edges was introduced. Originally, only cosmetic edges could have a format.

The BaseGeom object in edgeGeom knows if it is cosmetic or not. It also knows which cosmetic object it represents so the appearance attributes of the cosmetic object can be retrieved.

Now that it is possible to format an edge even if it is from the shape projection, maybe formatting information should be included in BaseGeom.
User avatar
bensay
Posts: 202
Joined: Wed Dec 22, 2021 8:14 pm
Location: Danmark
Contact:

Re: Removing BaseGeom

Post by bensay »

@wandererfan making basegeom and geomformat/lineformat being in the same data structure would help removing a lot of bloat. This is known as couplers, or more specifically inappropriate intimacy https://refactoring.guru/smells/inappropriate-intimacy.

If LineFormat is moved to BaseGeom, whole GeomFormat, looking up indexes, bool cosmetic and int source under BaseGeom will become obsolete. Would remove so much bloat.

Why is it, that the CosmeticEdge is also saved as BaseGeom? What is the difference between a vector and a propertylist? @wandererfan
User avatar
wandererfan
Veteran
Posts: 6268
Joined: Tue Nov 06, 2012 5:42 pm
Contact:

Re: Removing BaseGeom

Post by wandererfan »

bensay wrote: Thu Jun 08, 2023 8:52 am Why is it, that the CosmeticEdge is also saved as BaseGeom? What is the difference between a vector and a propertylist?
The Gui side draws BaseGeom objects, so it needs a BaseGeom representation of CosmeticEdge.

Vector is transient. PropertyList is persistent.
User avatar
bensay
Posts: 202
Joined: Wed Dec 22, 2021 8:14 pm
Location: Danmark
Contact:

Re: Removing BaseGeom

Post by bensay »

After many hours of coding and debugging, I now know why LineFormat was saved in GeomFormat and not in BaseGeom... because BaseGeom is never actually written/saved. What a headache this causes...
wandererfan wrote: Fri Jun 09, 2023 2:02 pm Vector is transient. PropertyList is persistent.
I didn't exactly get before now, that this was what you meant by transient and persistent...

@wandererfan Where can I call the BaseGeom::Save function to start saving BaseGeom?
Post Reply