Skip to content

BUG: Fix Computation of Interaction Transform Used in PositionProp | ⚠️ Changes removed from master #139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

jcfr
Copy link
Contributor

@jcfr jcfr commented Dec 21, 2023

⚠️ Changes originally introduced through this pull-request have been removed from master by force pushing. Corresponding changes are superseded by #141 ⚠️

Rational:


This commit fixes the incorrect and unnecessary conversion from axis-angle to quaternion inadvertently introduced in 503ef1d ("ENH: Add VR interactor style class for transforming objects in MRML", 2018-08-31).

The additional conversion, introduced in the mentioned commit, was redundant since the event orientations (internally set using vtkVRRenderWindowInteractor::ConvertPoseToWorldCoordinates, itself introduced in Kitware/VTK@803d3a327d, "ENH: Use pose matrices instead of camera parameters for VR controllers", 2018-09-25) are already in quaternion format.

The redundant conversion was leading to an invalid transformation of the selected object.

Reported by @LucasGandel and @sankhesh

This commit fixes the incorrect and unnecessary conversion from axis-angle
to quaternion inadvertently introduced in 503ef1d ("ENH: Add VR interactor
style class for transforming objects in MRML", 2018-08-31).

The additional conversion, introduced in the mentioned commit, was redundant
since the event orientations (internally set using
`vtkVRRenderWindowInteractor::ConvertPoseToWorldCoordinates`, itself introduced
in Kitware/VTK@803d3a327d, "ENH: Use pose matrices instead of camera parameters for
VR controllers", 2018-09-25) are already in quaternion format.

The redundant conversion was leading to an invalid transformation of the
selected object.

Reported-by: Lucas Gandel <[email protected]>
Co-authored-by: David Allemang <[email protected]>
@jcfr jcfr force-pushed the fix-computation-interaction-transform branch from f5cd99d to 3161d34 Compare December 21, 2023 22:05
@jcfr jcfr added bug and removed bug labels Dec 21, 2023
@jcfr jcfr merged commit c9ea608 into KitwareMedical:master Dec 21, 2023
@jcfr jcfr deleted the fix-computation-interaction-transform branch December 21, 2023 23:11
@jcfr
Copy link
Contributor Author

jcfr commented Dec 22, 2023

I may have missing something as inspecting code like the one in vtkInteractorStyle3D::Dolly3D, there is:

  vtkEventDataDevice3D* edd = static_cast<vtkEventDataDevice3D*>(ed);
  const double* wori = edd->GetWorldOrientation();

  // move HMD world in the direction of the controller
  vtkQuaternion<double> q1;
  q1.SetRotationAngleAndAxis(vtkMath::RadiansFromDegrees(wori[0]), wori[1], wori[2], wori[3]);

I am re-evaluating and I will likely force push to revert, and create a following-up topic.

@jcfr
Copy link
Contributor Author

jcfr commented Dec 22, 2023

Analysis

During my initial review of the code associated with vtkVRRenderWindowInteractor::ConvertPoseToWorldCoordinates, I got confused with the docstring mentioning \param wxyz Output world orientation quaternion1 which primed me during my visual inspection of vtkVRRenderWindowInteractor::ConvertPoseToWorldCoordinates source code. As I saw Matrix3x3ToQuaternion, I stopped visually parsing the code and went one with the conclusion that the orientation was a quaternion.

ConvertPoseToWorldCoordinates docstring Matrix3x3ToQuaternion

However, upon further consideration, I realized that the orientation is effectively2 converted to an angle-axis representation. This conversion aligns with VTK's typical internal handling of orientation.

cc: @allemangD

Footnotes

  1. https://github.com/Kitware/VTK/blob/2579c39f26d6800cea2b5b0ca7b094dd22bc7bf9/Rendering/VR/vtkVRRenderWindowInteractor.h#L97-L105

  2. https://github.com/Kitware/VTK/blob/2579c39f26d6800cea2b5b0ca7b094dd22bc7bf9/Rendering/VR/vtkVRRenderWindowInteractor.cxx#L258-L274

@jcfr jcfr changed the title BUG: Fix Computation of Interaction Transform Used in PositionProp BUG: Fix Computation of Interaction Transform Used in PositionProp | ⚠️ Changes removed from master Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant