Fix sparseindices #742

pca006132 · 2024-02-15T07:30:22Z

The changes are everywhere... Basically this PR does two things:

Use size_t for Vec indices when possible.
Catch Vec that are too large (>= INT32_MAX) so other parts that rely on int32 indices will not break.

1 is easy, 2 is hard, and I am pretty sure that there are missing cases or cases that can be relaxed a bit, but still it is better than the status quo.

resolves #720, resolves #738, resolves #740.

Btw @starseeker can you verify that the result is correct? The result is like 3 really long rods that are connected at the end or something, the mesh is extremely slim making it hard to screenshot...

codecov · 2024-02-15T07:40:50Z

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (4395ab6) 91.67% compared to head (d6a972e) 91.50%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
src/utilities/include/sparse.h	58.33%	5 Missing ⚠️
src/utilities/include/hashtable.h	78.94%	4 Missing ⚠️
src/manifold/include/manifold.h	75.00%	2 Missing ⚠️
src/manifold/src/boolean_result.cpp	71.42%	2 Missing ⚠️
src/manifold/src/impl.cpp	84.61%	2 Missing ⚠️
src/manifold/src/boolean3.cpp	95.00%	1 Missing ⚠️
src/utilities/include/par.h	0.00%	1 Missing ⚠️
src/utilities/include/public.h	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #742      +/-   ##
==========================================
- Coverage   91.67%   91.50%   -0.18%     
==========================================
  Files          37       37              
  Lines        4722     4766      +44     
==========================================
+ Hits         4329     4361      +32     
- Misses        393      405      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pca006132 · 2024-02-15T08:19:29Z

~~For some reason the macos craycloudbool case keeps failing, no idea why...~~

OK it is due to undefined behavior: log2(0).

starseeker · 2024-02-15T14:35:22Z

@pca006132 That sounds right - the original inputs were indeed long, super-thin cylinders.

starseeker · 2024-02-15T14:48:36Z

Yes - exporting the mesh and visually inspecting it, it appears to be correct. (For the record, our input meshes were rather messed up in the first place, but technically they were valid manifolds.)

pca006132 · 2024-02-15T23:34:36Z

@elalish btw not sure if we should use the generic std::out_of_range or define a custom exception. I just used that because I am lazy.

elalish · 2024-02-16T01:21:01Z

Good question. I think I only defined custom exceptions for things that are unique to manifold. Out of range sounds reasonable for using the standard.

pca006132 · 2024-02-16T01:36:58Z

one issue though: std::out_of_range is usually what you get when there is a bug, so it is generally hard to recover. However, in our case it means the operation on large meshes is not supported.

elalish · 2024-02-16T05:44:46Z

Fair, though I'm not really sure how you'd recover from a too-large mesh either, besides just cancel everything and post a message to the user.

pca006132 · 2024-02-16T05:56:28Z

The way I view this is similar to rust: if it is a software bug, e.g. the normal index out of bound thing, we should just abort (although we are not that strict now). Otherwise, it is an user error and user can cancel the operation.

elalish · 2024-02-16T05:47:03Z

src/manifold/src/boolean3.cpp

-  for_each_n(autoPolicy(p2q1.size()), zip(countAt(p1q2.size()), countAt(0)),
+  for_each_n(
+      autoPolicy(p1q2.size()),
+      zip(countAt(static_cast<size_t>(0)), countAt(static_cast<size_t>(0))),


Can we just tweak the countAt declaration somehow so we don't need all these static_cast?

Yes if we want to change everything into using size_t. Thinking about it, we can also use 0ll to replace the static_cast. I used to use 0l but it errors on Windows, because long is 32-bits on Windows and we need long long, so I just switched everything into static_cast...

ah, nevermind, https://en.cppreference.com/w/cpp/language/integer_literal size_t suffix is only supported on c++23, we can use something user defined though.

now we can use 0_z where the _z suffix specifies the number is a size_t.

elalish · 2024-02-16T05:52:37Z

src/manifold/src/face_op.cpp

  triVerts.resize(triCount.back());
  triNormal.resize(triCount.back());
  triRef.resize(triCount.back());

  auto processFace2 = std::bind(
-      processFace, [&](int face) { return std::move(results[face]); },
+      processFace, [&](size_t face) { return std::move(results[face]); },


If we aren't allowing more than MAX_INT faces, why do we need to index them with size_t now?

Ah this was to please the compiler, because we count with size_t below. Can change to int and do a cast below.

src/utilities/include/hashtable.h

elalish · 2024-02-16T06:01:41Z

src/utilities/include/sparse.h


-  inline int& Get(int i, bool use_q) {
+  inline int& Get(size_t i, bool use_q) {


These look like the critical changes to fix the bug, right?

yes, but not only here because we need to make sure that code in boolean3.cpp handles this correctly.

test/samples_test.cpp

src/sdf/src/sdf.cpp

elalish · 2024-02-16T06:09:01Z

src/manifold/src/boolean3.cpp

@@ -578,6 +584,14 @@ Boolean3::Boolean3(const Manifold::Impl &inP, const Manifold::Impl &inQ,
      Intersect12(inQ, inP, s20, p2q0, s11, p1q1, z20, xyzz11, p2q1_, false);
  PRINT("x21 size = " << x21_.size());

+  if (p0q2.size() >= std::numeric_limits<int>::max() ||
+      p2q0.size() >= std::numeric_limits<int>::max() ||


Why should 0-2 (vertices shadowing triangles) have to be fewer than INT_MAX? That's still worst case an O(n^2) combination problem. The other four below this are actually contributing verts, so the check makes more sense (though shouldn't it be their sum?).

Indeed, I was thinking that p0/q0 have the same size as p0q2/p2q0, and thought that they affect the size of w03_ and w30_. Will address this.

fixed, need to check if the current one is correct.

This reverts commit adfb86c.

elalish

Thanks, looks like we're close now.

elalish · 2024-02-16T16:02:22Z

src/manifold/src/boolean3.cpp

-  for_each_n(autoPolicy(p0q2.size()), zip(countAt(0), s02.begin(), z02.begin()),
-             p0q2.size(),
+  for_each_n(autoPolicy(p0q2.size()),
+             zip(countAt(0ll), s02.begin(), z02.begin()), p0q2.size(),


Is there a reason this one isn't 0_z?

elalish · 2024-02-16T16:03:42Z

src/manifold/src/boolean3.cpp

@@ -578,6 +578,10 @@ Boolean3::Boolean3(const Manifold::Impl &inP, const Manifold::Impl &inQ,
      Intersect12(inQ, inP, s20, p2q0, s11, p1q1, z20, xyzz11, p2q1_, false);
  PRINT("x21 size = " << x21_.size());

+  if (x12_.size() + x21_.size() >= std::numeric_limits<int>::max() ||
+      w03_.size() + w30_.size() >= std::numeric_limits<int>::max())


Thinking about this, we can probably remove w03 and w30 too: they always just have size of P.NumVert() and Q.NumVert() respectively.

ah, make sense

elalish · 2024-02-16T16:05:24Z

src/sdf/src/sdf.cpp

      2 * maxMorton, static_cast<Uint64>(10 * glm::pow(maxMorton, 0.667)));
  HashTable<GridVert, identity> gridVerts(tableSize);
  Vec<glm::vec3> vertPos(gridVerts.Size() * 7);

  while (1) {
    Vec<int> index(1, 0);
-    for_each_n(pol, countAt(0), maxMorton + 1,
+    for_each_n(pol, countAt(0ul), maxMorton + 1,


elalish · 2024-02-16T16:08:51Z

src/utilities/include/public.h

@@ -31,6 +31,8 @@
 #include <sstream>
 #endif

+constexpr std::size_t operator"" _z(unsigned long long n) { return n; }


This is cool, but it looks like emscripten doesn't like it?

elalish

Thanks!

* accept size_t for Vec * Prepare a test case for "SparseIndices too large" core dump * fix python docstring deps * fix compilation errors * limit sdf hashtable size * another fix to hashtable * log2(0) is undefined * fix formatting * Revert "fix compilation errors" This reverts commit adfb86c. * fix size_t stuff * remove processOverlap setting in test * fix hashtable * vertex number limit * address comments

pca006132 added 3 commits February 15, 2024 15:21

accept size_t for Vec

ee2e2e8

Prepare a test case for "SparseIndices too large" core dump

8c74e6f

fix python docstring deps

6f7ce43

pca006132 requested a review from elalish February 15, 2024 07:30

pca006132 added 3 commits February 15, 2024 15:42

fix compilation errors

adfb86c

limit sdf hashtable size

b224f96

another fix to hashtable

3dfeba3

pca006132 added 2 commits February 15, 2024 16:35

log2(0) is undefined

86de1fd

fix formatting

a700856

elalish requested changes Feb 16, 2024

View reviewed changes

pca006132 added 5 commits February 16, 2024 15:42

Revert "fix compilation errors"

126c366

This reverts commit adfb86c.

fix size_t stuff

cb8d202

remove processOverlap setting in test

fec0dbc

fix hashtable

e458f86

vertex number limit

fb27914

elalish requested changes Feb 16, 2024

View reviewed changes

address comments

d6a972e

elalish approved these changes Feb 16, 2024

View reviewed changes

elalish merged commit d59b9b9 into elalish:master Feb 16, 2024
18 checks passed

pca006132 deleted the fix-sparseindices branch February 18, 2024 12:06

cartesian-theatrics mentioned this pull request Apr 6, 2024

Vec out of Range #784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sparseindices #742

Fix sparseindices #742

pca006132 commented Feb 15, 2024 •

edited

Loading

codecov bot commented Feb 15, 2024 •

edited

Loading

pca006132 commented Feb 15, 2024 •

edited

Loading

starseeker commented Feb 15, 2024

starseeker commented Feb 15, 2024 •

edited

Loading

pca006132 commented Feb 15, 2024

elalish commented Feb 16, 2024

pca006132 commented Feb 16, 2024

elalish commented Feb 16, 2024

pca006132 commented Feb 16, 2024

elalish Feb 16, 2024

pca006132 Feb 16, 2024

pca006132 Feb 16, 2024

pca006132 Feb 16, 2024

elalish Feb 16, 2024

pca006132 Feb 16, 2024

elalish Feb 16, 2024

pca006132 Feb 16, 2024

elalish Feb 16, 2024

pca006132 Feb 16, 2024

pca006132 Feb 16, 2024

elalish left a comment

elalish Feb 16, 2024

elalish Feb 16, 2024

pca006132 Feb 16, 2024

elalish Feb 16, 2024

elalish Feb 16, 2024

elalish left a comment


		inline int& Get(int i, bool use_q) {
		inline int& Get(size_t i, bool use_q) {

Fix sparseindices #742

Fix sparseindices #742

Conversation

pca006132 commented Feb 15, 2024 • edited Loading

codecov bot commented Feb 15, 2024 • edited Loading

Codecov Report

pca006132 commented Feb 15, 2024 • edited Loading

starseeker commented Feb 15, 2024

starseeker commented Feb 15, 2024 • edited Loading

pca006132 commented Feb 15, 2024

elalish commented Feb 16, 2024

pca006132 commented Feb 16, 2024

elalish commented Feb 16, 2024

pca006132 commented Feb 16, 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

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 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

elalish left a comment

Choose a reason for hiding this comment

pca006132 commented Feb 15, 2024 •

edited

Loading

codecov bot commented Feb 15, 2024 •

edited

Loading

pca006132 commented Feb 15, 2024 •

edited

Loading

starseeker commented Feb 15, 2024 •

edited

Loading