Lazy transformations for CrossSection

[geoffder]

After reading through the transformation code more for Manifold (csg_tree and impl), I decided try bringing CrossSection more in line with the lazy transformation semantics expressed there. Only copying and applying the transformations when needed cuts out some of the repeated iteration and point type conversion that pervaded the wrapper.

Similar to the CSG nodes and GetImpl, the private GetPaths applies the transformation whenever paths_ is needed, and updates the object so that the same operation won't be repeated.

[codecov]

Codecov Report

Patch coverage: 86.79% and project coverage change: +0.03 🎉

Comparison is base (83131c6) 89.43% compared to head (931873f) 89.46%.

❗ Current head 931873f differs from pull request most recent head cd3b2df. Consider uploading reports for the commit cd3b2df to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #355      +/-   ##
==========================================
+ Coverage   89.43%   89.46%   +0.03%     
==========================================
  Files          33       33              
  Lines        3974     3968       -6     
==========================================
- Hits         3554     3550       -4     
+ Misses        420      418       -2     

Impacted Files	Coverage Δ
src/cross_section/src/cross_section.cpp	57.98% <86.79%> (-0.22%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pca006132]

Did you implement shared cache when users copy the path? If not, we will need to do n transformations instead of 1 when the path is copied n times, for example for other boolean operation or something.
(Although the transformation is probably cheap enough and not warrant such a cache)

Otherwise this looks good to me.

[geoffder]

It could be that I'm not totally familiar with when copies are happening due to my C++ inexperience, but I thought that in this setup there wouldn't be multiple repeated transformation copies of the vector for separate booleans on the same parent manifold since transform_ is reset to the identity.

I also could be wrong in my assumption that transformed.paths_ = paths_ doesn't copy the elements of the vector (my intention was for transform to be when the copy into a fresh PathsD is performed, severing the mutable link with the parent CrossSection if there was one). That's something I realized I should experiment with / read about, but forgot.

[pca006132]

Ah, no, I mean the copy constructor.

manifold/src/cross_section/src/cross_section.cpp

Lines 121 to 124 in f92667e

	CrossSection::CrossSection(const CrossSection& other) {
	paths_ = other.paths_;
	transform_ = other.transform_;
	}

The new CrossSection object will have a new paths_ vector that is not yet transformed, and the paths_ field is not shared between the copies, so calls to GetPaths on each copy will perform the same transformations repeatedly. For CsgNode, we have

manifold/src/manifold/src/csg_tree.h

Line 58 in 83131c6

mutable std::shared_ptr<const Manifold::Impl> pImpl_;

the std::shared_ptr implements a shared cache.

[geoffder]

Oh right. Well it wouldn't be too much effort to convert paths_ to a shared ptr to cover that case as well I think. I was also thinking about whether these mutables should be protected by a lock but you've beat me to bringing it up #356.

[geoffder]

Thinking about this a bit more, is there a point of having a copy constructor for these objects that don't expose any mutable state to the outside anyway? (there's probably some reason it's good to have that doesn't come to mind for me as a C++ novice, hence the stupid question)

[geoffder]

Looking at GetImpl(), the pImpl_ pointer appears to be replaced by a new pointer, rather than updated. So if any copies share the original pImpl_ pointer, they aren't actually updated, right? Am I getting lost in the indirection and looking at the wrong place?

std::shared_ptr<const Manifold::Impl> CsgLeafNode::GetImpl() const {
  if (transform_ == glm::mat4x3(1.0f)) return pImpl_;
  pImpl_ =
      std::make_shared<const Manifold::Impl>(pImpl_->Transform(transform_));
  transform_ = glm::mat4x3(1.0f);
  return pImpl_;
}

Also, since transform isn't similarly sitting behind a shared pointer, if application of transformation was shared between the copies, the reset of transform_ wouldn't be propagated, which would result in re-applications (if the shared mesh/manifold data was being mutated by GetImpl.

[elalish]

@geoffder I'll defer to @pca006132 who understands this part of the code better than I do, though I'd guess he's asleep now. It might help to make a little example and throw in print statements where transforms are actually applied to verts. The results would probably make for nice documentation as well.

[geoffder]

In the meantime, here is a bit more thinking out loud, plus an experiment.

The mutable std::shared_ptr<const Manifold::Impl> pImpl_; pointer in CsgLeafNode is actually const, so updating what it is pointing at with *pImpl_ = ... is not actually allowed (which from what I understand, would be how transformation application would be shared between copies). I tried out making the equivalent paths_ in CrossSection non-const for this experiment and it does not work as is. I made a branch to experiment with a shared_ptr version here. The comments re-iterating some of what I have already said in here for context.

Running this with the fresh ptr and const data version based on GetImpl, this test gives the following output:

making a...
making b...
copying b...
extruding a and b...
Applied transform in GetPaths()
Applied transform in GetPaths()
extruding b_copy...
Applied transform in GetPaths()
extruding the original b again...

So, as expected the same transformation is applied on the as yet un-transformed b_copy before it is extruded (meshes produced by the extrusions are identical). This behaviour is that same as the non-shared_ptr version of this PR, so there isn't anything funny happening, it just similarly does not avoid duplication of transformation effort between copies. Then again, calling back to my previous comment, I'm not sure how much of a problem that is with the functional style (nice) const interface.

[pca006132]

Oh, nice, I forgot to add a * for the pImpl_. And yes, the transform matrix also need to be shared between different meshes. I guess we can leave it here for now and fix them together with another PR. Thanks for spotting the bugs!

[pca006132]

I found that the old me is partially correct: the cache for CsgLeafNode is meant to be immutable, the benefit of the cache is to reduce the number of copying rather than reduce the number of transformation application. I implemented the transform cache just to find that due to additional copying, the performance is worse. This may also be important here: we should avoid copying the path as much as possible, and we can just perform the transform on-the-fly while copying at the end. I will document this later.

But for the CsgOpNode, there is just the concurrency issue.

[geoffder]

I finally went and re-read the reference page for vectors and was reminded (or TIL'd, I'm not sure) that the assignment operator for does indeed copy the contents. Something that was not obvious to me coming from languages where that is not the case 🤦‍♂️. I've updated paths_ to follow the immutable (contents) shared_ptr scheme accordingly.

[geoffder]

A (final?) note on the topic of the copy constructor again, I don't know that there is necessarily a better solution than what we are doing currently.

auto a = CrossSection::Circle(10).Scale({2, 0});
auto b = CrossSection(a);
auto ex_a = a.Extrude(5);
auto ex_b = b.Rotate(90).Extrude(5);

Even if the additional machinery was implemented to allow the transformation of a to be propagated to b, the same number of transformations is going to occur. Alternatively, if the copy constructor was changed to preemptively apply the transformations in a to cut off the possibility of re-doing the same transform_, if there are any further transforms to either or both, there could end up being more applications than the current case.

The solution is really to just not use the copy constructor, since there is nothing to be gained from it (just do what you want to a, don't make a copy of it to do things to).

[geoffder]

Rebased thinking I was going to use the ConcurrentSharedPtr (and I still could change it anyway), but then I remembered that the contents of the shared_ptr are const anyway, so there aren't any race conditions to protect against. Update is only done by replacing the whole thing. So if thread safety is to be added, it must be at the class member level (a private mutex member to protect the mutable paths_ and transform_ members, rather than one bundled with the pointer itself in ConcurrentSharedPtr). Right?

[pca006132]

Yes, you don't need ConcurrentSharedPtr if you directly replaces the pointer.

[elalish]

Thanks!

[pca006132]

Is it ready to merge?