[5.6] Streamline Endoscopy UX removing "Use this curve" button #7462
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit addresses an issue introduced during the integration of the pull request associated with commit 717fd6f (ENH: Add support for managing orientation keyframes in Endoscopy). The logic that was inadvertently removed is now reinstated to prevent unintended modification events triggered by changes to the input curve. The variable "ignoreInputCurveModified" is reintroduced to manage and prevent a second modification event. Additionally, this commit addresses typos in comments. Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com> (cherry picked from commit c8599af)
Supporting an API allowing to get output using both "return by value" and "pass mutable object as argument" is error-prone. Considering the returned value is only few bytes (e.g 32 bytes for wxyz orientation and 128 bytes for a 4x4 matrix), the added complexity it not worth it. To adhere to the principle of least surprise and make the code more readable, this commit simplifies the API and updates the code accordingly. (cherry picked from commit 474872a)
… functions This is intended to facilitate the reading and understanding of the code by directly using the vtkMRMLMarkupsNode::SetNthControlPointOrientation API. It also removes the unused function getRelativeOrientation() (cherry picked from commit b021205)
(cherry picked from commit cc0168c)
Co-authored-by: Lee Newberg <lee.newberg@kitware.com> (cherry picked from commit eda7a41)
…egree" Co-authored-by: Lee Newberg <lee.newberg@kitware.com> (cherry picked from commit 3476a2f)
The logic for when to loadCameraOrientations and recompute EndoscopyLogic fields was not working in all cases. The lazy approach to loading and recomputing could have been modified to fix this, but we did not pursue this approach because the lazy approach has a flaw: the user interface (e.g. number of frames available) should be available immediately, not lazily (e.g. only once the user starts a flythrough). (cherry picked from commit 5e45824)
…method (cherry picked from commit c28bfc7)
Co-authored-by: Lee Newberg <lee.newberg@kitware.com> (cherry picked from commit c6a86d9)
(cherry picked from commit 706a790)
…entations This allows to set and get the input curve CameraOrientations independently of the EndoscopyWidget. To support this it also updates the `interpolateOrientations` to require a cameraOrientations parameter. This allows to remove the need for maintaining a dedicated "cameraOrientations" instance variable. It also renames the functions: * `loadCameraOrientations` to `getCameraOrientationsFromInputCurve` * `saveCameraOrientations` to `setInputCurveCameraOrientations` (cherry picked from commit b6975a4)
(cherry picked from commit 5fdf4c2)
(cherry picked from commit eb73109)
(cherry picked from commit ef465b4)
…ionAtIndex (cherry picked from commit 6d081ff)
(cherry picked from commit 8e6bb82)
Directly set references to the transform and cursor nodes on the input curve node and introduce API methods for creating and retrieving the associated nodes: * createTransformFromInputCurve() / getTransformFromInputCurve() / setTransformToInputCurve() * createCursorFromInputCurve() / getCursorFromInputCurve() / setCursorToInputCurve() (cherry picked from commit 9ef20a6)
…points Since the function ensures a smooth transition of orientations along the control points, this name explicitly mentions the interpolation of orientations and clarifies that it is associated with control points. Suggested-by: Lee Newberg <lee.newberg@kitware.com> (cherry picked from commit 06f0e27)
…t curve Follow-up on commit e3eceb6 and rename "setControlPoints" to "setControlPointsByResamplingAndInterpolationFromInputCurve". This name emphasizes a direct relationship with the input curve and reflects the process of updating or creating control points through resampling and interpolation. Suggested-by: Lee Newberg <lee.newberg@kitware.com> (cherry picked from commit ee01826)
…ions Remove the need to explicitly interpolate orientations by systematically calling "interpolateOrientationsForControlPoints()" after saving or deleting orientations. (cherry picked from commit fb0675e)
…erence Remove cameraNode instance variable and manage association of camera with input curve by adding a "Camera" reference. (cherry picked from commit 280e7f3)
Selecting the curve is now sufficient to work with the associated flythrough path. Summary of changes: * Remove "Use this curve" button and instead use the input curve upon selection. * Simplify and instantiate one EndoscopyLogic * Remove enableOrDisableCreateButton and consolidate all UI update in updateWidgetFromMRML * Remove ignoreInputCurveModified instance variable and instead introduce a updatingControlPoints variable to guard setControlPoints * Observe the more specific curve events "PointModifiedEvent" and "PointEndInteractionEvent" instead of "ModifiedEvent" and adds the corresponding callbacks "onInputCurveControlPointModified" and "onInputCurveControlPointEndInteractionEvent". This ensures to have the flythrough path updated only when user is done updating the curve or when the input curve is programmatically modified. * Rename "onPlayButtonToggled" to "setPlaybackEnabled" to allow programmatic start/stop of playback. * Reset input curve and stop playback on module cleanup or when "slicer.mrmlScene.StartCloseEvent" is invoked. (cherry picked from commit 738aca6)
(cherry picked from commit a418b96)
(cherry picked from commit 58fafb5)
This commit addresses an issue in `EndoscopyLogic` related to obtaining and setting camera properties. The problem arose from the camera being set as a reference on the input curve, leading to inconsistencies in managing camera observers. The warnings "does not have observer" and "already has observer" were reported upon closing the scene, indicating a desynchronization in camera observer management. This commit resolves these warnings by simply removing the observation of the camera. Further investigation revealed that observing the camera is unnecessary because: 1. Camera properties are retrieved during keyframe saving. 2. Camera properties are updated when adjusting the position along the flythrough path. 3. Camera view angle is updated during adjustments. (cherry picked from commit 81e12b6)
jcfr
force-pushed
the
5.6-backport/Endoscopy
branch
from
8c54995
to
c427b52
Compare
Leengit
approved these changes
Dec 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backports changes originally contributed to
mainthrough the following pull requests:Verified that the
Endoscopy.pyfile associated with the5.6branch includes all changes from the one of themainbranch (as of 7bd7cd0) expect the one introduced through #7446