Replace GLM with linalg #984

elalish · 2024-10-11T22:00:39Z

Fixes #976
Related to #962

Here I'm removing our GLM dependency in favor of including a forked version of linalg directly, which is much smaller and simpler.

A question: should we have a follow-on that converts our public API from linalg::vec to std::array so that the linalg.h header is internal instead of public? linalg already provides these implicit conversions (through a copy).

elalish · 2024-10-11T22:01:33Z

include/manifold/linalg.h

+struct std_isfinite {
+  template <class A>
+  auto operator()(A a) const -> decltype(std::isfinite(a)) {
+    return std::isfinite(a);


I added a couple of functions that we commonly use.

include/manifold/linalg.h

elalish · 2024-10-11T22:12:02Z

I've got 80% done I think. Most of the remaining errors will be cleared by taking care of the comments above, and then I've just got a few more specialized math functions to rename and/or implement.

pca006132 · 2024-10-12T05:41:24Z

A question: should we have a follow-on that converts our public API from linalg::vec to std::array so that the linalg.h header is internal instead of public? linalg already provides these implicit conversions (through a copy).

Probably not needed, considering the header is included here already and it is small. Even if we return std::array, users still have to do conversion to their own math library data type, which is the same as exposing linalg types.

elalish · 2024-10-12T06:06:19Z

Okay, still some work to do to get this compiling, but it's pretty close.

elalish · 2024-10-12T06:07:41Z

include/manifold/common.h

+using mat2x3 = la::mat<double, 2, 3>;
+using mat3 = la::mat<double, 3, 3>;
+using mat4x3 = la::mat<double, 4, 3>;
+using mat3x4 = la::mat<double, 3, 4>;


I fixed our matrix naming from the stupid backwards way GLM had it.

elalish · 2024-10-12T06:08:34Z

include/manifold/common.h

+inline double degrees(double a) { return a * 180 / kPi; }
+
+inline mat3x4 Identity3x4() { return mat3x4(mat3(la::identity), vec3(0.0)); }
+inline mat2x3 Identity2x3() { return mat2x3(mat2(la::identity), vec2(0.0)); }


linalg doesn't do non-square identities like GLM did.

src/utils.h

elalish · 2024-10-12T06:12:02Z

include/manifold/common.h

 ///@}

+constexpr double kPi = 3.14159265358979323846264338327950288;
+constexpr double kTwoPi = 6.28318530717958647692528676655900576;
+constexpr double kHalfPi = 1.57079632679489661923132169163975144;


does this seem like a reasonable place to define these?

I think cmath should have M_PI and M_PI_2 defined, but anyway this should be fine.

Oof, well an hour later I know understand painfully well to never use M_PI.

elalish · 2024-10-13T05:36:26Z

Okay, I've got it compiling and running now (locally at least). Some tests even pass! I definitely have some math errors to track down. @pca006132 if you have a chance to look, it would be great to have another set of eyes on this. I bet the problems are mostly just typos. Also the CI is suddenly more annoyed about size_t -> int conversions.

pca006132

I think for the narrowing warnings, we can just disable them for now. We do so many narrowing operations that it is not worth it to mark them explicitly each time.

pca006132 · 2024-10-13T13:08:22Z

include/manifold/common.h

-using ivec3 = glm::vec<3, int>;
-using ivec4 = glm::vec<4, int>;
-using quat = glm::dquat;
+namespace la = linalg;


Not sure if we want this namespace alias in our public header. Maybe define this in our internal header?

It seems that this is fine, we are having la alias inside our manifold namespace, so this will not cause conflict elsewhere.

pca006132 · 2024-10-13T13:17:27Z

include/manifold/common.h

 ///@}

+constexpr double kPi = 3.14159265358979323846264338327950288;
+constexpr double kTwoPi = 6.28318530717958647692528676655900576;
+constexpr double kHalfPi = 1.57079632679489661923132169163975144;


I think cmath should have M_PI and M_PI_2 defined, but anyway this should be fine.

pca006132 · 2024-10-13T13:17:50Z

include/manifold/common.h

+constexpr double kTwoPi = 6.28318530717958647692528676655900576;
+constexpr double kHalfPi = 1.57079632679489661923132169163975144;
+
+inline double radians(double a) { return a * kPi / 180; }


constexpr?

src/csg_tree.cpp

pca006132 · 2024-10-15T05:43:00Z

src/csg_tree.cpp

@@ -111,29 +111,29 @@ std::shared_ptr<CsgNode> CsgNode::Boolean(
 }

 std::shared_ptr<CsgNode> CsgNode::Translate(const vec3 &t) const {
-  mat4x3 transform(1.0);
+  mat3x4 transform(1.0);


This is not the identity matrix, this is a matrix with all entries = 1.

Good catch - I had fixed a bunch of those, but must have missed this one.

pca006132 · 2024-10-15T07:25:19Z

CMakeLists.txt

      -Wno-unused
-      -Wno-array-bounds


I think those are left from the thrust era.

pca006132 · 2024-10-15T07:27:18Z

test/test_main.cpp

-                          inMesh.triVerts[3 * inTri + 1],
-                          inMesh.triVerts[3 * inTri + 2]};
-      inTriangle *= inMesh.numProp;
+      ivec3 inTriangle(inMesh.triVerts[3 * inTri],


Apparently, the compiler is more picky when it performs implicit conversion when putting things inside the implicit constructor. Maybe the more implicit it gets, the more likely it is an issue...

pca006132 · 2024-10-15T15:16:09Z

Windows users wanted! Can someone using Windows check if this PR is faster/slower comparing with the current master, when running extras/perfTest and test/manifold_test.

In addition, try the following patch and see if it affects anything:

diff --git a/src/polygon.cpp b/src/polygon.cpp
index 8f8fcdaa..6625288f 100644
--- a/src/polygon.cpp
+++ b/src/polygon.cpp
@@ -30,11 +30,6 @@ static ExecutionParams params;
 
 constexpr double kBest = -std::numeric_limits<double>::infinity();
 
-// it seems that MSVC cannot optimize la::determinant(mat2(a, b))
-constexpr double determinant2x2(vec2 a, vec2 b) {
-  return a.x * b.y - a.y * b.x;
-}
-
 #ifdef MANIFOLD_DEBUG
 struct PolyEdge {
   int startVert, endVert;
@@ -180,7 +175,7 @@ bool IsConvex(const PolygonsIdx &polys, double precision) {
     for (size_t v = 0; v < poly.size(); ++v) {
       const vec2 edge =
           v + 1 < poly.size() ? poly[v + 1].pos - poly[v].pos : firstEdge;
-      const double det = determinant2x2(lastEdge, edge);
+      const double det = la::determinant(mat2(lastEdge, edge));
       if (det <= 0 ||
           (std::abs(det) < precision && la::dot(lastEdge, edge) < 0))
         return false;
@@ -454,11 +449,11 @@ class EarClip {
     // goes to the outside. No need to check the other side, since all verts are
     // processed in the EarCost loop.
     double SignedDist(VertItr v, vec2 unit, double precision) const {
-      double d = determinant2x2(unit, v->pos - pos);
+      double d = la::determinant(mat2(unit, v->pos - pos));
       if (std::abs(d) < precision) {
-        double dR = determinant2x2(unit, v->right->pos - pos);
+        double dR = la::determinant(mat2(unit, v->right->pos - pos));
         if (std::abs(dR) > precision) return dR;
-        double dL = determinant2x2(unit, v->left->pos - pos);
+        double dL = la::determinant(mat2(unit, v->left->pos - pos));
         if (std::abs(dL) > precision) return dL;
       }
       return d;
@@ -470,7 +465,7 @@ class EarClip {
       double cost = std::min(SignedDist(v, rightDir, precision),
                              SignedDist(v, left->rightDir, precision));
 
-      const double openCost = determinant2x2(openSide, v->pos - right->pos);
+      const double openCost = la::determinant(mat2(openSide, v->pos - right->pos));
       return std::min(cost, openCost);
     }
 
@@ -672,7 +667,7 @@ class EarClip {
     auto AddPoint = [&](VertItr v) {
       bBox.Union(v->pos);
       const double area1 =
-          determinant2x2(v->pos - origin, v->right->pos - origin);
+          la::determinant(mat2(v->pos - origin, v->right->pos - origin));
       const double t1 = area + area1;
       areaCompensation += (area - t1) + area1;
       area = t1;

elalish

Thanks for debugging this for me, not to mention all the cleanup!

elalish · 2024-10-15T16:11:17Z

include/manifold/common.h

@@ -578,5 +578,3 @@ struct ExecutionParams {
 };

 }  // namespace manifold
-
-#undef HOST_DEVICE


elalish · 2024-10-15T18:21:53Z

I like the idea of simplifying the determinant above - let's do that as a follow-up, once we have assurance from Windows that it's not slower.

fire · 2024-10-15T18:32:32Z

Horray!

pca006132 · 2024-10-16T14:55:50Z

Windows users wanted! Can someone using Windows check if this PR is faster/slower comparing with the current master, when running extras/perfTest and test/manifold_test.

In addition, try the following patch and see if it affects anything:

diff --git a/src/polygon.cpp b/src/polygon.cpp
index 8f8fcdaa..6625288f 100644
--- a/src/polygon.cpp
+++ b/src/polygon.cpp
@@ -30,11 +30,6 @@ static ExecutionParams params;
 
 constexpr double kBest = -std::numeric_limits<double>::infinity();
 
-// it seems that MSVC cannot optimize la::determinant(mat2(a, b))
-constexpr double determinant2x2(vec2 a, vec2 b) {
-  return a.x * b.y - a.y * b.x;
-}
-
 #ifdef MANIFOLD_DEBUG
 struct PolyEdge {
   int startVert, endVert;
@@ -180,7 +175,7 @@ bool IsConvex(const PolygonsIdx &polys, double precision) {
     for (size_t v = 0; v < poly.size(); ++v) {
       const vec2 edge =
           v + 1 < poly.size() ? poly[v + 1].pos - poly[v].pos : firstEdge;
-      const double det = determinant2x2(lastEdge, edge);
+      const double det = la::determinant(mat2(lastEdge, edge));
       if (det <= 0 ||
           (std::abs(det) < precision && la::dot(lastEdge, edge) < 0))
         return false;
@@ -454,11 +449,11 @@ class EarClip {
     // goes to the outside. No need to check the other side, since all verts are
     // processed in the EarCost loop.
     double SignedDist(VertItr v, vec2 unit, double precision) const {
-      double d = determinant2x2(unit, v->pos - pos);
+      double d = la::determinant(mat2(unit, v->pos - pos));
       if (std::abs(d) < precision) {
-        double dR = determinant2x2(unit, v->right->pos - pos);
+        double dR = la::determinant(mat2(unit, v->right->pos - pos));
         if (std::abs(dR) > precision) return dR;
-        double dL = determinant2x2(unit, v->left->pos - pos);
+        double dL = la::determinant(mat2(unit, v->left->pos - pos));
         if (std::abs(dL) > precision) return dL;
       }
       return d;
@@ -470,7 +465,7 @@ class EarClip {
       double cost = std::min(SignedDist(v, rightDir, precision),
                              SignedDist(v, left->rightDir, precision));
 
-      const double openCost = determinant2x2(openSide, v->pos - right->pos);
+      const double openCost = la::determinant(mat2(openSide, v->pos - right->pos));
       return std::min(cost, openCost);
     }
 
@@ -672,7 +667,7 @@ class EarClip {
     auto AddPoint = [&](VertItr v) {
       bBox.Union(v->pos);
       const double area1 =
-          determinant2x2(v->pos - origin, v->right->pos - origin);
+          la::determinant(mat2(v->pos - origin, v->right->pos - origin));
       const double t1 = area + area1;
       areaCompensation += (area - t1) + area1;
       area = t1;

@starseeker do you have windows machines around for benchmark? If yes, can you try this and see if there is any difference in performance?

starseeker · 2024-10-16T15:13:20Z

@starseeker do you have windows machines around for benchmark? If yes, can you try this and see if there is any difference in performance?

I do, but it will take some time to get set up on on it.

starseeker · 2024-10-16T20:56:44Z

Without patch:

.\perfTest.exe
nTri = 512, time = 0.002399 sec
nTri = 2048, time = 0.0057338 sec
nTri = 8192, time = 0.0191217 sec
nTri = 32768, time = 0.0676792 sec
nTri = 131072, time = 0.256196 sec
nTri = 524288, time = 1.04938 sec

.\largeSceneTest.exe
n = 20
nTri = 91814, time = 24.9764 sec

With patch:

.\perfTest.exe
nTri = 512, time = 0.0020762 sec
nTri = 2048, time = 0.0060967 sec
nTri = 8192, time = 0.0194322 sec
nTri = 32768, time = 0.0685545 sec
nTri = 131072, time = 0.265982 sec
nTri = 524288, time = 1.10732 sec
nTri = 2097152, time = 4.72829 sec

.\largeSceneTest.exe
n = 20
nTri = 91814, time = 23.7445 sec

elalish · 2024-10-16T21:31:54Z

Okay, looks like we should apply the patch then, thanks! 👍

pca006132 · 2024-10-17T02:32:18Z

The weird thing though is that the performance difference is pretty large and inconsistent, maybe we should try multiple runs and account for deviations, and let the system rest before running another run because the CPU may be hotter for example. Alternatively, we may want to run these single threaded with turboboost disabled to avoid deviation due to CPU states.

E.g. for nTri = 524288, the patch made it slower by 4.5%, and pretty consistently slower for other sphere sizes. For largeSceneTest however, it is somehow 5% faster.

* added linalg * removed GLM * fix matrix multiplication * more matrix constructors * fixed more functions * fixed matrix notation * more fixes * fix ostream * further fixes * added more rotation functions * compiles * some fixes * fix2 * fix build * another warning... * fix install * for wasm? * no warning for unknown options * proper fix? * remove legacy warning flags * format * misc constexpr --------- Co-authored-by: pca006132 <john.lck40@gmail.com>

elalish added 2 commits October 11, 2024 12:33

added linalg

a4e12c3

removed GLM

cbda76e

elalish self-assigned this Oct 11, 2024

elalish commented Oct 11, 2024

View reviewed changes

elalish added 4 commits October 11, 2024 20:08

fix matrix multiplication

69b808a

more matrix constructors

71c8c20

fixed more functions

268b467

fixed matrix notation

79a7b00

elalish added 3 commits October 11, 2024 22:43

more fixes

a5bacb9

fix ostream

27017b8

further fixes

4a07da3

elalish commented Oct 12, 2024

View reviewed changes

elalish added 2 commits October 12, 2024 21:37

added more rotation functions

2b7a786

compiles

66fa688

elalish requested a review from pca006132 October 13, 2024 05:32

pca006132 reviewed Oct 13, 2024

View reviewed changes

pca006132 reviewed Oct 15, 2024

View reviewed changes

pca006132 added 9 commits October 15, 2024 13:55

some fixes

ec9e54b

fix2

3392b97

fix build

a27eba8

another warning...

e55c188

fix install

8048c67

for wasm?

44ee21b

no warning for unknown options

43551db

proper fix?

12a103f

remove legacy warning flags

547c63e

format

0454c63

pca006132 reviewed Oct 15, 2024

View reviewed changes

misc constexpr

7923149

elalish commented Oct 15, 2024

View reviewed changes

include/manifold/common.h

@@ -578,5 +578,3 @@ struct ExecutionParams {

};

} // namespace manifold

#undef HOST_DEVICE

Copy link

Owner Author

elalish Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

elalish changed the title ~~[WIP] Replace GLM with linalg~~ Replace GLM with linalg Oct 15, 2024

elalish force-pushed the linalg branch from 8f82837 to 7923149 Compare October 15, 2024 18:00

merging master

8f7ccca

elalish merged commit e067653 into master Oct 15, 2024
19 checks passed

elalish deleted the linalg branch October 15, 2024 18:22

fire mentioned this pull request Oct 15, 2024

Fix mesh corruption of CSG by using elalish/manifold godotengine/godot#94321

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace GLM with linalg #984

Replace GLM with linalg #984

elalish commented Oct 11, 2024 •

edited

Loading

elalish Oct 11, 2024

elalish commented Oct 11, 2024

pca006132 commented Oct 12, 2024

elalish commented Oct 12, 2024

elalish Oct 12, 2024

elalish Oct 12, 2024

elalish Oct 12, 2024

pca006132 Oct 13, 2024

elalish Oct 15, 2024

elalish commented Oct 13, 2024

pca006132 left a comment

pca006132 Oct 13, 2024

pca006132 Oct 15, 2024

pca006132 Oct 13, 2024

pca006132 Oct 13, 2024

pca006132 Oct 15, 2024

elalish Oct 15, 2024

pca006132 Oct 15, 2024

pca006132 Oct 15, 2024

pca006132 commented Oct 15, 2024

elalish left a comment

elalish Oct 15, 2024

elalish commented Oct 15, 2024

fire commented Oct 15, 2024

pca006132 commented Oct 16, 2024

starseeker commented Oct 16, 2024

starseeker commented Oct 16, 2024

elalish commented Oct 16, 2024

pca006132 commented Oct 17, 2024

Replace GLM with linalg #984

Replace GLM with linalg #984

Conversation

elalish commented Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

elalish commented Oct 11, 2024

pca006132 commented Oct 12, 2024

elalish commented Oct 12, 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

elalish commented Oct 13, 2024

pca006132 left a 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

pca006132 commented Oct 15, 2024

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elalish commented Oct 15, 2024

fire commented Oct 15, 2024

pca006132 commented Oct 16, 2024

starseeker commented Oct 16, 2024

starseeker commented Oct 16, 2024

elalish commented Oct 16, 2024

pca006132 commented Oct 17, 2024

elalish commented Oct 11, 2024 •

edited

Loading