Skip to content

MPR slice w/ volume mapper clips #2113

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
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

MPR slice w/ volume mapper clips #2113

wants to merge 5 commits into from

Conversation

DrewLazzeriKitware
Copy link
Contributor

@DrewLazzeriKitware DrewLazzeriKitware commented Oct 12, 2021 •
edited
Loading

PR and Code Checklist

  • semantic-release commit messages
  • Run npm run reformat to have correctly formatted code

Context

fix #1872
Added a helper for slicing imagemapper with clip planes and removed vtkStyleInteractorMPRSlice.
The included example is similar to the vtkStyleInteractorMPRSlice example.

Changes

  • Documentation and TypeScript definitions were updated to match those changes

@DrewLazzeriKitware DrewLazzeriKitware changed the title Mpr volume mapper Mpr w/ volume mapper clips Oct 12, 2021
@DrewLazzeriKitware DrewLazzeriKitware changed the title Mpr w/ volume mapper clips MPR slice w/ volume mapper clips Oct 12, 2021
@DrewLazzeriKitware DrewLazzeriKitware changed the title MPR slice w/ volume mapper clips (WIP) MPR slice w/ volume mapper clips Oct 12, 2021
jourdain
jourdain reviewed Oct 13, 2021
View reviewed changes
jourdain
jourdain reviewed Oct 13, 2021
View reviewed changes
jourdain
jourdain reviewed Oct 13, 2021
View reviewed changes
jourdain
jourdain reviewed Oct 13, 2021
View reviewed changes
jourdain
jourdain reviewed Oct 13, 2021
View reviewed changes
jourdain
jourdain reviewed Oct 13, 2021
View reviewed changes
finetjul
finetjul reviewed Oct 13, 2021
View reviewed changes
@DrewLazzeriKitware
Copy link
Contributor Author

@floryst There is an example which relies on InteractorStyleMPRSlice, VolumeOutline, which I'm having trouble replicating using sliceHelper. In particular, setUseLabelOutline() seems to have trouble with the way I've approached clipping, which you can see here:
Peek 2021-10-15 12-10

setUseLabelOutline() seems important for that example. Here are images with it and without
Screenshot from 2021-10-15 12-20-21

Screenshot from 2021-10-15 12-18-52

Should I leave InteractorStyleMPRSlice in place for setUseLabelOutline?

@floryst
Copy link
Collaborator

floryst commented Oct 20, 2021

I think that has to do with intermixed volumes. Though why the other example works, I have no idea. In any case, I would recommend not using the label outline example and instead just use one volume.

@agirault agirault self-requested a review November 15, 2021 22:23
@agirault
Copy link
Contributor

In particular, setUseLabelOutline() seems to have trouble with the way I've approached clipping, which you can see here:

I agree with @floryst that we don't need an example with two volumes, though the gif shows that there is a mismatch between work coordinates of those two datasets, so we should probably investigate the source. We should be able to render two aligned images with different orientations using two of those mappers with clipping.

@DrewLazzeriKitware DrewLazzeriKitware force-pushed the mpr-volume-mapper branch 2 times, most recently from d76b628 to e8cf590 Compare November 18, 2021 18:18
@DrewLazzeriKitware DrewLazzeriKitware mentioned this pull request Nov 18, 2021
Open
@DrewLazzeriKitware
Copy link
Contributor Author

I've removed the VolumeOutline example, and captured the issue here: #2169

The failed CI was a timeout. Is that likely just a transient issue unrelated to this change?

I am adding types to this, then should be ready to merge pending CI and review.

@DrewLazzeriKitware DrewLazzeriKitware changed the title (WIP) MPR slice w/ volume mapper clips MPR slice w/ volume mapper clips Nov 18, 2021
@DrewLazzeriKitware
Copy link
Contributor Author

Ok types are added and CI passed this time, please review.

finetjul
finetjul reviewed Nov 19, 2021
View reviewed changes
finetjul
finetjul reviewed Nov 19, 2021
View reviewed changes
finetjul
finetjul reviewed Nov 19, 2021
View reviewed changes
@finetjul
Copy link
Member

The commit should be flagged as BREAKING_CHANGE because it removes existing files/classes

DrewLazzeriKitware
DrewLazzeriKitware commented Nov 23, 2021
View reviewed changes
DrewLazzeriKitware
DrewLazzeriKitware commented Nov 23, 2021
View reviewed changes
@DrewLazzeriKitware
Copy link
Contributor Author

This is updated and ready for review again.

finetjul
finetjul reviewed Nov 23, 2021
View reviewed changes
finetjul
finetjul reviewed Nov 24, 2021
View reviewed changes
@DrewLazzeriKitware
Copy link
Contributor Author

I've left the class checks in AbstractMapper but made them more general. I've also changed the removeClippingPlane method to use instances rather than indexes since this is already a breaking change.

I'm against changing AbstractMapper more in this PR unless it's related to the original issue of MPR though.

@DrewLazzeriKitware DrewLazzeriKitware mentioned this pull request Dec 2, 2021
Merged
@DrewLazzeriKitware
Copy link
Contributor Author

I've split the AbstractMapper changes into this smaller PR: #2186

I'll revisit this once that gets merged.

@DrewLazzeriKitware DrewLazzeriKitware force-pushed the mpr-volume-mapper branch 4 times, most recently from 8d1dc4e to 45cd175 Compare January 17, 2022 21:09
@DrewLazzeriKitware
Copy link
Contributor Author

Since #2186 was merged, I've rebased this on master and it's ready for merge.

finetjul
finetjul reviewed Jan 18, 2022
View reviewed changes
daker
daker suggested changes Jan 18, 2022
View reviewed changes
@finetjul finetjul force-pushed the mpr-volume-mapper branch 2 times, most recently from d891e25 to c41e52c Compare September 10, 2022 11:06
DrewLazzeriKitware and others added 4 commits September 10, 2022 13:12
@DrewLazzeriKitware @finetjul
feat(sliceHelper): add helper for slicing imageMapper with clip planes
446cdaa 
To deprecate vtkInteractorStyleMPRSlice I add a helper using two
clip planes instead. The included example is similar to
vtkInteractorStyleMPRSlice example.

re Kitware#1872
@DrewLazzeriKitware @finetjul
fix(InteractorStyleMPRSlice): deprecate InteractorStyleMPRSlice
079ab65 
BREAKING CHANGE: InteractorStyleMPRSlice has been removed.
vtkSliceHelper achieves the same goal with clip planes instead.

fix Kitware#1872
@DrewLazzeriKitware @finetjul
docs(VolumeMapper): Add vtkSliceHelper types
d4f8ca7
@finetjul
docs(volumeproperty): add warning that gray transfer function is not …
a260c95 
…supported
daker
daker reviewed Sep 12, 2022
View reviewed changes
@finetjul
feat(reslicecursorwidget): add GPU rendering to ResliceCursorWidget
cd364a8 
Some features are still not yet supported:
- [] display bug when no opaque actors (showDebugActors=false) are rendered.
- [] volume mapper should write the plane in depth buffer.
- [] volume mapper should render BEFORE the handles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@finetjul finetjul finetjul left review comments

@agirault agirault Awaiting requested review from agirault

@jourdain jourdain Awaiting requested review from jourdain

+1 more reviewer

@daker daker daker requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate vtkInteractorStyleMPRSlice and use VolumeMapper's clipping planes instead.
6 participants
@DrewLazzeriKitware @floryst @agirault @finetjul @jourdain @daker