chores #833

pca006132 · 2024-06-08T17:33:29Z

@elalish When doing this change, I found that we are using 32-bit integers extensively. I feel like the proper way would be to use 64-bit integers. There may be some performance hit, but I doubt if it will be huge. And it just feels better if we can make things work with larger meshes, considering we don't have any reasons not to do so? Some public APIs will have to be changed, but they will be changed when we update to double precision floating point.

For #804, it should now build fine, but it is not the best way of handling exceptions. We need more careful checks everywhere, probably together with #744.

…t narrowing

pca006132 · 2024-06-08T18:40:48Z

probably messed up the python binding somewhere, will fix tmr.

elalish

Thanks, this looks great. Given the breaking changes, what do you say we hold this for ~week while we release v2.5, then put this in for v3.0 with the switch to doubles and all?

src/manifold/include/manifold.h

src/utilities/include/vec_view.h

pca006132 · 2024-06-10T11:38:38Z

and for the record, yes it is fine to hold this for a week and wait for the next release.

codecov · 2024-06-20T07:18:35Z

Codecov Report

Attention: Patch coverage is 91.66667% with 13 lines in your changes missing coverage. Please review.

Project coverage is 89.95%. Comparing base (d437097) to head (84ba93c).
Report is 55 commits behind head on master.

Files	Patch %	Lines
bindings/c/conv.cpp	0.00%	3 Missing ⚠️
bindings/c/manifoldc.cpp	50.00%	2 Missing ⚠️
src/collider/include/collider.h	0.00%	2 Missing ⚠️
src/manifold/src/impl.h	60.00%	2 Missing ⚠️
src/manifold/src/properties.cpp	84.61%	2 Missing ⚠️
bindings/c/include/conv.h	0.00%	1 Missing ⚠️
src/manifold/src/subdivision.cpp	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #833      +/-   ##
==========================================
- Coverage   91.84%   89.95%   -1.89%     
==========================================
  Files          37       64      +27     
  Lines        4976     9067    +4091     
  Branches        0      948     +948     
==========================================
+ Hits         4570     8156    +3586     
- Misses        406      911     +505

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

pca006132 · 2024-06-20T07:25:13Z

Converting everything to size_t turns out to be quite complicated, mainly due to SparseIndices and Collider. Probably need some more time to do it, and evaluate the performance differences. This can be merged even though the changes are incomplete.

pca006132 · 2024-06-20T07:26:17Z

@elalish note that the old ASSERT is now DEBUG_ASSERT, and ASSERT is for things that should still be checked when MANIFOLD_DEBUG is disabled.

elalish

Looks great, thanks for all this! Just noticed a few small things.

elalish · 2024-06-20T18:05:48Z

bindings/python/manifold3d.cpp

@@ -68,7 +68,7 @@ struct nb::detail::type_caster<glm::vec<N, T, Q>> {
    int size = PyObject_Size(src.ptr());  // negative on failure
    if (size != N) return false;
    make_caster<T> t_cast;
-    for (size_t i = 0; i < size; i++) {
+    for (int i = 0; i < size; i++) {


seems backwards?

Ah, should fix this. Thanks.

flake.nix

src/cross_section/src/cross_section.cpp

elalish · 2024-06-20T18:17:21Z

src/manifold/include/manifold.h

-    if (vertProperties.size() / numProp >= std::numeric_limits<int>::max())
-      throw std::out_of_range("mesh too large");
+    ASSERT(vertProperties.size() / numProp <
+               static_cast<size_t>(std::numeric_limits<unsigned int>::max()),


Why unsigned int max? I was thinking it would be either int max or something 64-bit, once Collider gets updated.

I forgot why, need to check. Probably due to MeshGL indices are/should be unsigned integers?

elalish · 2024-06-20T18:22:17Z

src/manifold/src/boolean3.cpp

@@ -137,7 +139,7 @@ inline thrust::pair<int, glm::vec2> Shadow01(

 // https://github.com/scandum/binary_search/blob/master/README.md
 // much faster than standard binary search on large arrays
-size_t monobound_quaternary_search(VecView<const int64_t> array, int64_t key) {
+ssize_t monobound_quaternary_search(VecView<const int64_t> array, int64_t key) {


what is ssize_t? Okay, I found this and am slightly more confused.

ssize_t is signed version of size_t. But thinking about it, we only need that -1 to signal failure, and I can do it with size_t max as well.

src/polygon/src/polygon.cpp

elalish · 2024-06-20T18:34:28Z

src/utilities/include/utils.h

@@ -18,6 +18,11 @@
 #include <mutex>
 #include <unordered_map>

+#if defined(_MSC_VER)
+#include <BaseTsd.h>
+typedef SSIZE_T ssize_t;


This makes me slightly worried about ssize_t...

yeah, thinking about it ssize_t is not really needed.

elalish · 2024-06-20T18:37:00Z

src/utilities/include/vec.h

 #include <exception>
+#endif


Do we need to include this per file, or can it just be wherever we define ASSERT?

yeah probably just need it when we define ASSERT

elalish · 2024-06-20T18:37:12Z

src/utilities/include/vec_view.h

 #include <stdexcept>
+#endif


same as above.

elalish · 2024-06-20T18:47:27Z

.github/workflows/manifold.yml

@@ -107,6 +107,9 @@ jobs:
    timeout-minutes: 30
    runs-on: ubuntu-22.04
    if: github.event.pull_request.draft == false
+    strategy:
+      matrix:
+        exception: [ON, OFF]


Is there any reason to build WASM with exceptions off? Maybe better to test one of the nix builds since they're fast?

If testing generally exceptions off is the test. then it could be linux.

@fire I think your earlier comment mentioned you do build WASM with exceptions off - that requires special CMake instructions which I was considering removing since JS is generally exceptions-based. But if WASM sans exceptions is actually useful to someone, then @pca006132 has it set up properly now. Do you build for WASM without exceptions?

I would expect to build Godot Engine WASM without exceptions since the Godot Engine editor and Godot Engine game templates build without exceptions in the default configuration on all platforms.

Thanks! Okay, let's leave it as is in this PR then.

pca006132 · 2024-06-27T12:07:43Z

sorry, was having a headache today. Will update this tmr

pca006132 · 2024-06-28T04:37:43Z

I also updated nanobind version.

pca006132 · 2024-06-28T05:02:13Z

there are still a lot of uses of integer that should be fixed, but we should probably leave to another PR and unblock your iterator changes.

elalish

Thanks, this looks great!

pca006132 added 4 commits June 9, 2024 01:29

fix comparison between integers with different signedness and implici…

ea5dcf7

…t narrowing

fix compilation error and formatting

0ad9657

allow build with exception disabled

5871752

CI for building without exception

df00c1b

pca006132 changed the title ~~fix comparison between integers with different signedness and implicit narrowing~~ chores Jun 8, 2024

pca006132 added 7 commits June 9, 2024 02:07

format

1f748ad

another fix

d1edf3b

msvc stuff

f337243

fix python bindings

fd014b6

update nix

94bfe23

formatting

b33f8d8

old llvm somehow likes this better

dca5365

elalish reviewed Jun 8, 2024

View reviewed changes

src/manifold/include/manifold.h Outdated Show resolved Hide resolved

src/utilities/include/vec_view.h Outdated Show resolved Hide resolved

pca006132 added 9 commits June 20, 2024 12:55

fix bug

a16b20b

Merge remote-tracking branch 'upstream/master' into shut-up

b4bddf5

rename ASSERT into DEBUG_ASSERT

97d91a4

remove unused file

9620c59

use ASSERT and remove obsolete limits

32e4e80

polygon: change to size_t

86e2b5d

format

486a6b0

fix sign compare

1988a67

install optional_assert.h

68681a4

elalish requested changes Jun 20, 2024

View reviewed changes

elalish mentioned this pull request Jun 25, 2024

Remove fancy iterators #852

Merged

pca006132 added 5 commits June 28, 2024 10:39

address comments

a8591b6

Merge remote-tracking branch 'upstream/master' into shut-up

8648a57

fix

a446e0d

fix

80b83fd

fix

eeefbea

fix format

84ba93c

elalish approved these changes Jun 28, 2024

View reviewed changes

elalish merged commit b817d07 into elalish:master Jun 28, 2024
21 checks passed

elalish mentioned this pull request Jul 13, 2024

Build without exceptions #804

Closed

elalish mentioned this pull request Nov 5, 2024

v3.0 #1024

Merged

pca006132 deleted the shut-up branch November 16, 2024 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chores #833

chores #833

pca006132 commented Jun 8, 2024 •

edited

Loading

pca006132 commented Jun 8, 2024

elalish left a comment

pca006132 commented Jun 10, 2024

codecov bot commented Jun 20, 2024 •

edited

Loading

pca006132 commented Jun 20, 2024

pca006132 commented Jun 20, 2024

elalish left a comment

elalish Jun 20, 2024

pca006132 Jun 21, 2024

elalish Jun 20, 2024

pca006132 Jun 21, 2024

elalish Jun 20, 2024

pca006132 Jun 21, 2024

elalish Jun 20, 2024

pca006132 Jun 21, 2024

elalish Jun 20, 2024

pca006132 Jun 21, 2024

elalish Jun 20, 2024

elalish Jun 20, 2024

fire Jun 20, 2024 •

edited

Loading

elalish Jun 20, 2024

fire Jun 20, 2024

elalish Jun 20, 2024

pca006132 commented Jun 27, 2024

pca006132 commented Jun 28, 2024

pca006132 commented Jun 28, 2024

elalish left a comment

chores #833

chores #833

Conversation

pca006132 commented Jun 8, 2024 • edited Loading

pca006132 commented Jun 8, 2024

elalish left a comment

Choose a reason for hiding this comment

pca006132 commented Jun 10, 2024

codecov bot commented Jun 20, 2024 • edited Loading

Codecov Report

pca006132 commented Jun 20, 2024

pca006132 commented Jun 20, 2024

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

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

fire Jun 20, 2024 • edited Loading

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 Jun 27, 2024

pca006132 commented Jun 28, 2024

pca006132 commented Jun 28, 2024

elalish left a comment

Choose a reason for hiding this comment

pca006132 commented Jun 8, 2024 •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading

fire Jun 20, 2024 •

edited

Loading