Multiple rotations in setInitDisp methods #591
Conversation
…_multiRotation
|
Hey @akeeste would it be possible to get some unit tests for this function? I'm thinking at least two tests, where we pass different rotation sequences and check that its returning what we expect... e.g. x 90, z 90, x -90 |
|
@dav-og Good idea, I'll make tests for those cases. I'll also add two for the special cases of converting a rotation matrix back to an axis-angle (180 deg or 0 deg in an axis) |
|
Also, what do you think of passing the rotation axis and amount together? (You could then also just have a couple lines to extract angleList and axisList so to keep the rest of the code as it is 👍) |
|
@dav-og Also I'm not sure why ptoClass.m is conflicting, but I'll fix that when I push the tests too. |
|
@dav-og I have pushed a few CI tests for these functions and fixed the ptoClass.m conflict. Do you think it is worthwhile to change all the function parameters to a combined axis-angle format or leave them as is? |
|
hey @akeeste, do you mean like the I think it would be neater/consistent...but any function that just takes one axis & one rotation is less of a priority IMO. I just thought for a UI that enables a sequence of rotations...then maybe passing one list of axes & angles would be a cleaner UI than a list of axes and a separate list of angles? |
|
Yes I was referring to the other angle functions like axisAngle2RotMat or rotateXYZ. Since its not as important, I will leave them as is for now. Its a library change that will be more difficult to pull in/check. I did push an update to combine the axis-angle for body, constraint, pto, mooring setInitDisp() method as you suggest. All the CI tests I added for the function are working. |
dav-og
left a comment
There was a problem hiding this comment.
This is a good new feature that gives greater flexibility to the user when defining initial displacement/orientation (i.e. for decay tests). Any rotation sequence can be specified to set the initial orientation - and the function is agnostic wrt the rotation order convention used, which is great.
My feedback:
- ✅ unit tests
- tests look good and they are all passing on my local version
- ✅ modified UI
- I suggested combining axis and angle - maybe in a dictionary - to make it simpler for the user (and harder to specify the wrong angle for desired axis). The modification does this and I think its fine but everything is in one list - so we do have mixed types in the same array. Probably fine but something to bear in mind.
This PR adds the ability to include multiple consecutive rotations in the body, constraint and pto setInitDisp() methods.
To keep compatibility with the old setInitDisp function, the axis-angle input is kept. Now, a list of axes is input as a size [nAngle 3] array, and the angle is input as [nAngle 1] array. I thought this implementation is less restrictive than forcing users to input Euler-XYZ convention or similar. They can effectively use any extrinsic convention by listing axes in the desired order.
I removed the x_rot (rotation point) parameter because only the relative coordinates are required. The displacement due to the rotation can be simplified to only depend on relCoord = cg - x_rot, removing the requirement for the cg or rotation point to be defined. For simplicity, only a single relCoord is allowed.