IfcAdvancedBrep for example file cube-advanced-brep

[christophKaiser]

In this pull request, IfcAdvancedBrep will be implemented to run the example file cube-advanced-brep.ifc. This includes the implementation and adjustment of a few other functions, too. A rough overview of the involved functions was given in a comment in the corresponding issue #60.

Finally, this PR will close #60. Probably, issue #58 could be closed, as well. However, this will be investigated at the right time in future. [currently, the non-linked issue is intended]

UPDATE 21.02.2022: This PR will close #58, as well.

[christophKaiser]

While I have analysed the function convertIfcFaceList in commit 346e611 and 032606c, I came over a 'TODO Stefan': https://github.com/tumcms/Open-Infra-Platform/blob/1f1fc35d5c265a7c28675fe82872cd1a7595dfac/Core/src/IfcGeometryConverter/FaceConverter.h#L750-L766 (Note: The cited code is without the new changes.)

It was added in commit 2aec12f last summer. However, I wasn't able to find more information about that. Currently, the function does its purposed task.

@pjanck Should I leave that comment there for the moment, or can you give me more information about it?

My next task in this PR will be an analyse of the function convertIfcFace. This will lead in a (probably) small refactoring of this function and the implementation of its subtype IfcFaceSurface, which is the supertype of IfcAdvancedFace (which I will need at the end).

[pjanck]

Good analysis of the task!

1. About the TODO Stefan: no idea what I meant there. If the function does its task, you can safely delete it.
2. You can change the for loop to for( const auto& face : faces ) - perhaps that was the only purpose of the TODO?
3. convertIfcFace - it seems that the function returns bool - this can be changed to be more in parallel with other functions. I trust you here to judge by yourself what to do. Perhaps it is needed, that a bool is returned. In case of doubt, ask.

[christophKaiser]

[Refactoring of convertIfcFace is still in process.]

Like promised in commit 3ae4b05 “Fixed semi-bug of double triangulation of triangular ifcFaceBound”, this comment gives more details about the fixed issue – especially screenshots of an example.

Before this commit, faces with an exactly triangular bound loop (ifcFaceBound) were added twice to the variable polygon (of type PolyhedronData). First, the triangle face was added in the if-block if (loopVertices3D.size() == 3) (in convertIfcFace). The second adding occurred inside of the function triangulateFace which is called in convertIfcFace. This became implicitly visible by the variable polygon.faceCount, respectively polygon.faceIndices. Moreover, the first adding has not gave respect to holes inside the outer triangle bound, respectively they were covered by the first added triangle face as shown in the following screenshot:

After this fix, the code decides between the case of only one triangular bound loop without an inner bound (= hole), and the case of an arbitrary number of vertices and / or holes inside the face. Now, the hole in my example is visible:

The relevant ifc-data of my example:

#38= IFCFACE((#202,#203));

#202= IFCFACEOUTERBOUND(#366,.T.);
#203= IFCFACEBOUND(#367,.T.);

#366= IFCPOLYLOOP((#527,#526,#606));
#367= IFCPOLYLOOP((#528,#608,#529));

#526= IFCCARTESIANPOINT((0.,0.,0.));
#527= IFCCARTESIANPOINT((1000.,0.,0.));
#606= IFCCARTESIANPOINT((0.,1000.,0.));

#528= IFCCARTESIANPOINT((250.,250.,0.));
#529= IFCCARTESIANPOINT((500.,250.,0.));
#608= IFCCARTESIANPOINT((250.,500.,0.));

[christophKaiser]

The refactoring of the function convertIfcFace is done - at least for the moment. (Of course, the doxygen-comments will be updated soon.) Maybe, its sub-function addArbitraryFaceToPolyhedronData could still need some effort.

Next, I will implement the functions for IfcFaceSurface and IfcAdvancedFace (both are sub-types of IfcFace). During or after this, I can probably decide whether [parts of] addArbitraryFaceToPolyhedronData could be reused there.

[christophKaiser]

Currently, the function convertIfcFaceBound and its superfunction convertIfcFaceBoundList are implemented in the class FaceConverter and are called only by the function convertIfcFace. However, the entity IfcFaceBound is a subtype of IfcTopologicalRepresentationItem (according to the documentation). Thus, should they be implemented in the class ReprestationConverter (shift the existing code into that place) and be called by the function convertIfcTopologicalRepresentationItem as well?

[pjanck]

• no need to move the code, it is fine where it is (the functions are converting faces, right?)
• call appropriately from the convertIfcTopologicalRepresentationItem function - representationConverter has a faceConverter as a member (see e.g. the convertIfcGeometricRepresentationItem)

[christophKaiser]

* call appropriately from the `convertIfcTopologicalRepresentationItem` function - representationConverter has a faceConverter as a member (see e.g. the `convertIfcGeometricRepresentationItem`)

Done in commit 5c2770c (IfcFaceBound). I have observed the same issue with IfcFace and IfcLoop. The calls are added similar in commits 54e028b and 01a3edc.

[pjanck]

I have observed the same issue with IfcFace and IfcLoop.

Very good! If you wish, you can look into IfcVertex as well. Should be easy to put together and some functionality should be already present in the converters.

[christophKaiser]

Very good! If you wish, you can look into IfcVertex as well. Should be easy to put together and some functionality should be already present in the converters.

IfcVertex with IfcVertexPoint are already on my agenda - they are used by the example file cube-advanced-brep. I will come to them after some effort into IfcLoop (with subtypes).

[christophKaiser]

The current pushes implement the functions to convert an IfcAdvancedFace. Surprisingly, the visual result does not look wrong:

I'm surprised because there is a bunch of functions and ifc-entities on my agenda, where I want to take a detailed look into - for example convertIfcLoop. Maybe, they actually do the right things, or it's just luck to get right looking results. Furthermore, in those subfunctions a few ToDo's have been there before the start of this pull request.

Besides that, in the current work are a few things, which might need some rework and additions, like:

• take sameSense into account by the addFace-construction
• the B-Spline surface should be trimmed by the loop boundary: I don't have an idea how to do this task, but I already have a few papers which talk about it.
• discussion about the return type of convertIfcSurface: Each surface (e.g., each side of a cube) is calculated into one Polyhedron. According to my general understanding of a polyhedron, the entire cube should be stored in one Polyhedron-object. Currently, the new function mergePolyhedronsIntoOnePolyhedron copies the data into one object.

[christophKaiser]

Beside small adjustments (commits 2034c1c, bc916f2, b59dcb3, 100c1e1, ef67c56), the structure of functions which are called from convertIfcFace was reordered: A new function computeIfcFaceSurface was introduced in commit b9d8d5d. This function is separated, because it is called by convertIfcFaceSurface and convertIfcAdvancedFace (see calls in commits b9d8d5d and 25d2800) - in both cases it has to do the same computation. In commit 86f6db6, the parameter faceBoundLoops was removed and replaced by the appropriate function call at the location when it is needed. Previously, the function mergePolyhedronsIntoOnePolyhedron has put together several polyhedrons into one polyhedron, but without a topological merge. The result was like a few LEGO bricks in one bowl without sticked together. Now with commit 88e11fe, the existing mesh points are taken into account. If a point (respectively its coordinates) already exists in the target mesh, the existing point will be reused - in this way, several meshes are connected topological. In the example above, the several LEGO bricks in the bowl are sticked together. Doxygen-comments were added in commit 97efdce.

Commits 34b0b46 and 1cb442c added two functions check whether the points of the boundary loop exist in the surface mesh. If not all boundary loop points are in the surface, a warning is displayed in the console output. Its purpose is to indicate, whether the calculated geometry could be wrong (but don't has to be) - especially in the case when the surface should be trimmed by the boundary (but it isn't, due to a missing functionality). The example file basin-advanced-brep.ifc works fine with that. However, this warning is raised in the example cube-advanced-brep.ifc, where it shouldn't. It's caused by a default precision of 1.49e-08 (from GeomSettings->getPrecision, fix value in carve), which is to tight. If the precision of 1.0e-4 from this specifc ifc-file is used (defined in the entity IfcGeometricRepresentationContext), this problem will not occur - an appropriate ToDo is already in the code: https://github.com/tumcms/Open-Infra-Platform/blob/025cf1713a7d8a39d24ac58822d7b8c7e7a1aa03/Core/src/IfcGeometryConverter/GeometrySettings.cpp#L102

Beside this aesthetical issue, this pull request has reached its intended functionality (as communicated in dev-meetings). As indicated in the previous section, the trimming of the face surface by the boundary loop is not implemented. This problem will be explained in more detail in a new issue. Furthermore, the attribute sameSense (see in function computeIfcFaceSurface) is not considered, an appropriate ToDo is left in the code. This attribute affects the normal direction of the faces. At least with the current shading settings in OIP, the normal direction has no visible impact.

Before merge, three ToDo's left on my side:

• [x] Merge tumcms/development (with update to VS2019) into this branch, and make sure it works with VS2019 [at last done in commit a83a47b]
• [x] Add unit tests [done in commits 41e84a4, 14e2007 and bbceed0]
• [x] Update the list in SupportedIFCrepresentations.md [done in commit df133e2]

UPDATE 21.02.2022: Updated check boxes of ToDo's.

[pjanck]

Please mark it as ready for review, then I'll go through the code once more.

[christophKaiser]

Commit ac64eb1 updates the Doxygen comments, which are related to the parameters polyhedron and polyhedronIndices.

A detailed explanation about these parameters is in the description of the function convertIfcFace. As shown in the screenshot below, this section is separated with a bold header:

Furthermore, all functions which have polyhedron and polyhedronIndices as parameter refer to this description by the Doxygen command \see. This generates the section 'See also', as shown in the screenshot: