Skip to content
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.

Sign up for GitHub

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

Jump to bottom

New Feature: Multiplanar Oblique Views #359

Merged
merged 25 commits into from
Oct 27, 2023
Merged

New Feature: Multiplanar Oblique Views #359

merged 25 commits into from
Oct 27, 2023

Conversation

jadh4v
Copy link
Collaborator

@jadh4v jadh4v commented Jun 29, 2023 •
edited
Loading

Add a new layout with Multiplanar Oblique Views

Sub-tasks:

Additional Fixes (9/28/2023):

  • Make image outline semi-transparent
  • Change outline colors to less saturated / less bright
  • Sync colors with orientation box
  • 3D view - parallel projection
  • Fix initial reset-camera by using a composite representation
  • remove custom-events / replace with createEventHook
  • gray out tool bar buttons that don't work with oblique views

A complete list of all vtk-js PRs related to this Volview feature:

Kitware/vtk-js#2908
Kitware/vtk-js#2906
Kitware/vtk-js#2896
Kitware/vtk-js#2895
Kitware/vtk-js#2888
Kitware/vtk-js#2883
Kitware/vtk-js#2850
Kitware/vtk-js#2847

oblique-views-volview.mp4

@netlify
Copy link

netlify bot commented Jun 29, 2023 •
edited
Loading

✅ Deploy Preview for volview-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit a0317ea
🔍 Latest deploy log https://app.netlify.com/sites/volview-dev/deploys/653be7afe684340008796481
😎 Deploy Preview https://deploy-preview-359--volview-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jadh4v jadh4v force-pushed the feat-add-reslicemapper branch 2 times, most recently from ac0af62 to 82ce49f Compare July 17, 2023 17:38
@jadh4v jadh4v mentioned this pull request Jul 17, 2023
Merged
5 tasks
@jadh4v jadh4v force-pushed the feat-add-reslicemapper branch from 82ce49f to 79f7296 Compare July 19, 2023 15:44
@jadh4v jadh4v marked this pull request as ready for review July 19, 2023 15:45
jadh4v
jadh4v commented Jul 19, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
@jadh4v jadh4v force-pushed the feat-add-reslicemapper branch from 79f7296 to d281a34 Compare July 19, 2023 16:15
@jadh4v
Copy link
Collaborator Author

jadh4v commented Jul 19, 2023

@floryst @finetjul Please review

@jadh4v jadh4v force-pushed the feat-add-reslicemapper branch from d281a34 to 8f52381 Compare July 21, 2023 21:14
@floryst
Copy link
Collaborator

floryst commented Jul 21, 2023

I seem to be getting some initial flickering behavior. Repro: load the protate example, then switch to the oblique only view. Moving my mouse over the three views (intersecting with the reslice cursor widget) causes the rendering to change.

@jadh4v jadh4v force-pushed the feat-add-reslicemapper branch 2 times, most recently from 182486f to 5738aa3 Compare July 21, 2023 22:50
@jadh4v
Copy link
Collaborator Author

jadh4v commented Jul 21, 2023

I seem to be getting some initial flickering behavior. Repro: load the protate example, then switch to the oblique only view. Moving my mouse over the three views (intersecting with the reslice cursor widget) causes the rendering to change.

Updated the branch to fix this issue, and some other interaction issues regarding panning operation. Please test again.

@aylward
Copy link
Contributor

aylward commented Jul 22, 2023

This is outstanding! Very responsive on my slow/old laptop. No memory issues or crashing!

Very Nice!

I think we need to tweak the look-and-feel a bit:

  1. The control/indicator lines are too thick. They obscure too much of the data.
  2. The circles for controlling rotation also cover too much data - probably better to move them towards the ends of the lines instead of near the middle where interesting things happen in the data.
  3. I do not think we should use a change in the cursor to indicate function - the item in the scene should clearly indicate function, without having to cursor over it. So, the circle handles to indicate "grab these to rotate" should instead look like rotation widgets that are intuitive.
  4. The rotation cursor is subtle - still primarily an arrow with a small rotation indicator added. The entire cursor should change to a rotation indicator.
  5. It requires mental effort to map which window changes when I move - that is, which frame does the blue line correspond to. The windows need boundaries indicating which line they belong to.
  6. We need a 3D view! I got completely lost in the data when I started changing more than one axis, and I cannot figure out how to get it back to the original view. The 3D view should show the 3 slices and the bounding box of the data. It should also use color or some indicator to make it very intuitive/fast/low-effort map each 3D slice to the corresponding 2D frame. I think we should use color borders like 3D Slicer in the oblique view - I cannot think of anything more intuitive.
  7. We need a reset button - to go back to the original view. I tried, but cannot figure out why, in the image below, the data continues to be tilted.
    image
    Here is how the data looks (nicely square) when I first load it
    image

Again - very nice! Thank you for the contribution!!!

s

@floryst
Copy link
Collaborator

floryst commented Aug 9, 2023

I think a good next step for this is to use this in place of the 2D views, especially paired with Kitware/vtk-js#2887. And a subsequent step would be if the volume is too big for GPU memory (based on some arbitrary threshold and user setting), we can fall back to the ImageMapper.

@jadh4v jadh4v force-pushed the feat-add-reslicemapper branch from eb2d9d8 to 51f2555 Compare August 18, 2023 22:33
@jadh4v jadh4v force-pushed the feat-add-reslicemapper branch 2 times, most recently from 40c18fa to b34d440 Compare August 30, 2023 22:24
finetjul
finetjul reviewed Aug 31, 2023
View reviewed changes
src/shims-vtk.d.ts Outdated Show resolved Hide resolved
@jadh4v jadh4v mentioned this pull request Aug 31, 2023
Merged
5 tasks
@jadh4v jadh4v force-pushed the feat-add-reslicemapper branch from b34d440 to 443ff8a Compare September 1, 2023 15:35
jadh4v added 13 commits October 25, 2023 21:03
@jadh4v
fix(ObliqueView): correct the reset camera behavior
6082711
@jadh4v
feat(resetViews): Add a global button to reset all views in layout
1de788e 
Add a global `reset views` button to reset all currently mounted
views in the layout. The tool button's emitted event can be listened
through newly added custom-events store.
@jadh4v
fix: placement of useVTKCallback
1418039
@jadh4v
fix(resliceCursor): add opacity to handles and lines
fc24649
@jadh4v
feat(resetViews): connect event with TwoView and ThreeView
bc5ed8f
@jadh4v
fix(ResliceCursorWidget): single App-level instance
8f6b0c7 
Move instantiation of vtkResliceCursorWidget to App component
so that only one is created and shared between the oblique views.
@jadh4v
feat(Oblique3D): add 3D view in Oblique layout
ea975fc 
The 3D view re-uses the reslice proxy representations from
the 2D views, and adds an outline for the ImageData for context.
@jadh4v
fix(oblique2D): remove internal camera-reset buttons
36934e5 
Remove the individual camera reset buttons within the 2D viewports.
Since all three oblique 2D views are linked via the reslice cursor
widget, they affect each other anyway. The global camera reset button
does the same thing.
@jadh4v
fix(Oblique): sync outline colors with orientation box
a196e3b 
Update and sync outline colors with orientation box icon
Use parallel projection for 3D oblique view
@jadh4v
fix(Oblique3D): change to use a composite representation proxy
90bc5d1 
Create a new representation proxy called vtkMultiSliceRepresentationProxy,
which will represent the outline box of the source imagedata and the
three vtkResliceRepresentationProxy reslice proxies internally.

Update VtkObliqueThreeView to use this new proxy.

This is required since the architecture of VolView is designed such that
in any given "view", there can be exactly one representation proxy that
can be used for each source type (e.g. vtkImageData).
@jadh4v
fix(resetViews): use createEventHook in place of store for events
744c52b
@jadh4v
fix: import paths and onVTKEvent updates after rebase
0c83389
@jadh4v
fix(ToolStrip): disable tools that don't work in oblique layout
4570aac 
Temporarily disable all tools that do not work in oblique views layout.
These will be re-enabled as support for each tool is added.
@jadh4v jadh4v force-pushed the feat-add-reslicemapper branch from 739be23 to 125a335 Compare October 26, 2023 01:22
@jadh4v jadh4v force-pushed the feat-add-reslicemapper branch from 125a335 to 64cb97c Compare October 26, 2023 01:24
@aylward
Copy link
Contributor

aylward commented Oct 26, 2023

Looking good! Is not crashing. Orientation is fixed.

For me the layout menu still doesn't auto-close when an option has been selected.

My preference is to continue to have sliders for moving between slices in each view - but I realize that this is a bit odd, since the amount you should step when you advance a "slice" in an oblique direction is not well defined. However, I think it would be useful in practice. Could you set the step size to the smallest of the voxel spacing values for every window and then provide sliders similar to the sliders in the quad / axial views?

If you don't have time for the slider or for auto-closing the layout menu now, you can merge as-is, and I can make an issue/feature requests instead.

Overall, this is outstanding!

@aylward aylward self-requested a review October 26, 2023 13:45
aylward
aylward approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@aylward aylward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jadh4v
Copy link
Collaborator Author

jadh4v commented Oct 26, 2023

Looking good! Is not crashing. Orientation is fixed.

For me the layout menu still doesn't auto-close when an option has been selected.

My preference is to continue to have sliders for moving between slices in each view - but I realize that this is a bit odd, since the amount you should step when you advance a "slice" in an oblique direction is not well defined. However, I think it would be useful in practice. Could you set the step size to the smallest of the voxel spacing values for every window and then provide sliders similar to the sliders in the quad / axial views?

If you don't have time for the slider or for auto-closing the layout menu now, you can merge as-is, and I can make an issue/feature requests instead.

Overall, this is outstanding!

Yes, setting the step size to the smallest spacing would be good empirical default I think. I will check if this and the menu closing can be done quickly / easily.

@finetjul
Copy link
Member

Could you set the step size to the smallest of the voxel spacing values for every window and then provide sliders similar to the sliders in the quad / axial views?

For some reason (can't recall why), we used a slightly different approach within ResliceCursorWidget:
https://github.com/Kitware/vtk-js/blob/master/Sources/Widgets/Widgets3D/ResliceCursorWidget/behavior.js#L293-L313

The idea is that 1 step should "change" the current slice. (a tiny voxel spacing in an axis (e.g. Z) should not impact the XZ plane sliding).

I believe the code I shared is wrong because it does not apply the "spacing" on dirProj.

@floryst
Copy link
Collaborator

floryst commented Oct 27, 2023

I'll help address the e2e test failures

@floryst
Copy link
Collaborator

floryst commented Oct 27, 2023

I'm good to merge soon. Anything else @jadh4v?

@jadh4v
Copy link
Collaborator Author

jadh4v commented Oct 27, 2023

I'm good to merge soon. Anything else @jadh4v?

Yes, let's merge. I will do another PR for adding the sliders. It will need some additional work regarding dynamic update of range and slider position (when plane rotates).

@floryst floryst merged commit d879675 into Kitware:main Oct 27, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luciemac luciemac luciemac left review comments

@floryst floryst floryst left review comments

@finetjul finetjul finetjul left review comments

@aylward aylward aylward approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jadh4v @floryst @aylward @finetjul @luciemac