removed RotateUp and uint #1000

elalish · 2024-10-19T04:36:52Z

Fixes #999
Fixes #996

@starseeker I don't have exactly your repro, but I believe this should fix the problem you reported. Would you mind verifying that?

elalish · 2024-10-19T04:39:21Z

test/boolean_complex_test.cpp

-    manifold::Manifold rotated_cyl =
-        origin_cyl.Transform(manifold::RotateUp(evec));
-    manifold::Manifold right = rotated_cyl.Translate(ev1);
+    quat q = rotation_quat(normalize(evec), vec3(0, 0, 1));


@pca006132 I was a little suprised I didn't need la::rotation_quat or manifold::la::rotation_quat. I recall you made a comment about my namespacing - did I do it wrong?

I think this is a weird thing called Argument-dependent lookup for functions, which basically adds the argument type namespaces into the lookup search path. I don't think we have anything wrong with our namespacing.

Oh, that's cool! Thanks for the explanation.

elalish · 2024-10-19T05:18:17Z

On second thought, considering the very non-obvious behavior from #996, perhaps we should add assertions on all our transform inputs that they are finite.

starseeker · 2024-10-19T11:49:19Z

I'll try it out when I get a chance, but the first thing to do is remove the "if (len < 0.03) continue;" test with the TODO comment from the offset test, if that's not already removed - that was hitting the issue for me.

starseeker · 2024-10-19T20:14:37Z

Confirmed - removing the len < 0.03 filter still results in successful running of the offset test.

starseeker · 2024-10-19T23:40:21Z

@elalish did you want to remove that workaround as part of this PR, or as a separate one?

elalish · 2024-10-20T00:16:31Z

Yeah, I'll deal with it here.

elalish · 2024-10-20T04:25:18Z

src/boolean_result.cpp

+  if (inQ_.status_ != Manifold::Error::NoError) {
+    auto impl = Manifold::Impl();
+    impl.status_ = inQ_.status_;
+    return impl;


I changed this to pass through the input error instead of changing it, which I think makes it a be easier to deduce what went wrong first.

elalish · 2024-10-20T04:26:27Z

include/manifold/manifold.h

+    case Manifold::Error::FaceIDWrongLength:
+      return stream << "Face ID Wrong Length";
+    case Manifold::Error::InvalidConstruction:
+      return stream << "Invalid Construction";


I got tired of not being able to see what the error was. I really wish C++ had a way to turn enums into strings automatically, but apparently we need this lovely boiler plate instead.

I feel that a more general approach would be to implement this as a toString function.

elalish · 2024-10-20T04:27:13Z

src/impl.cpp

@@ -513,6 +513,10 @@ Manifold::Impl Manifold::Impl::Transform(const mat3x4& transform_) const {
    result.status_ = status_;
    return result;
  }
+  if (!all(isfinite(transform_))) {
+    result.MarkFailure(Error::NonFiniteVertex);


Added this to catch any kind of bad transform.

Apparently we need to make it explicit, i.e. all(la::isfinite(transform_)) to make msvc happy.

fwiw, in BRL-CAD when we use C++ we generally are explicit with the namespaces to avoid lookup surprises. It's definitely a readability tradeoff though.

elalish · 2024-10-20T04:28:26Z

src/properties.cpp


  bool operator()(size_t edge) const {
    const Halfedge halfedge = halfedges[edge];
    if (halfedge.startVert == -1 || halfedge.endVert == -1) return true;
    if (halfedge.pairedHalfedge == -1) return false;

-    if (!std::isfinite(vertPos[halfedge.startVert][0])) return false;
-    if (!std::isfinite(vertPos[halfedge.endVert][0])) return false;


Not sure when this showed up, but we have a dedicated IsFinite method which was being elided by this throwing a NonManifold error, which was pretty unclear.

elalish · 2024-10-20T04:40:17Z

@pca006132 I think this one is ready to merge, but I have no idea why the Windows compiler is suddenly pissed - doesn't look like the spew has anything to do with these changes. Thoughts?

pca006132 · 2024-10-20T05:00:16Z

From the error message:

D:\a\manifold\manifold\src\impl.cpp(516,6): error C2668: 'linalg::isfinite': ambiguous call to overloaded function [D:\a\manifold\manifold\build\src\manifold.vcxproj]
D:\a\manifold\manifold\include\manifold/linalg.h(1298,44): message : could be 'linalg::mat<bool,3,4> linalg::isfinite<manifold::mat3x4>(const A &)' [found using argument-dependent lookup] [D:\a\manifold\manifold\build\src\manifold.vcxproj]
          with
          [
              A=manifold::mat3x4
          ]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\corecrt_math.h(401,32): message : or       'bool isfinite<manifold::mat3x4>(_Ty) noexcept' [D:\a\manifold\manifold\build\src\manifold.vcxproj]
          with
          [
              _Ty=manifold::mat3x4
          ]

Apparently it is some weird header in MSVC (TM) that tries to match for every isfinite(T) and causes lookup issue...

* removed RotateUp and uint * avoid new API * simplify * change error reporting * fixed error reporting * fix compile * addressing feedback

removed RotateUp and uint

74b74c7

elalish self-assigned this Oct 19, 2024

elalish commented Oct 19, 2024

View reviewed changes

elalish requested a review from pca006132 October 19, 2024 04:39

avoid new API

548f377

pca006132 approved these changes Oct 19, 2024

View reviewed changes

elalish added 2 commits October 19, 2024 10:55

simplify

0452de2

Merge branch 'master' of github.com:elalish/manifold into miscfix

a5a9099

elalish mentioned this pull request Oct 20, 2024

Odd failure creating and unioning multiple cylinders #996

Closed

elalish added 2 commits October 19, 2024 21:02

change error reporting

a8af0c2

fixed error reporting

d04b754

elalish commented Oct 20, 2024

View reviewed changes

fix compile

985bb2d

elalish requested a review from pca006132 October 20, 2024 04:39

addressing feedback

f837cb9

elalish merged commit e647093 into master Oct 21, 2024
19 checks passed

elalish deleted the miscfix branch October 21, 2024 04:48

starseeker pushed a commit to starseeker/manifold that referenced this pull request Oct 21, 2024

removed RotateUp and uint (elalish#1000)

47cb328

* removed RotateUp and uint * avoid new API * simplify * change error reporting * fixed error reporting * fix compile * addressing feedback

elalish mentioned this pull request Nov 5, 2024

v3.0 #1024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removed RotateUp and uint #1000

removed RotateUp and uint #1000

elalish commented Oct 19, 2024

elalish Oct 19, 2024

pca006132 Oct 19, 2024

elalish Oct 19, 2024

elalish commented Oct 19, 2024

starseeker commented Oct 19, 2024

starseeker commented Oct 19, 2024

starseeker commented Oct 19, 2024

elalish commented Oct 20, 2024

elalish Oct 20, 2024

elalish Oct 20, 2024

pca006132 Oct 20, 2024

elalish Oct 20, 2024

pca006132 Oct 20, 2024

starseeker Oct 20, 2024

elalish Oct 20, 2024

elalish commented Oct 20, 2024

pca006132 commented Oct 20, 2024

removed RotateUp and uint #1000

removed RotateUp and uint #1000

Conversation

elalish commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elalish commented Oct 19, 2024

starseeker commented Oct 19, 2024

starseeker commented Oct 19, 2024

starseeker commented Oct 19, 2024

elalish commented Oct 20, 2024

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

elalish commented Oct 20, 2024

pca006132 commented Oct 20, 2024