fix(Triangle): better handle flat triangle normal computation

[BotellaA]

fix #1194

[github-actions]

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-tidy (v21.1.8) reports: 12 concern(s)

• include/geode/geometry/basic_objects/triangle.hpp:58:11: warning: [cppcoreguidelines-special-member-functions]

  class 'GenericTriangle' defines a copy constructor, a copy assignment operator, a move constructor and a move assignment operator but does not define a destructor

     58 |     class GenericTriangle
        |           ^

• include/geode/geometry/basic_objects/triangle.hpp:110:11: warning: [cppcoreguidelines-special-member-functions]

  class 'OwnerTriangle' defines a copy constructor, a copy assignment operator, a move constructor and a move assignment operator but does not define a destructor

    110 |     class OwnerTriangle
        |           ^

• include/geode/geometry/basic_objects/triangle.hpp:129:11: warning: [cppcoreguidelines-special-member-functions]

  class 'Triangle' defines a copy constructor, a copy assignment operator, a move constructor and a move assignment operator but does not define a destructor

    129 |     class Triangle : public GenericTriangle< RefPoint< dimension >, dimension >
        |           ^

• include/geode/geometry/basic_objects/triangle.hpp:139:9: warning: [google-explicit-constructor]

  single-argument constructors must be marked explicit to avoid unintentional implicit conversions

    139 |         Triangle( const OwnerTriangle< dimension >& other ) noexcept;
        |         ^
        |         explicit 

• src/geode/geometry/basic_objects/triangle.cpp:41:57: warning: [readability-function-cognitive-complexity]

  function 'simple_pivot_and_normal' has cognitive complexity of 11 (threshold 10)

     41 |     std::optional< geode::internal::PivotNormalResult > simple_pivot_and_normal(
        |                                                         ^
  /__w/OpenGeode/OpenGeode/src/geode/geometry/basic_objects/triangle.cpp:47:9: note: +1, including nesting penalty of 0, nesting level increased to 1
     47 |         for( const auto pivot : geode::LRange{ 3 } )
        |         ^
  /__w/OpenGeode/OpenGeode/src/geode/geometry/basic_objects/triangle.cpp:49:46: note: +2, including nesting penalty of 1, nesting level increased to 2
     49 |             const auto next = pivot + 1 == 3 ? 0 : pivot + 1;
        |                                              ^
  /__w/OpenGeode/OpenGeode/src/geode/geometry/basic_objects/triangle.cpp:52:13: note: +2, including nesting penalty of 1, nesting level increased to 2
     52 |             if( edge_length < geode::GLOBAL_EPSILON )
        |             ^
  /__w/OpenGeode/OpenGeode/src/geode/geometry/basic_objects/triangle.cpp:57:42: note: +2, including nesting penalty of 1, nesting level increased to 2
     57 |             const auto prev = pivot == 0 ? 2 : pivot - 1;
        |                                          ^
  /__w/OpenGeode/OpenGeode/src/geode/geometry/basic_objects/triangle.cpp:60:13: note: +2, including nesting penalty of 1, nesting level increased to 2
     60 |             if( length1 < geode::GLOBAL_EPSILON )
        |             ^
  /__w/OpenGeode/OpenGeode/src/geode/geometry/basic_objects/triangle.cpp:67:13: note: +2, including nesting penalty of 1, nesting level increased to 2
     67 |             if( length > geode::GLOBAL_ANGULAR_EPSILON )
        |             ^

• src/geode/geometry/basic_objects/triangle.cpp:112:47: warning: [cppcoreguidelines-avoid-magic-numbers]

  3. is a magic number; consider replacing it with a named constant

    112 |         return ( point0 + point1 + point2 ) / 3.;
        |                                               ^

• tests/geometry/test-triangle.cpp:35:6: warning: [misc-use-internal-linkage]

  function 'test' can be made static or moved into an anonymous namespace to enforce internal linkage

     35 | void test()
        |      ^
        | static 

• tests/geometry/test-triangle.cpp:37:26: warning: [readability-identifier-length]

  variable name 'a' is too short, expected at least 3 characters

     37 |     const geode::Point3D a{ { 0.0, 0.0, 0.0 } };
        |                          ^

• tests/geometry/test-triangle.cpp:39:26: warning: [readability-identifier-length]

  variable name 'b' is too short, expected at least 3 characters

     39 |     const geode::Point3D b{ { 0.6, 0.3 * geode::GLOBAL_EPSILON, 0.0 } };
        |                          ^

• tests/geometry/test-triangle.cpp:40:26: warning: [readability-identifier-length]

  variable name 'c' is too short, expected at least 3 characters

     40 |     const geode::Point3D c{ { 1.0, 0.0, 0.0 } };
        |                          ^

• tests/geometry/test-triangle.cpp:49:9: warning: [bugprone-unchecked-optional-access]

  unchecked access to optional value

     49 |         normal_ref.value() == answer, "Wrong normal of reference triangle" );
        |         ^~~~~~~~~~

• tests/geometry/test-triangle.cpp:54:9: warning: [bugprone-unchecked-optional-access]

  unchecked access to optional value

     54 |         pivot_ref.value() == 0, "Wrong pivot of reference triangle" );
        |         ^~~~~~~~~

Have any feedback or feature suggestions? Share it here.

[BotellaA]

@anj1 did you really have an issue with this code? I am unsure about issue #1194, which seems to produce a valid normal.

[anj1]

I'm interpeting GLOBAL_ANGULAR_EPSILON as a threshold for triangle validity, while the fallback appears to treat it only as a direct-pivot conditioning threshold.

So you bring up a good point. Which one of these is correct? If the fallback behavior is intended, maybe it's better to separatenormal() from strict angular degen detection.

[BotellaA]

GLOBAL_ANGULAR_EPSILON is meant to detect too small/large angle between vectors. Those angles make any geometric computation unreliable. So first, try to compute a pivot, a triangle vertex where its angle is good enough.

Then, if no vertex is found but the triangle is not degenerate (meaning all vertices are not aligned on a thick line), the triangle can be split to generate a smaller triangle where the angles are enough to compute a normal.

[BotellaA]

Not sure to follow what you mean by separate the two

[anj1]

I think based on your explanation the original code probably implemented the correct semantics then.

[BotellaA]

If you have a counter example, do not hesitate to reach out.