tumcms / Open-Infra-Platform

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

Mesh grid lines B-spline surface #538

Closed christophKaiser closed 2 years ago

christophKaiser commented 2 years ago

The mesh lines along the uv-evaluation grid of B-spline surfaces will become visible.

Will close #537.

christophKaiser commented 2 years ago

The commits above (95e068c, 396f461, 3e70c71, b782f44 and 90cc125) do some refactoring in the function convertIfcBSplineSurface. The major change is commit b782f44, which implements another algorithm to construct the mesh grid lines. Now, the polylines are straight from one boundary to the other (instead of u-shaped fragments). Furthermore, this new algorithm considers the one missing boundary line.

christophKaiser commented 2 years ago

Commit bde2dc5 adds the meshGridLines to the geometry of IfcAdvancedBrep. This is the first (probably) straight forward approach, which has impact only on the involved converter functions.

However, I'm not happy with my first suggestion because the haulage of the meshGridLines through these functions is not nice. At this point, I want to suggest an extention of the class ItemData by a new variable meshGridLines, similar to the existing vector of shared pointers polylines. In this way, the mesh grid lines can be treated in correct manner inside the (ifc) converter functions. The function OpenInfraPlatform.Core::ConverterBuwT::createTrianglesJob() would merge the vector meshGridLines into the existing one polylines, because of the currently static visualization of mesh grid lines (according to last meeting). This merge will happen immediately before the polylines are inserted into the buffer. Furthermore, if someone would change the visualization style of the mesh grid lines, there is just this one place to adjust.

Because of the intended change on ItemData, I want to ask for discussion / permission before implement.

christophKaiser commented 2 years ago

The visualization in the viewport already matches the goals of the issue. ToDo:

EDIT 11.04.2022: Updated ToDo

pjanck commented 2 years ago

I approve the idea to change ItemData as proposed. Makes sense to distinguish pure curves from helping grid lines. May prove useful elsewhere as well.

christophKaiser commented 2 years ago

Updated unit tests basin-adanced-brep and cube-advanced-brep. Already with mesh grid lines are the unit tests bspline-surface-with-knots and rational-bspline-surface-with-knots.

christophKaiser commented 2 years ago

The lines are very dense - what is the default there? (@pjanck)

Currently, the meshGridLines are identical with the evaluation points (curve points in u- and in v-direction). The function obtainNumCurvePoints defines a temporary hard-coded default number of curve points as 'number of knots' * 10:

https://github.com/tumcms/Open-Infra-Platform/blob/4fa2e0a4d39c42ae113ff4ba90e04e9b380d1ed3/Core/src/IfcGeometryConverter/SplineConverter.h#L380-L384

One approach how we can reduce the density of the lines is a reduced number of curve points in obtainNumCurvePoints. However, the curves and surfaces would be more rough. A side effect would be a faster computation. Another approach would be to skip curve points, in example a mesh gird line at each 4th curve point. Therefor, a few adjustments are necessary in the loops of line constructions inside the function convertIfcBSplineSurface.

// meshGridLines in u-direction
for (size_t v = 0; v < numCurvePointsV; v++)
{ ... }
// meshGridLines in v-direction
for (size_t u = 0; u < numCurvePointsU; u++)
{ ... }

Is there anything left to do here or can this PR be merged? (@jschlenger)

Probably, it will depend on the described issue with the density of the mesh grid lines. From my sight the PR is finished, if we don't change the density [for now].

pjanck commented 2 years ago

hard-coded default number of curve points as 'number of knots' * 10

I find this plausible and a good choice.

skip curve points

Good idea. Since we're multiplying with 10, lets make it every 5th point (this way it always works in division).

Please do it in a separate PR. Merging.