search

tumcms / Open-Infra-Platform

This is the official repository of the open-source Open Infra Platform software (as of April 2020).
Other
48 stars 22 forks source link

[REF] SplineConverter - Surfaces #455

Closed christophKaiser closed 3 years ago

christophKaiser commented 3 years ago

Goal of this pull request is the code-reactivation and refactoring to convert entities of IfcBSplineSurface including its sub-types. It will fix #199.

Currently, the main functionality will be developed in SplineConverter::convertIfcBSplineSurface to achieve a working code. Some out-commented code fragments were found in the FaceConverter, and some functions of SplineUtilities will be used, as well. The distribution between these classes (especially SplineConverter and FaceConverter) will be probably decided later during developing.

christophKaiser commented 3 years ago

The effort to get the control points in a std::vector<std::vector<EXPRESSReference<typename IfcEntityTypesT::IfcCartesianPoint>>> seems to be quit high - at least in terms of source code (see 75811a2 or below). Did I do something to complicated at this place or missed an already exiting function to do that? https://github.com/tumcms/Open-Infra-Platform/blob/41df2e1639b87691fe91126ebe9b86ce6435a630/Core/src/IfcGeometryConverter/SplineConverter.h#L170-L189

The function FaceConverter::convertIfcCartesianPoint2DVector seems suitable to convert that vector to a vector of carve::geom::vector<3>. This function wasn't called from any code, before. I made slight changes to get it to compile and to work (41df2e1). Probably, further refactoring is reasonable to reuse the function CurveConverter::convertIfcCartesianPointVector or at least PlacementConverter::convertIfcCartesianPoint.

pjanck commented 3 years ago

Did I do something to complicated at this place or missed an already exiting function to do that?

Nope, your approach seems OK. This area of code did not receive any love before, therefore it seems so strange. But you're on the right track!

christophKaiser commented 3 years ago

Added a further suggestion of refactoring the function FaceConverter::convertIfcCartesianPoint2DVector in commit c5322c6: The function design is adapted from CurveConverter::convertIfcCartesianPointVector which converts a list by spliting into single points. The refactored function works similar by spliting an array into several lists. The treatment of lengthFactor and number of dimension are handled by the called function PlacementConverter::convertIfcCartesianPoint.

christophKaiser commented 3 years ago

Commit 0b515e0 implements the setup in the FaceConverter to represent the inheritance structure of IfcBSplineSurface and IfcBSplineSurfaceWithKnots. The parameter type and return type of SplineConverter::convertIfcBSplineSurface is adjusted, as well. Commit 044a541 adjusts the function name to a more suitable one; additionally this should prevent confusion with the function FaceConverter::convertIfcBSplineSurface. Commit 48d2604 moves the obtainment of the control points into the FaceConverter, which makes obsolete the temporary instance of FaceConverter inside the SplineConverter. The control points are loaded based on an IfcBSplineSurface entity.

The commits 31c3e93 and 51241cd changed the existing function of loadKnotArray to be reused for the surface. The call structure of the functions looks like the following schema: 210523 obtainKnotArray schema

Commit 4bfa5a6 prepares the structure to distinguish between IfcBSplineSurfaceWithKnots and IfcRationalBSplineSurfaceWithKnots; its content is in process.

christophKaiser commented 3 years ago

Most reason changes: (not all commits will be named individually)

Commits 4ce7377 and cbfd66a changed the loading process of weights for surfaces: Now, the existing function loadWeightsData receives a vector of IfcReals instead of one EXPRESSReference entity, thus it can be reused by loading the array of weights in case of a surface.

Commit 299dc6d splits the function obtainProperties into multiple functions, the new function obtainNumCurvePoints is called twice by the surface-converter.

Commits 957bd7a and 4f1a128 implement the functions for the calculation of B-Spline-Surfaces and rational-B-Spline-Surfaces. Both sets of functions are quite similar, but with difference of having a weights array, and different internal formulars (steps 3a and 3b). This segregation into two sets of functions was with the attempt of only one task per function (like it was the case in curve calculations, as well).

I took a view in more detail about the meaning of the parameter pos. Commit c627ad8 applies it to the curve points, this resolves an open conversation above. Thanks @pjanck for the given search keywords in the private message!

Commit a373272 implements a suggestion from my side how to distribute the tasks to the three classes:

EDIT 11th June: Updated checkbox. EDIT 15th June: Updated checkbox.

christophKaiser commented 3 years ago

As I distributed the changes to several commits (to give them some kind of change structure) and implemented the additional parts, I was interrupted by an unexpected live lecture (including a time-consuming preparation for it). That's why four commits now popped up before your comment, @pjanck.

In commit 840bd7d, the construction of polyhedrons is adapted to the function convertIfcPlane in a similar way like in convertIfcBSplineSurface (commit 0787fe7). At least for the example file about the IfcAdvancedBrep (which will be a new PR), the IfcPlane will be needed as a surface. I put this change in this PR, because here the restructuring to polyhedrons was done.

About the doxy-comments, I'm not sure whether you wanted to (implicit) refer to something specific. In commit 7286264 are some changes in doxy-comments.

The unit tests will be pushed soon.

christophKaiser commented 3 years ago

Commit 6253f17 adds a suggestion about reducing repetitive code. However, it breaks in some way the rule of 'only one task per function' because two types of B-Splines are now handled in one function. If this doesn't fit in this case, it's no problem to revert this commit - at least, it was a nice personal experience of default parameters in practise.

christophKaiser commented 3 years ago

Unit Tests are added by commits 897d9c9, 1d21e67, and 569f9a8. The markdown table of supported ifc representations is updated in commit 1200977.

From my perspective, the refactoring of the SplineConverter seems to be finished with these commits. However, a performance optimisation lefts (see already existing issue #408); this become noticeable especially during the computation of B-Spline-Surfaces (here it's just a lag of a few seconds).