Reorganization of Transforms

z3dev commented 5 years ago

This pull request creates a small module of the transform functions. The goals are:

Convert to V2 math
Standardize API; function names, parameters, error checks, etc.
Add full test suites

mirror

[X] code complete
[X] tests complete. 96.43% coverage
[X] review complete

rotate

[X] code complete
[X] tests complete, 92.5% coverage
[X] review complete

scale

[X] code complete
[X] tests complete, 86.67% coverage
[X] review complete

transform

[X] code complete
[X] tests complete, 94.12% coverage
[X] review complete

translate

[X] code complete
[X] tests complete, 89.29% coverage
[X] review complete

z3dev commented 5 years ago

The associated translateX, etc. functions are also defined. See the aliases defined at the end of each file.

z3dev commented 5 years ago

I have a BIG issue with the current rotate() function as found in V1. If you look into the code, you will find.

    return o.rotateX(v[0]).rotateY(v[1]).rotateZ(v[2])

This is the OpenSCAD definition of how rotate works.

If you look at the 'mutators' for rotate() then you find this definition.

  prot.rotate = function (rotationCenter, rotationAxis, degrees) {
    return this.transform(Matrix4x4.rotation(rotationCenter, rotationAxis, degrees))
  }

Matrix4x4.rotation() calculates a matrix using OrthonormalBasis. Very ugly. I traced back the commits to find out when this was added, expecting some comments. NONE.

From my point of view, both of these implementations are insufficient. So, I added a new function, which is inside rotate.js. It actually has some mathematical background. :)

Thoughts?

kaosat-dev commented 5 years ago

about rotation: the openscad rotatation is the instinctive/ natural one: the one with a center and axis is very counter intuitive (but can have its uses), I will need to go through your new implementation to double check how that works

z3dev commented 5 years ago

I will need to go through your new implementation to double check how that works

Thanks. An extra pair of eyes and another brain would be good here.

I fogot to mention that the mutators also have 'rotateEulerAngles', which supports a point of rotation. I would like to add fromEulersRotation() to mat4. Is that fine?

kaosat-dev commented 5 years ago

hmm adding more stuff ? less is more unless really needed :D adding the code is one thing, but more api is another :D I have more time tomorrow so I can sit down and tests the rotation 'manually'

z3dev commented 5 years ago

rotateEulerAngles

I think this was added/fixed a few years back by @vr050714 Again, another undocumented feature of the library but useful because it has an arbritary point of rotation. Could this be a possible replacement for OrthonormalBasis?

I won't add the new function in this pull request but will keep the code around. :)

vr050714 commented 5 years ago

rotateEulerAngles

I think this was added/fixed a few years back by @vr050714 Again, another undocumented feature of the library but useful because it has an arbritary point of rotation. Could this be a possible replacement for OrthonormalBasis?

I won't add the new function in this pull request but will keep the code around. :)

OrthoNormalBasis, if you are talking about src/math/OrthoNormalBasis.js file, might be hard to use. Hard to imagine how to get plane normal and some vector pointing to "right". Also it has an issue when this vector is collinear to normal.

rotateEulerAngles function is not perfect too. It must only evaluate the new basis according to given Euler's angles whereas the translation does not match to semantic of the function. Ideally, it should be like this code, for example:

//... required imports

let rotZ1 = degToRad(90);
let rotX  = degToRad(45);
let rotZ2 = degToRad(30);

let newBasis = fromEulerAnglesZXZ(rotZ1, rotX, rotZ2);

let somePoint = [10, 20, 42];
let newPosition = translate(somePoint);

let alignedShape = transform(
                       newPosition,
                       transform(
                           newBasis,
                           originalShape
                       )
                   );

z3dev commented 5 years ago

@vr050714 WOW! Thanks for the immediate feedback after... many years!

As far as OrthonormalBasis goes... we want to eliminate it. Hopefully, it won’t be difficult once we have some of the new functionality in place.

As for rotate, I think a traditional Euler angle (ZXZ) implementation is too difficult for most users to understand. Therefore, I implemented a function that uses Tait-Bryan Euler angles. Can you review the rotation() function inside of rotate.js ?

vr050714 commented 5 years ago

@vr050714 WOW! Thanks for the immediate feedback after... many years!

As far as OrthonormalBasis goes... we want to eliminate it. Hopefully, it won’t be difficult once we have some of the new functionality in place.

As for rotate, I think a traditional Euler angle (ZXZ) implementation is too difficult for most users to understand. Therefore, I implemented a function that uses Tait-Bryan Euler angles. Can you review the rotation() function inside of rotate.js ?

Sorry, I cannot find rotation() function in any of 5 rotate.js files as well as a mention of either Tait-Bryan or ZYX angles. Can you point out exact file, please?

Anyway, the general purpose library must have all possible variants. And it would be awesome to have links to Wikipedia in docstrings.

z3dev commented 5 years ago

Anyway, the general purpose library must have all possible variants. And it would be awesome to have links to Wikipedia in docstrings.

I found an even better reference, possibly the most complete. https://en.wikipedia.org/wiki/Euler_angles

@vr050714 Please click on the "Files changed" tab above, then scroll down to the changes for rotate.js. Any thoughts would be welcome. :)

z3dev commented 5 years ago

@vr050714 thanks again for the assistance.

I have one more request. In the V2 branch, we have a set of new math functionality. (src/math/*) As you mentioned above, the orientation of the coordiante system is very important, especially for rotation. Therefore, I added a special test to verify that all math functionality is respecting the right-hand-rule of rotation.

Can you review src/math/rotation.test.js ? Even now, I think some of my comments are wrong. But the rotations are correct. :)

z3dev commented 5 years ago

Pending any final comments, this pull request is ready to merge. :)

kaosat-dev commented 5 years ago

@z3dev dang, I missed that last comment of yours: so is everything ok here from your end ? it all seems ok to me

z3dev commented 5 years ago

@kaosat-dev Correct. Please merge.

jscad / csg.js

Reorganization of Transforms #171