Issue147 degree elevation #172

cm314 · 2019-07-01T14:11:47Z

fixes algorithm for degree elevation #147 and Decompose NURBS in Bezier-Segments #162

…dinates.

# Conflicts: # src/spl/physical_space.h

…nes.

…ension.

# Conflicts: # src/io/io_converter.cc # src/io/io_converter.h # src/io/vtk_writer.h # src/spl/parameter_space.h # src/spl/spline.h # src/spl/weighted_physical_space.h

KBiber94

Why did you change stuff in the IGA part? Shouldn't be necessary for extending the core functionality of the SplineLib 🤔

Apart from this, numerous but only minor remarks made and (style) changes requested :)

example/iga/src/mapping_handler.h

example/iga/src/element_generator.h

example/iga/src/mapping_handler.h

example/iga/src/solution_spline.h

test/spl/degree_elevation_test.cc

codecov · 2019-09-02T09:57:56Z

Codecov Report

Merging #172 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #172    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          97     98     +1     
  Lines        7479   7773   +294     
======================================
+ Hits         7479   7773   +294

Impacted Files	Coverage Δ
src/baf/knot_vector.h	`100% <ø> (ø)`	⬆️
test/spl/degree_elevation_test.cc	`100% <100%> (ø)`
src/spl/weighted_physical_space.h	`100% <100%> (ø)`	⬆️
src/baf/knot_vector.cc	`100% <100%> (ø)`	⬆️
src/spl/random_spline_utils.h	`100% <100%> (ø)`	⬆️
src/spl/parameter_space.h	`100% <100%> (ø)`	⬆️
src/spl/b_spline.h	`100% <100%> (ø)`	⬆️
src/spl/spline.h	`100% <100%> (ø)`	⬆️
src/spl/nurbs.h	`100% <100%> (ø)`	⬆️
src/io/iges_reader.cc	`100% <0%> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 713be86...016ef76. Read the comment docs.

…/SplineLib into issue147_degree_elevation

KBiber94 · 2019-10-02T06:20:13Z

example/iga/src/mapping_handler.h

-      param_coords[i] = ParamCoord{((elm.GetUpperBound() - elm.GetLowerBound()).get() * itg_pnts[i]
-          + (elm.GetUpperBound() + elm.GetLowerBound()).get()) / 2.0};
+      param_coords[i] = ParamCoord{((elm.GetUpperBound() - elm.GetLowerBound()).get() * itg_pnts[i] +
+          (elm.GetUpperBound() + elm.GetLowerBound()).get()) / 2.0};


Please fix alignment of operands (bold): ... {((...).get() ... + (...).get()) / 2.0};

KBiber94 · 2019-10-02T06:21:04Z

example/iga/src/mapping_handler.h

-          (spline_->GetKnotVector(i)->GetKnot(knot_span[i] + 1).get()
-              - spline_->GetKnotVector(i)->GetKnot(knot_span[i]).get()) / 2;
+          (spline_->GetKnotVector(i)->GetKnot(knot_span[i] + 1).get() -
+              spline_->GetKnotVector(i)->GetKnot(knot_span[i]).get()) / 2;


Please fix alignment as described above.

KBiber94 · 2019-10-02T06:26:18Z

.clang-tidy

@@ -1,4 +1,4 @@
-Checks: '*,-android-*,-llvm-include-order,-fuchsia-overloaded-operator,-fuchsia-default-arguments,-cppcoreguidelines-pro-bounds-constant-array-index'
+Checks: '*,-hicpp-braces-around-statements,-readability-braces-around-statements,-android-*,-llvm-include-order,-fuchsia-overloaded-operator,-fuchsia-default-arguments,-cppcoreguidelines-pro-bounds-constant-array-index'


Are there any other effects that we might not want to have?

KBiber94 · 2019-10-02T06:31:40Z

src/baf/knot_vector.cc

@@ -112,6 +112,14 @@ size_t baf::KnotVector::GetNumberOfKnots() const {
  return knots_.size();
 }

+int baf::KnotVector::GetNumberOfDifferentKnots() const {
+  int number = 1;


Currently, you add 1 here, as you do not initialize with 0. I would consider to replace int number = 1 by int number_non_zero_knot_spans = 0 and add 1 when returning.

KBiber94 · 2019-10-02T06:41:14Z

src/baf/knot_vector.cc

+int baf::KnotVector::GetNumberOfDifferentKnots() const {
+  int number = 1;
+  for (size_t i = 1; i < knots_.size(); ++i) {
+    if (knots_[i].get() > knots_[i - 1].get()) ++number;


++number_non_zero_knot_spans

KBiber94 · 2019-10-02T06:41:39Z

src/baf/knot_vector.cc

+  for (size_t i = 1; i < knots_.size(); ++i) {
+    if (knots_[i].get() > knots_[i - 1].get()) ++number;
+  }
+  return number;


return (number_non_zero_knot_spans + 1);

src/baf/knot_vector.h

KBiber94 · 2019-10-02T07:34:03Z

src/spl/parameter_space.h

@@ -223,6 +202,23 @@ class ParameterSpace {
    }
  }

+  void RecreateBasisFunctions() {
+    for (int current_dim = 0; current_dim < DIM; ++current_dim) {


Please rename current_dim to current_dimension. Same for following variables.

KBiber94 · 2019-10-02T07:36:20Z

src/spl/parameter_space.h

+      basis_functions_current_dim.reserve(knot_vector_current_dim->GetNumberOfKnots() - degree_current_dim.get() - 1);
+      for (int current_basis_function = 0;
+           current_basis_function <
+               (static_cast<int>(knot_vector_current_dim->GetNumberOfKnots()) - degree_current_dim.get() - 1);


Can you remove the static cast? Could you do the line break after the minus operator?

KBiber94 · 2019-10-02T07:41:36Z

src/spl/random_spline_utils.h

@@ -77,7 +77,7 @@ class RandomSplineUtils {
    std::vector<double> weights;
    util::MultiIndexHandler<DIM> point_handler(number_of_points);
    for (int i = 0; i < point_handler.Get1DLength(); ++i, ++point_handler) {
-      weights.emplace_back(util::Random::GetBinomialRandom<double>(0.1, 5, 0.1));
+      weights.emplace_back(util::Random::GetBinomialRandom<double>(0.1, 2, 0.1));


Feel free to create a task for this :)

KBiber94 · 2019-10-02T07:50:47Z

src/spl/spline.h

-    for (int i = static_cast<int>(GetKnotVector(dimension)->GetNumberOfDifferentKnots() - 2); i >= 0; --i) {
-      new_bez_points[i] = DegreeElevateBezierSegment(GetBezierSegment(dimension, i), alpha, dimension);
+        new_bez_points(static_cast<uint64_t>(this->GetKnotVector(dimension)->GetNumberOfDifferentKnots() - 1));
+    for (int current_new_bezier_point = GetKnotVector(dimension)->GetNumberOfDifferentKnots() - 2;


Should we emphasize that current_new_bezier_point is not a point but just an index by renaming the variable?

KBiber94 · 2019-10-02T07:51:19Z

src/spl/spline.h

    }
    return alpha;
  }

  std::vector<int> ProduceBezierSegments(int dimension) {
-    size_t position = GetKnotVector(dimension)->GetNumberOfKnots() - GetDegree(dimension).get() - 2;
+    auto current_knot = GetDegree(dimension).get() + 1;


Index again?

KBiber94 · 2019-10-02T08:03:34Z

src/spl/spline.h

+    for (int k = 0; k < point_handler.Get1DLength(); ++k, ++point_handler) {
+      auto index = point_handler[0];
+      auto pos = point_handler.Get1DIndex();
+      double new_weight = point_length == GetPointDim() ? 1.0 :


At a first glance it seems to me as if there are common operations for B-splines & NURBS during degree elevation. Thus, this functionality should be gathered in the base class spline. However, calls to functions with different implementations for different spline types are always possible. In this case, a virtual method like ComputeWeightForDegreeElevation might be appropriate?

KBiber94

Thanks for your changes so far!

KBiber94 · 2019-10-02T08:34:37Z

src/spl/nurbs.h

+  }
+
+  void SetNewPoint(baf::ControlPoint new_point, double new_weight, std::array<int, DIM> indices) override {
+    physical_space_->SetWeightedControlPoint(indices, new_point, new_weight);


If WeightedPhysicalSpace<DIM>::SetWeightedControlPoint and PhysicalSpace<DIM>::SetControlPoint really implement the same functionality only for physical spaces appropriate for NURBS and B-Splines, respectively, then they should be implementations of the same virtual function! So our problem right now would only be the consequence of a flawed design of these more fundamental classes.

example/\

cm314 · 2019-11-07T12:47:30Z

I'm closing this pull request now as I've connected all open conversations to issues and this branch is outdated.

cm314 added 30 commits December 3, 2018 13:09

implement basic framework for knot removal.

f441f43

add knot removal test.

14afc94

knot removal algorithm working for very simple test case.

e0786ad

add test (not working yet).

b0badc9

tests for 1d knot removal working.

5ba1ad7

not working...

ad353c4

adjust test for 1d knot removal.

96e09b3

add tolerance for knot removal, tests working.

0d9e860

remove redundant use of temporary vector control_points_removed.

826ecf3

move adjustment of control points for knot removal into an own method.

06f7e36

start implementation of knot removal for 1d nurbs.

5f63bae

search for mistake in knot removal for nurbs.

cacaa1e

temporarily exclude redundant tests and iga lib to reduce compile time.

add890b

correct mistake in irit reader and writer concerning homogeneous coor…

356230f

…dinates.

improve getting euclidian control point coordinates in irit reader.

f488064

Merge branch 'mistake_in_irit_writer_reader' into issue146_knot_removal

cf27357

# Conflicts: # src/spl/physical_space.h

remove output.

9ece4d9

correct cpplint error.

cca2e0a

correct mistake in computation of new weights for knot removal.

1aa96b1

add test for knot removal of 2d bspline.

82bdfde

knot removal test for 2d bspline working.

a4a941d

fix problem in multi index handler for last index length equals zero.

b8aa9ed

fix tolerance for knot removal of 2d bsplines.

d2113e0

clean up.

2325ac3

uncomment all tests and dependencies in CMakeLists.txt

7bff609

add computation of moved control points for knot removal of bsplines.

2936f55

correct computation of moved control points for knot removal of bspli…

7db105e

…nes.

knot removal working for 2d bspline and removal of knot in second dim…

dc55c96

…ension.

clean up.

e6dfc78

correct linter errors.

05fde73

cm314 added 7 commits June 25, 2019 10:03

restructure degree elevation tests.

9c93ec0

correct linter errors.

c3d5425

uncomment CMakeLists.txt.

87fc83d

Merge branch 'master' into issue147_degree_elevation

8630671

# Conflicts: # src/io/io_converter.cc # src/io/io_converter.h # src/io/vtk_writer.h # src/spl/parameter_space.h # src/spl/spline.h # src/spl/weighted_physical_space.h

fix merging errors.

d36073d

small style adjustments.

29ac052

add missing include.

6a20ad4

cm314 requested a review from KBiber94 July 1, 2019 15:55

KBiber94 requested changes Aug 21, 2019

View reviewed changes

cm314 added 2 commits September 2, 2019 11:51

add missing include.

ba646c6

requested changes.

674ba9b

cm314 added 2 commits September 2, 2019 11:58

Merge branch 'issue147_degree_elevation' of https://github.com/mfcats…

872bd56

…/SplineLib into issue147_degree_elevation

requested changes in degree elevation tests.

a39ce79

cm314 requested a review from KBiber94 September 23, 2019 07:55

increase tolerance of one complex degree elevation test.

016ef76

KBiber94 reviewed Oct 2, 2019

View reviewed changes

KBiber94 requested changes Oct 2, 2019

View reviewed changes

christophsusen mentioned this pull request Nov 7, 2019

Adapt iga example to global namespaces and add it to travis again #176

Open

cm314 closed this Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue147 degree elevation #172

Issue147 degree elevation #172

cm314 commented Jul 1, 2019

KBiber94 left a comment

codecov bot commented Sep 2, 2019 •

edited

Loading

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 Oct 2, 2019

KBiber94 left a comment

KBiber94 Oct 2, 2019

cm314 commented Nov 7, 2019

		@@ -1,4 +1,4 @@
		Checks: ',-android-,-llvm-include-order,-fuchsia-overloaded-operator,-fuchsia-default-arguments,-cppcoreguidelines-pro-bounds-constant-array-index'
		Checks: ',-hicpp-braces-around-statements,-readability-braces-around-statements,-android-,-llvm-include-order,-fuchsia-overloaded-operator,-fuchsia-default-arguments,-cppcoreguidelines-pro-bounds-constant-array-index'

Issue147 degree elevation #172

Issue147 degree elevation #172

Conversation

cm314 commented Jul 1, 2019

KBiber94 left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 2, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBiber94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cm314 commented Nov 7, 2019

codecov bot commented Sep 2, 2019 •

edited

Loading