Skip to content
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.

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

Cache intermediate results automatically #199

Closed
elalish opened this issue Sep 10, 2022 · 7 comments · Fixed by #206
Closed

Cache intermediate results automatically #199

elalish opened this issue Sep 10, 2022 · 7 comments · Fixed by #206

Comments

@elalish
Copy link
Owner

elalish commented Sep 10, 2022

Regarding this comment @pca006132, I did a little test with ./manifold_test --gtest_filter=Samples.Frame -v before and after removing the two cache-forcing lines. I'm seeing at least double the number of Boolean operations when I remove those lines. Is there a way to make the CsgNode aware of when it's being used more than once so it can skip flattening and allow reuse instead? Maybe some kind of ref-counter?

@pca006132
Copy link
Collaborator

Yes, we are already using ref-counter so maybe we can do use use_count.

@elalish
Copy link
Owner Author

elalish commented Sep 11, 2022

Sounds good; do you want to give it a try?

@pca006132
Copy link
Collaborator

diff --git a/src/manifold/src/csg_tree.cpp b/src/manifold/src/csg_tree.cpp
index 9e31137..d11fb5f 100644
--- a/src/manifold/src/csg_tree.cpp
+++ b/src/manifold/src/csg_tree.cpp
@@ -366,7 +366,7 @@ std::vector<std::shared_ptr<CsgNode>> &CsgOpNode::GetChildren(
 
   CsgNodeType op = op_;
   for (auto &child : children_) {
-    if (child->GetNodeType() == op) {
+    if (child->GetNodeType() == op && child.use_count() == 1) {
       auto grandchildren =
           std::dynamic_pointer_cast<CsgOpNode>(child)->GetChildren(finalize);
       int start = children_.size();

Indeed it works, but there is still 2 additional boolean operations, not sure what they are, will check later.

@elalish
Copy link
Owner Author

elalish commented Sep 11, 2022

Excellent, I love a simple fix! Looking forward to the PR. 👍

@elalish
Copy link
Owner Author

elalish commented Sep 18, 2022

Any more progress on this, or would you like me to take a look?

@pca006132
Copy link
Collaborator

Sorry I am a bit busy recently, maybe you can take a look at it first.

@pca006132
Copy link
Collaborator

pca006132 commented Sep 20, 2022

OK I understand what is going on for that 2 additional boolean operations now. The problem is that we are cloning all children when we do CsgOpNode::Transform, so we kind of lose track of how many copies of the nodes are there. A way to mitigate this is to share the child list.

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 a pull request may close this issue.

2 participants