Simplify Endoscopy API relelated to quaternion

[jcfr]

This is a follow-up of commit 717fd6f (ENH: Add support for managing orientation keyframe in Endoscopy) introduced through #7165

The set of commits aims to simplify the Endoscopy API (removing ~60 ~30 lines):

• Simplifies math helper functions API supporting only "return by value": Supporting both "return by value" and "pass mutable object as argument" API is error-prone.
• Removes unnecessary wrapping of relative orientation functions.
• Directly utilizes vtkMRMLMarkupsNode::SetNthControlPointOrientation.
• Directly employs VTK's math functions for converting between matrices and quaternions.
• Streamlines interpolation by eliminating unnecessary functions: orientationToQuaternion and quaternionToOrientation are converting to/from the same object.
• Remove unused functions:
  • getRelativeOrientation
  • matrix3x3ToQuaternion and quaternionToMatrix3x3 (these were also not working)

[Leengit]

• Streamlines interpolation by eliminating unnecessary functions: orientationToQuaternion and quaternionToOrientation are converting to/from the same object.

To clarify, are you saying that we should store quaternions rather than orientations with our

self.inputCurve.SetAttribute(
    EndoscopyLogic.NODE_PATH_CAMERA_ORIENTATIONS_ATTRIBUTE_NAME, cameraOrientationsString,
)

command?

Backing up a few steps, to make sure that we are on the same page ... quaternions and orientations are the same Python type <class 'numpy.ndarray'> and shape (4,), but their numerical values are not the same, and math is required to convert from one to the other.

• orientation = (angle, *axis), where angle is in radians and axis is the unit 3D-vector for the axis of rotation.
• quaternion = (cos(angle/2), *axis * sin(angle/2))

The vtkQuaternionInterpolator operates on quaternions but we currently use orientations with inputCurve.SetAttribute(...) under the theory that orientations are more intuitive to the user.

[jcfr]

Thanks for the review and clarification

For the current integration, I've removed that specific portion.

Analysis

The 4-value "representation" array was treated as a "wxyz quaternion" instead of an "axis-angle" representation. While this approach somewhat maintained the overall logic, including keyframe saving and deletion, it led to a couple of issues:

1. The inverse of the "orientation" was not computed correctly.
2. The wxyz parameter associated with vtkMRMLMarkupsNode::SetNthControlPointOrientation(int n, const double wxyz[4]) is expected to be in "axis-angle" representation¹.

Footnotes

1. Internally, the implementation uses vtkMRMLMarkupsNode::ConvertOrientationMatrixToWXYZ and vtkMRMLMarkupsNode::ConvertOrientationWXYZToMatrix, which correspond to EndoscopyLogic.orientationToMatrix3x3 and EndoscopyLogic.matrix3x3ToOrientation. ↩

[Leengit]

Any time a 4-value array is in angle-axis format, the corresponding variable is named with "orientation", and whenever a 4-value array is in quaternion format, the corresponding variable is named with "quaternion". I recommend that we keep this convention.

I see that this pull request

1. Changes the math helper functions API by supporting only return by value.
2. Replaces some functions by inline implementations of them where they were previously invoked.

If the code is also intended to change the computation of the inverse of an orientation, I don't see where that is accomplished.

[jcfr]

If the code is also intended to change the computation of the inverse of an orientation, I don't see where that is accomplished.

The comments and analysis were specific to the now removed attempt at treating the 4-value "representation" array as a "wxyz quaternion" instead of an "axis-angle" representation.

This is why this topic doesn't implement any changes related to the inverse of the "orientation"

[Leengit]

A few changes happened between my pushed branches and the final merge. In case this proves useful for the next steps, I offer these clarifications.

Remove unused functions:

* `getRelativeOrientation`

This and similar functions were provided to hide the interface to where the orientations were stored and whether the actual storage was as worldOrientations or relativeOrientations. We may wish to restore these functions in the future, especially if we change either of these decisions, because it localizes these decisions to a small number of locations rather than spreading them to everywhere where these deleted functions were invoked.

Remove unused functions:

* `matrix3x3ToQuaternion` and `quaternionToMatrix3x3` (these were also not working)

The case change EndoscopyLogic.Matrix3x3ToQuaternion to EndoscopyLogic.matrix3x3ToQuaternion (and similar) also inadvertently changed vtk.vtkMath.Matrix3x3ToQuaternion to vtk.vtkMath.matrix3x3ToQuaternion (and similar), thus breaking the calls to those subroutines, which otherwise were working.

[jcfr]

To help address this, could you reference the commit in main introducing the regression?

[Leengit]

To help address this, could you reference the commit in main introducing the regression?

Commit 0a5af4e on my branch 6502_flythrough had the correct case for the vtk.vtkMath functions. It is not in the direct history of the main branch because of the refactoring that took place before #7165 was merged. In the refactored commits that are in the history of the main branch, commit 717fd6f has the first use of the vtk.vtkMath functions in the modern Endoscopy module, but uses the wrong case.

[jcfr]

re: EndoscopyLogic.matrix3x3ToQuaternion / EndoscopyLogic.quaternionToMatrix3x3

Indeed, in the context of the integration, for sake of consistency, the capitalization of all the functions added to EndoscopyWidget and EndoscopyLogic were changed to start with a lowercase letter.

While working on this, in commit 717fd6f, the capitalization of the functions calls associated with vtkMath was also inadvertently changed :

    @staticmethod
    def matrix3x3ToQuaternion(matrix3x3, quaternion=None):
        quaternion = quaternion if quaternion is not None else np.zeros((4,))
-        vtk.vtkMath.Matrix3x3ToQuaternion(matrix3x3, quaternion)
+        vtk.vtkMath.matrix3x3ToQuaternion(matrix3x3, quaternion)
        return quaternion

    @staticmethod
    def quaternionToMatrix3x3(quaternion, matrix3x3=None):
        matrix3x3 = matrix3x3 if matrix3x3 is not None else np.zeros((3, 3))
-        vtk.vtkMath.QuaternionToMatrix3x3(quaternion, matrix3x3)
+        vtk.vtkMath.quaternionToMatrix3x3(quaternion, matrix3x3)
        return matrix3x3

Since the two functions EndoscopyLogic.matrix3x3ToQuaternion and EndoscopyLogic.quaternionToMatrix3x3 were never user, this had no impact on the functioning of the module.

Then, the two unused functions were removed in efcc1a5.

[jcfr]

A few changes happened between my pushed branches and the final merge

Since the changes introduced in this pull request have not been changed since the approval.

I will assume, you are referring to the changes introduced in #7165

[Leengit]

the capitalization of all the functions added to EndoscopyWidget and EndoscopyLogic were changed to start with a lowercase letter.

I had been using a starting uppercase letter for methods that were @staticmethod, but your way works -- thanks!

[jcfr]

re: commit 5f201f5 (Simplify Endoscopy API removing wrapping of relative orientation functions)
This and similar functions were provided to hide the interface to where the orientations were stored and whether the actual storage was as worldOrientations or relativeOrientations. We may wish to restore these functions in the future, especially if we change either of these decisions, because it localizes these decisions to a small number of locations rather than spreading them to everywhere where these deleted functions were invoked.

If we revisit how the relative "positioning" is done, this will likely be done by changing how the transformation is computed and this will likely be done in the functions worldOrientationToRelativ and worldOrientationToRelative.