8000 Remove triProperties by elalish · Pull Request #1232 · elalish/manifold · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove triProperties #1232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 34 commits into from
Apr 28, 2025
Merged

Remove triProperties #1232

merged 34 commits into from
Apr 28, 2025

Conversation

elalish
Copy link
Owner
@elalish elalish commented Apr 20, 2025 •
edited
Loading

Finally finished this refactor - been meaning to deal with this for ages. Also did a bunch of renaming and various other cleanup.

@elalish elalish self-assigned this Apr 20, 2025
@codecov Codecov
Copy link
codecov bot commented Apr 22, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 95.16129% with 18 lines in your changes missing coverage. Please review.

Project coverage is 91.97%. Comparing base (f3a6a47) to head (71287f1).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/edge_op.cpp 65.21% 8 Missing ⚠️
src/sort.cpp 80.95% 4 Missing ⚠️
src/manifold.cpp 88.46% 3 Missing ⚠️
src/smoothing.cpp 97.82% 2 Missing ⚠️
src/csg_tree.cpp 91.66% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1232      +/-   ##
==========================================
- Coverage   92.18%   91.97%   -0.22%     
==========================================
  Files          31       31              
  Lines        6000     5955      -45     
==========================================
- Hits         5531     5477      -54     
- Misses        469      478       +9

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@elalish elalish marked this pull request as ready for review April 25, 2025 09:52
@elalish elalish requested a review from pca006132 April 25, 2025 09:52
@elalish
Copy link
Owner Author
elalish commented Apr 25, 2025

@pca006132 Okay, I have this working in that it's passing all the tests, which means I can now start removing triProperties. However, both the CI and my local machine fail on the WASM tests with a heap memory corruption. Do you have any idea what might be happening?

@elalish elalish mentioned this pull request Apr 25, 2025
Merged
@pca006132
Copy link
Collaborator

It seems that:

[==========] Running 276 tests from 11 test suites.
[----------] Global test environment set-up.
[----------] 21 tests from Properties
[ RUN      ] Properties.Measurements
[       OK ] Properties.Measurements (4 ms)
[ RUN      ] Properties.Epsilon
[       OK ] Properties.Epsilon (1 ms)
[ RUN      ] Properties.Epsilon2
[       OK ] Properties.Epsilon2 (0 ms)
[ RUN      ] Properties.Tolerance
[       OK ] Properties.Tolerance (46 ms)
[ RUN      ] Properties.ToleranceSphere
=================================================================
==42==ERROR: AddressSanitizer: out of memory: allocator is trying to allocate 0x2dc6bfd0 bytes
    #0 0x2b8bb0 in malloc+0x2b8bb0 (/home/pca006132/code/manifold/build2/test/manifold_test.js+0x2b8bb0)
    #1 0xeccb4 in manifold::Manifold::Impl::Impl(manifold::Manifold::Impl const&)+0xeccb4 (/home/pca006132/code/manifold/build2/test/manifold_test.js+0xeccb4)
    #2 0x1415d7 in manifold::Manifold::SetTolerance(double) const+0x1415d7 (/home/pca006132/code/manifold/build2/test/manifold_test.js+0x1415d7)
    #3 0x2ad4e in Properties_ToleranceSphere_Test::TestBody()+0x2ad4e (/home/pca006132/code/manifold/build2/test/manifold_test.js+0x2ad4e)
    #4 0x8000189d  (JavaScript+0x189d)
    #5 0x2036c7 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x2036c7 (/home/pca006132/code/manifold/build2/test/manifold_test.js+0x2036c7)
    #6 0x203391 in testing::Test::Run()+0x203391 (/home/pca006132/code/manifold/build2/test/manifold_test.js+0x203391)

==42==HINT: if you don't care about these errors you may set allocator_may_return_null=1
SUMMARY: AddressSanitizer: out-of-memory (/home/pca006132/code/manifold/build2/test/manifold_test.js+0x2b8bac) in malloc+0x2b8bac
==42==ABORTING

Tolerance sphere is running out of memory, probably the allocator just returns null.

Reducing n from 1000 to 500 fixes the issue. I guess we were already close to running out of memory previously with this test, but with the additional propVert field we shoot through the 4GB limit?

@elalish elalish merged commit 54baa62 into master Apr 28, 2025
27 checks passed
@elalish elalish deleted the removeTriProp branch April 28, 2025 10:14
@elalish elalish mentioned this pull request May 14, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pca006132 pca006132 Awaiting requested review from pca006132

Assignees

@elalish elalish

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@elalish @pca006132
0