Fix plane rotation method (again...)

[Peque]

So, it seems the test I implemented in #235 was not very complete. Out of laziness, I only used "random" planes with the normal in the X, Y or Z directions. Therefore, the implementation was broken. 😓

Hopefully, these tests are now more complete and exhaustive.

About the implementation... I wanted to open the PR to hear from you. It looks so ugly and I do not know much, or anything, about Python-OCC. So I bet it can be implemented differently and more elegantly. 😇

@adam-urbanczyk @jmwright @dcowden @fragmuffin Any suggestions?

[Peque]

Ok, so NumPy is not a dependency of CadQuery. 😅

Will have a look later, or tomorrow, to see if I see an alternative cross implementation within the available dependencies. Or I will make my own.

[jmwright]

Ok, so NumPy is not a dependency of CadQuery.

It must be at least an implicit dep of CQ 2.0 because I have a freshly built cadquery Anaconda environment and can do import numpy in a Python console without any errors.

[jmwright]

I personally would be fine with installing numpy via pip in the CI environments. It seems to be included in the Anaconda environment, and I think that Numpy is too natural of a package to use with CadQuery not to include it anyway.

[Peque]

@jmwright I added NumPy as a dependency for now. Just to make tests pass.

I am more interested in receiving feedback about the implementation. If I have to remove NumPy dependency, happy to do so. That would be a minor thing. 😊

Anyway, NumPy feels natural to me here too.

[codecov]

Codecov Report

Merging #243 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
- Coverage   94.95%   94.87%   -0.09%     
==========================================
  Files          18       18              
  Lines        4279     4308      +29     
==========================================
+ Hits         4063     4087      +24     
- Misses        216      221       +5     

Impacted Files	Coverage Δ
cadquery/occ_impl/geom.py	88.66% <0.00%> (-1.34%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45cfa16...79f220c. Read the comment docs.

[adam-urbanczyk]

@Peque I do not get it - why the changes in geom?

[Peque]

@adam-urbanczyk The rotation matrix (class Matrix) was always using world-axis for the rotation. But the rotation of a plane should be relative to its axis. This is the way I managed to pass the proper axis to the rotation matrix before rotating the plane. There may be a better way, since I have zero experience with python-occ and little with CadQuery.

I hope the tests are clear though. Plane rotations needed some tests. ❤️

[Peque]

By the way, the methods Matrix.rotateX, Matrix.rotateY and Matrix.rotateZ are called nowhere except from Plane.rotated.

That is the reason why Codecov complains about some misses: I left an if direction is None: if-clause for backwards compatibility, but now Plane.rotated is always passing its axis to Matrix.rotateX, so that case is not really covered in the test suite. And I am not sure it should be. Maybe the direction parameter should be mandatory (?).

[Peque]

@adam-urbanczyk Friendly ping. 😇

[adam-urbanczyk]

Thanks for picking this up @Peque !

[jmwright]

I'm cross-referencing issues here.

#254 (comment)

[Peque]

@adam-urbanczyk Thanks for having a look at #255. 😊

This MR is ready too. I pushed changes after your suggestions/corrections. Did not resolve the conversations since I think only the one requesting changes should be deciding whether or not the changes actually resolved what they asked for.

If you are okay with it, I will squash the fixup commits and force-push the branch before merging.

I think it could be merged before reaching an agreement on a new API and implementing the required changes (#254). This MR only fixes the current behavior without changing the API.

[jmwright]

@adam-urbanczyk @Peque Where are we at on this PR? It sounds like it might be ready for a final check.

[adam-urbanczyk]

Alright let's merge this - @Peque I added one minor comment request (to clarify the situation for ppl reading the code in the future - we abused the Vector to store angles which is probably very confusing). Note that the behavior w.r.t. original version is changing here (although the docs were describing the behavior implemented correctly in this PR).

[Peque]

@adam-urbanczyk Tell me if you want me to squash this or if you would rather do it yourself (I have pushed all changes as "fixup" commits for easier review). Same for rebasing against upstream/master.

[jmwright]

@Peque Looks like the Black lint check threw an error.

[Peque]

@jmwright Rebased and force-pushed, fixing formatting too.

[adam-urbanczyk]

Looks good, thanks @Peque !