Skip to content

Commit

Permalink
fixed csg tree cache concurrency bug (#356)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pca006132
pca006132 committed Mar 9, 2023
1 parent f6ff8c3 commit 9f8a6d4
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 32 deletions.
51 changes: 30 additions & 21 deletions src/manifold/src/csg_tree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,17 +223,19 @@ CsgOpNode::CsgOpNode() {}

CsgOpNode::CsgOpNode(const std::vector<std::shared_ptr<CsgNode>> &children,
OpType op)
: impl_(std::make_shared<Impl>()) {
impl_->children_ = children;
: impl_(Impl{}) {
auto impl = impl_.GetGuard();
impl->children_ = children;
SetOp(op);
// opportunistically flatten the tree without costly evaluation
GetChildren(false);
}

CsgOpNode::CsgOpNode(std::vector<std::shared_ptr<CsgNode>> &&children,
OpType op)
: impl_(std::make_shared<Impl>()) {
impl_->children_ = children;
: impl_(Impl{}) {
auto impl = impl_.GetGuard();
impl->children_ = children;
SetOp(op);
// opportunistically flatten the tree without costly evaluation
GetChildren(false);
Expand All @@ -243,27 +245,30 @@ std::shared_ptr<CsgNode> CsgOpNode::Transform(const glm::mat4x3 &m) const {
auto node = std::make_shared<CsgOpNode>();
node->impl_ = impl_;
node->transform_ = m * glm::mat4(transform_);
node->op_ = op_;
return node;
}

std::shared_ptr<CsgLeafNode> CsgOpNode::ToLeafNode() const {
if (cache_ != nullptr) return cache_;
if (impl_->children_.empty()) return nullptr;
// turn the children into leaf nodes
GetChildren();
auto &children_ = impl_->children_;
auto impl = impl_.GetGuard();
auto &children_ = impl->children_;
if (children_.size() > 1) {
switch (impl_->op_) {
switch (op_) {
case CsgNodeType::Union:
BatchUnion();
break;
case CsgNodeType::Intersection: {
std::vector<std::shared_ptr<const Manifold::Impl>> impls;
for (auto &child : children_) {
impls.push_back(std::dynamic_pointer_cast<CsgLeafNode>(child)->GetImpl());
impls.push_back(
std::dynamic_pointer_cast<CsgLeafNode>(child)->GetImpl());
}
children_.clear();
children_.push_back(std::make_shared<CsgLeafNode>(BatchBoolean(OpType::Intersect, impls)));
children_.push_back(std::make_shared<CsgLeafNode>(
BatchBoolean(OpType::Intersect, impls)));
break;
};
case CsgNodeType::Difference: {
Expand All @@ -283,6 +288,8 @@ std::shared_ptr<CsgLeafNode> CsgOpNode::ToLeafNode() const {
// unreachable
break;
}
} else if (children_.size() == 0) {
return nullptr;
}
// children_ must contain only one CsgLeafNode now, and its Transform will
// give CsgLeafNode as well
Expand Down Expand Up @@ -345,7 +352,8 @@ void CsgOpNode::BatchUnion() const {
// If the number of children exceeded this limit, we will operate on chunks
// with size kMaxUnionSize.
constexpr int kMaxUnionSize = 1000;
auto &children_ = impl_->children_;
auto impl = impl_.GetGuard();
auto &children_ = impl->children_;
while (children_.size() > 1) {
const int start = (children_.size() > kMaxUnionSize)
? (children_.size() - kMaxUnionSize)
Expand Down Expand Up @@ -392,7 +400,8 @@ void CsgOpNode::BatchUnion() const {
}
}
children_.erase(children_.begin() + start, children_.end());
children_.push_back(std::make_shared<CsgLeafNode>(BatchBoolean(OpType::Add, impls)));
children_.push_back(
std::make_shared<CsgLeafNode>(BatchBoolean(OpType::Add, impls)));
// move it to the front as we process from the back, and the newly added
// child should be quite complicated
std::swap(children_.front(), children_.back());
Expand All @@ -408,18 +417,18 @@ void CsgOpNode::BatchUnion() const {
*/
std::vector<std::shared_ptr<CsgNode>> &CsgOpNode::GetChildren(
bool finalize) const {
auto &children_ = impl_->children_;
if (children_.empty() || (impl_->simplified_ && !finalize) ||
impl_->flattened_)
auto impl = impl_.GetGuard();
auto &children_ = impl->children_;
if (children_.empty() || (impl->simplified_ && !finalize) || impl->flattened_)
return children_;
impl_->simplified_ = true;
impl_->flattened_ = finalize;
impl->simplified_ = true;
impl->flattened_ = finalize;
std::vector<std::shared_ptr<CsgNode>> newChildren;

CsgNodeType op = impl_->op_;
CsgNodeType op = op_;
for (auto &child : children_) {
if (child->GetNodeType() == op && child.use_count() == 1 &&
std::dynamic_pointer_cast<CsgOpNode>(child)->impl_.use_count() == 1) {
std::dynamic_pointer_cast<CsgOpNode>(child)->impl_.UseCount() == 1) {
auto grandchildren =
std::dynamic_pointer_cast<CsgOpNode>(child)->GetChildren(finalize);
int start = children_.size();
Expand All @@ -444,13 +453,13 @@ std::vector<std::shared_ptr<CsgNode>> &CsgOpNode::GetChildren(
void CsgOpNode::SetOp(OpType op) {
switch (op) {
case OpType::Add:
impl_->op_ = CsgNodeType::Union;
op_ = CsgNodeType::Union;
break;
case OpType::Subtract:
impl_->op_ = CsgNodeType::Difference;
op_ = CsgNodeType::Difference;
break;
case OpType::Intersect:
impl_->op_ = CsgNodeType::Intersection;
op_ = CsgNodeType::Intersection;
break;
}
}
Expand Down
12 changes: 6 additions & 6 deletions src/manifold/src/csg_tree.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,18 +72,18 @@ class CsgOpNode final : public CsgNode {

std::shared_ptr<CsgLeafNode> ToLeafNode() const override;

CsgNodeType GetNodeType() const override { return impl_->op_; }
CsgNodeType GetNodeType() const override { return op_; }

glm::mat4x3 GetTransform() const override;

private:
struct Impl {
CsgNodeType op_;
mutable std::vector<std::shared_ptr<CsgNode>> children_;
mutable bool simplified_ = false;
mutable bool flattened_ = false;
std::vector<std::shared_ptr<CsgNode>> children_;
bool simplified_ = false;
bool flattened_ = false;
};
std::shared_ptr<Impl> impl_ = nullptr;
mutable ConcurrentSharedPtr<Impl> impl_ = ConcurrentSharedPtr<Impl>(Impl{});
CsgNodeType op_;
glm::mat4x3 transform_ = glm::mat4x3(1.0f);
// the following fields are for lazy evaluation, so they are mutable
mutable std::shared_ptr<CsgLeafNode> cache_ = nullptr;
Expand Down
13 changes: 8 additions & 5 deletions src/utilities/include/utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,12 @@ class strided_range {
template <typename T>
class ConcurrentSharedPtr {
public:
ConcurrentSharedPtr(T value) : impl(std::make_shared<T>(value)), mutex(std::make_shared<std::mutex>()) {}
ConcurrentSharedPtr(const ConcurrentSharedPtr<T>& other) : impl(other.impl), mutex(other.mutex) {}
ConcurrentSharedPtr(T value) : impl(std::make_shared<T>(value)) {}
ConcurrentSharedPtr(const ConcurrentSharedPtr<T>& other)
: impl(other.impl), mutex(other.mutex) {}
class SharedPtrGuard {
public:
SharedPtrGuard(std::mutex* mutex, T* content)
SharedPtrGuard(std::recursive_mutex* mutex, T* content)
: mutex(mutex), content(content) {
mutex->lock();
}
Expand All @@ -208,14 +209,16 @@ class ConcurrentSharedPtr {
T* operator->() { return content; }

private:
std::mutex* mutex;
std::recursive_mutex* mutex;
T* content;
};
SharedPtrGuard GetGuard() { return SharedPtrGuard(mutex.get(), impl.get()); };
unsigned int UseCount() { return impl.use_count(); };

private:
std::shared_ptr<T> impl;
std::shared_ptr<std::mutex> mutex;
std::shared_ptr<std::recursive_mutex> mutex =
std::make_shared<std::recursive_mutex>();
};
/** @} */
} // namespace manifold

0 comments on commit 9f8a6d4

Please sign in to comment.