Skip to content

Mini Changes#18

Merged
luisfonsivevo merged 7 commits intoBorgerLand:masterfrom
JaminKoke:mini_changes
Jan 6, 2026
Merged

Mini Changes#18
luisfonsivevo merged 7 commits intoBorgerLand:masterfrom
JaminKoke:mini_changes

Conversation

@JaminKoke
Copy link
Copy Markdown
Contributor

A collection of small changes

@luisfonsivevo
Copy link
Copy Markdown
Member

Was this causing a failed test? Looks good either way, after the merge conflict.

I'm not sure if these are super important, but it's probably good to use
these instead of using `cos()` and `sin()`.
This was the logic in the original `impl.cpp` file, but was missed
during the port. This dramatically improves the performance of things
with high vert counts. Most noticeable in the
`Properties.ToleranceSphere` Manifold test
@JaminKoke
Copy link
Copy Markdown
Contributor Author

I should have made note of this in the PR comment, but there are 2 main changes here.

The first, porting sind() and cosd(), I don't know if it actually contributes to making any of the tests pass, but I just thought that since Manifold does it, we might as well too.

Second, other changes such as with the SimpleRecorder struct or Make MeshBool::set_properties() take impl Fn, just clean up a few things to make other PRs cherry-picking things from #7 easier.

@luisfonsivevo
Copy link
Copy Markdown
Member

Got it thanks. I must've falsely assumed that sind() and cosd() were C++ standard library.

@luisfonsivevo luisfonsivevo merged commit 30718e2 into BorgerLand:master Jan 6, 2026
@JaminKoke JaminKoke deleted the mini_changes branch January 6, 2026 21:40
Comment thread src/boolean_result.rs
}

impl<'a> Boolean3<'a> {
pub fn result(self, op: OpType) -> MeshBoolImpl {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just noticed this change here while fixing some merge conflicts for my project. Consuming self was intentional here: Manifold establishes a pattern in this function of freeing memory as early as possible to avoid running out of RAM during very large mesh boolean operations. Was this a tidying task or was something else not working?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We would have to use .clone() here otherwise. It's only thing that that change is used for if I remember. Probably could have been better to put it in #20 so the reason for that change was in the same PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants