Fix Unit3 copy assign

[martinvl]

Unit3::operator= doesn't properly handle setting the cached basis and Jacobian. If B_ and/or H_B_ was calculated prior to calling Unit3::operator= then they are not reset, which causes subsequent calls to basis() to return the previous basis and Jacobian.

This PR fixes Unit3::operator=(const Unit3& u) so that B_ and H_B_ are set to the cached values of u if they exist, and reset otherwise.

---

This change is 

[varunagrawal]

Great PR, thanks @martinvl. Just requesting one small change.

[varunagrawal]

Test name should follow the naming convention.

[dellaert]

Thanks! However, The current implementation of the operator makes me think it is deliberate. The reason is that your implementation probably agrees with the default copy operator, right?

The current state is almost certainly my doing, but I forgot, and I neglected to comment this :-( I imagine it was to state that the cache is probably only actually populated for a specific instance, e.g., during optimization, and when copied it probably won’t be used again - so it’s wasteful to copy it.

Finally, I don’t quite understand your statement about “the previous” Basis or Jacobian. The Unit3 is immutable, so the cache is either not there, or correct.

Is there a bug or a problem that you’re trying to fix here? If there is, and copying the cache is desired, then the solution is probably to remove the the assign and copy implementations. But I’m somehow doubtful, given the last comment above.

[martinvl]

@dellaert I see your point about not making wasteful copies.

Unit3 is not fully immutable since it has the copy assign operator. As per my test case, the following code is valid:

Unit3 p{1, 0.2, 0.3};
p = Unit3{-1, 2, 8};

The problem here is if B_ and H_B_ were cached before the call to copy assign, because then B_ and H_B_ won't be updated to account for the new value:

Unit3 p{1, 0.2, 0.3};
p.error(p); // this triggers the computation of `B_`
p = Unit3{-1, 2, 8};
const auto err = p.error(p); // no new computation of `B_` is triggered, and `err` gets wrong value

Another acceptable solution would be to simply call reset() on B_ and H_B_, which would not make a copy, and force a recalculation if they are needed later. And yet another solution is to remove the copy assign operator altogether, as you mentioned.

[dellaert]

@martinvl you are right. Probably the right thing is to just delete the copy constructor and the assignment operator. I am assuming your test would fail if that does not do the right thing. Could you add a similar test for the copy constructor and then just remove both?

[martinvl]

Sure, will fix it first thing in the morning!

[martinvl]

The copy constructor cannot cause this type of issue, because in this case B_ and H_B_ are default-initialized to nullopt, so I can't really make the same test for that.

This is only an issue with the assignment operator, as it is the only mutable method of Unit3. Therefore I've only removed the assignment operator. Just waiting to see if the CI passes now.

[varunagrawal]

@martinvl so CI is failing because of an assignment in Manifold.h.

Can you please take a look at it?

[martinvl]

First of all: Sorry for the delay! For some reason I didn't get a notification about the CI and totally forgot!

As you say @varunagrawal, we actually need the assignment operator, so simply removing does not seem a viable option.

I did some more digging, and it turns out the Unit3 has a std::mutex member when TBB is on. The mutex member causes the default assignment operators (as well as copy/move ctors) to be ill-formed. @dellaert questioned why the non-default operators were used in the first place, and I think the TBB-mutex was the real reason.

Therefore I suggest using my original patch, which simply fixed the issues in the non-default operators.

Resetting to a6e9b02.

[dellaert]

OK, I'll merge. Thanks !