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.

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

Multi volume rendering #966

Closed
wants to merge 4 commits into from

Conversation

@cpinter
Copy link

commented Jun 7, 2018

This PR implements real multi-volume support in Slicer, using the vtkMultiVolume actor. The actor is not fully implemented, so it has limitations, such as no shading, no cropping by ROI, no per-volume visibility. For a few of these, workarounds have been implemented. Hence, the options is currently marked experimental in the drop-down list.

Additionally, I refactored the volume rendering options so that many of them now are in the view node instead of the display node, and added those to Application Settings.

The PR contains four commits for easier review, and I think it makes sense to keep them separate when integrating.

cpinter added some commits May 1, 2018

ENH: Added real MultiVolume volume rendering option
Added vtkMultiVolume driven volume rendering method as third option.

Two blocking issues with vtkMultiVolume make it unusable in Slicer as is. Workarounds have been applied:
1. https://gitlab.kitware.com/vtk/vtk/issues/17325: Added dummy single-voxel all-transparent volume so that all user transforms are correctly handled
2. https://gitlab.kitware.com/vtk/vtk/issues/17302: Volume connections removed from actor and mapper so that it is actually hidden from the view when visibility checkbox is turned off
ENH: Moved volume rendering properties from display node to 3D view node
- Moved properties from vol.ren. display node to view node: RaycastTechnique, ExpectedFPS, GPUMemorySize, EstimatedSampleDistance (renamed to VolumeRenderingOversamplingFactor, as the former name was misleading as it does not specify sample distance but an oversampling factor), PerformanceControl (renamed to VolumeRenderingQuality, as it is not directly related to performance, and even the enum was called Quality), SurfaceSmoothing (renamed to VolumeRenderingSurfaceSmoothing). Reasons:
  1. Setting these options differently for volumes is not useful, however setting some of them per view could be useful
  2. The new multi volume rendering method has only one common mapper, and some properties cannot be set per volume
  3. It would be nice to expose these properties in Application Settings, to be able to define application defaults. For this, the view node is a better place
Important note: this commit breaks backwards compatibility in terms of loading a scene containing special volume rendering settings, so those settings will not be loaded correctly. However Slicer 4.10 and soon after Slicer 5 is coming up, so this might be a good time to make this step.

- Moved GetSampleDistance function from displayable manager to vtkMRMLVolumeRenderingDisplayNode
- Fixed bugs related to multi-volume rendering
- Fixed vtkSlicerVolumeRenderingLogicTest and fixed todo in vtkMRMLVolumeRenderingDisplayable by adding utility functions in the displayable manager
- Style related changes in view node and slice node classes to keep formatting consistent
ENH: Added volume rendering options to Application Settings
In addition to the current rendering method and GPU memory size options, the following were added:
- Default quality
- Default interactive speed
- Default surface smoothing

@cpinter cpinter requested review from pieper, jcfr and lassoan Jun 7, 2018


public slots:
void setDefaultRenderingMethod(const QString& method);
void setDefaultQuality(const QString& quality);
void setDefaultInteractiveSpeed(int interactiveSpeed);
void setDefaultSurfaceSmoothing(bool surfaceSmoothing);
void setGPUMemory(int gpuMemory);
void setGPUMemory(QString gpuMemory);

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

const QString& gpuMemory


public slots:
/// Set currently selected GPU memory in MB or percentage (% value between 0-1)
void setCurrentGPUMemory(double memory);

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

This could also be a property


/// Return total memory available in the GPU
Q_INVOKABLE int totalGPUMemoryInMB()const;
/// Return currently selected GPU memory in MB or percentage (% value between 0-1)

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

How to choose one vs the other ?

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

Should currentGPUMemory be renamed to something like allocatedGPUMemory ?

This comment has been minimized.

Copy link
@cpinter

cpinter Jun 8, 2018

Author

currentGPUMemory is what the user selected. In the analogy of currentNode etc.


class qSlicerGPUMemoryComboBoxPrivate;

class Q_SLICER_MODULE_VOLUMERENDERING_WIDGETS_EXPORT qSlicerGPUMemoryComboBox

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

Thinking more about this, I wonder if this widget should simply be renamed qSlicerMemoryComboBox (or even ctkMemoryComboBox). Basically a widget allowing to either select existing preset, and also add/remove other ones.

The code specific to VTK would then be move outside of the widget.

This comment has been minimized.

Copy link
@cpinter

cpinter Jun 8, 2018

Author

Sounds reasonable. Can we do it as a separate step later?

q->setEditable(true);
q->lineEdit()->setValidator( new QRegExpValidator(this->MemoryRegExp, q));
q->addItem(DefaultText);
//q->addItem(q->tr("25 %")); //TODO: Uncomment when totalGPUMemoryInMB works

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

I think the widget should have two "modes" based on the value of availableMemory properties.

  • if the property availableMemory is set to > 0, the widget could remove the irrelevant values and prepend the %.
  • if the property availableMemory set to <=0, a fixed list of choice without the %

This comment has been minimized.

Copy link
@cpinter

cpinter Jun 8, 2018

Author

I don't yet understand. I think we should do a refactoring of this widget later after we agree on details. This widget works for this very specific case in the two places it's used (Settings and VolRen module), but it is pretty much WIP considering the % mode doesn't work at all, and the comments you had earlier about generalizing.

/// Tracking GPU memory size (in MB), not saved into scene file
/// because different machines may have different GPU memory
/// values.
/// A value of 0 indicates to use the default value in the settings

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

For now, I don't think we should associate this property with the view.

What about adding an availableGPUMemory property to the vtkMRMLApplicationLogic or the qSlicerCoreApplication


virtual vtkMRMLNode* CreateNodeInstance() VTK_OVERRIDE;

// Description:

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

May be removed // Description:

@@ -165,6 +200,11 @@ vtkMRMLVolumeRenderingDisplayableManager::vtkInternal::vtkInternal(vtkMRMLVolume
, OriginalDesiredUpdateRate(0.0) // 0 fps is a special value that means it hasn't been set
, Interaction(0)
{
this->MultiVolumeActor = vtkSmartPointer<vtkMultiVolume>::New();
this->MultiVolumeMapper = vtkSmartPointer<vtkGPUVolumeRayCastMapper>::New();
// Set GPU mapper to the multi-volume actor. Both objects are only used for GPU ray casting

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

May be worth clarifying this comment

This comment has been minimized.

Copy link
@cpinter

cpinter Jun 8, 2018

Author

I added a short description about how multi-volume is different from the others, and an ascii diagram (the same thing I had in front of me on a piece of paper while implementing, it's very helpful). I think it should be enough.

vtkMRMLMultiVolumeRenderingDisplayNode::SafeDownCast(displayNode);
vtkGPUVolumeRayCastMapper* gpuMapper = vtkGPUVolumeRayCastMapper::SafeDownCast(mapper);

//TODO: All the multi-volume display nodes must have the same settings that concern the mapper

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

May be worth clarifying this comment

@@ -27,6 +27,11 @@
// MRML DisplayableManager includes
#include <vtkMRMLAbstractThreeDViewDisplayableManager.h>

//TODO: For testing, remove

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

Then, should these lines be removed ?

This comment has been minimized.

Copy link
@cpinter

cpinter Jun 8, 2018

Author

Good catch, thanks!

void operator=(const vtkMRMLMultiVolumeRenderingDisplayNode&);

/* techniques in GPU ray cast
* 0: composite with directional lighting (default)

This comment has been minimized.

Copy link
@jcfr

jcfr Jun 8, 2018

Member

What about adding a comment referencing the associated enum

\sa vtkMRMLXXXX:NameOfEnum
@jcfr

This comment has been minimized.

Copy link
Member

commented Jun 8, 2018

Thanks @cpinter , outstanding 🥇 👍

@cpinter cpinter force-pushed the cpinter:multi-volume-rendering branch from 5f59413 to 50d4d16 Jun 8, 2018

@cpinter

This comment has been minimized.

Copy link
Author

commented Jun 8, 2018

Thanks, @jcfr! I amended the last commit with changes according to your suggestions. I left the GPU memory combobox alone for now, as I feel it calls for major changes, which are better done later after discussions (perhaps even in las Palmas).

ENH: Added GPU memory combobox
Added new widget to set GPU memory both in Application Settings and in Volume Rendering module.

Note: Options to specify GPU memory as a percentage of the total memory have been added, but getting the GPU memory does not work. I asked for advice in this thread
http://vtk.1045678.n5.nabble.com/vtkGPUInfoList-does-not-work-with-OpenGL2-td5747672.html

@cpinter cpinter force-pushed the cpinter:multi-volume-rendering branch from 50d4d16 to 10072cb Jun 8, 2018

@cpinter

This comment has been minimized.

Copy link
Author

commented Jun 11, 2018

If there are no further comments and requests, then I'll integrate the branch this afternoon. There are no apparent bugs, so if the PR looks good based on the commit messages, then it should be good to go for the nightly. Thanks everyone!

@jcfr

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

integrate the branch this afternoon

👍

While trying to build locally, I rebased the topic, split some of the style change into their own commit, change the commit style to imperative and published it here: master...jcfr:multi-volume-rendering-rebased

Could you integrate this rebased topic instead ?

@cpinter

This comment has been minimized.

Copy link
Author

commented Jun 11, 2018

Thanks, Jc! I integrated your branch just now.

@cpinter cpinter closed this Jun 11, 2018

phcerdan added a commit to phcerdan/Slicer that referenced this pull request Jun 16, 2018

ENH: Update LibArchive. Fix gcc8 warnings.
The gcc8 warnings were generating build errors at Debug mode,
because the aggressive Werror flag in LibArchive.

git shortlog 98a695399e8e7420635a5448aecde8b0a82fb83a..d7dea508

Arshan Khanifar (7):
      flush pending chdirs prior to processing mtree files
      Test case for using -C in conjunction with an @mtree format file
      corrected licensing, removed unnecessary variables
      build fail fix, simplified logic
      absolute path fix
      alphabetical order
      put the absolute path in quotes in case user's directory has spaces in it

BenjaminTrapani (9):
      Fix bsdtar test compilation on windows with platform toolset 141
      Fix missing hardlink source test windows
      Fix version format to match unit test
      Copy reference resource to binary directory to make it possible to debug tests via visual studio cmake
      Add space after version to fix all unit tests besides sparse tests
      Fix sparse tests windows
      Revert "Copy reference resource to binary directory to make it possible to debug"
      Fix indentation to match rest of file
      Fix warnings treated as errors during x64 build

Bernard Spil (1):
      fix build with LibreSSL 2.7

Brad King (1):
      Do not use nanosecond file time APIs on macOS < 10.13

Chris Roberts (1):
      Check libgnu for xattr functions on Haiku

Colin Percival (1):
      Avoid overflow when reading corrupt cpio archive

Ed Maste (1):
      ensure ar strtab is null terminated

Graham Percival (1):
      Spelling fixes

Joerg Sonnenberger (12):
      Don't call wmemmove if size is zero.
      Revert addition of assert.
      Do something sensible for empty strings to make fuzzers happy.
      Match full strings, not just prefixes.
      Don't allow sparse mapping entry to pass beyond 63bit.
      Place a limit on the mtree line length to make fuzzers happy.
      Ensure that the AES extension header is large enough.
      Avoid a read off-by-one error for UTF16 names in RAR archives.
      Fix case in comment.
      Match archive.h for la_int64_t vs int64_t
      Fix archive freeing bug in bsdcat.
      Check size of the extended time field in zip archives

John Starks (2):
      Windows: set errno on CreateFileW failure
      zip: Allow backslash as path separator

Martin Matuska (8):
      Merge pull request Slicer#926 from emaste/libarchive
      archive_write_ar_data(): replace strncpy() with memcpy()
      bsdtar manpage: unify descriptions of compression options
      Fix build on FreeBSD with NFS4 ACLs without ACL_ENTRY_INHERITED
      Remove lzop package from Darwin testing
      Unbreak write test for libzstd 1.3.4
      Merge pull request Slicer#973 from o3dwade:master
      Add some gzip header tests including RFC 1952 compression level flag

Martin Matuška (1):
      Merge pull request Slicer#968 from trollixx/cmake-lz4-option

Mike Frysinger (1):
      delete dead ppmd7 alloc callbacks (Slicer#893)

Oleg Shparber (2):
      cmake: Add ENABLE_LZ4 option
      cmake: Fix missing override for LZO2_FOUND

Omar Farooq (1):
      Adding compression level support for gzip

Pablo Hernandez-Cerdan (1):
      Fix gcc 8 warning about strncpy

Paul Spangler (2):
      archive_write_disk_{posix,windows}.c: Don't modify attributes for existing directories when ARCHIVE_EXTRACT_NO_OVERWRITE is set
      Simplify test case per PR comments

Sean Purcell (14):
      Add Zstandard read support
      Add Zstandard write support
      Small zstd fixes
      Add zstd test suite
      Whitespace fixes
      Fix zstd reader and change variable scopes
      Fix compile errors with cmake and when zstd isn't present
      Skip zstd write tests when library not present
      Don't try to use libzstd versions without streaming API
      Build with zstd on TravisCI on OS X
      zstd: Address comments by @terrelln
      zstd: Don't bid on skippable frames
      Fix alphabetical order, other small fixes
      Fix zstd memory allocation and null checks

Tim Kientzle (19):
      Libarchive 3.3.3dev
      Issue Slicer#924: fix capitalization of bcrypt.h header
      Merge pull request Slicer#929 from cperciva/master
      Merge pull request Slicer#930 from Tarsnap/spelling-upload
      Merge pull request Slicer#934 from tonytheodore/cflags-private
      Merge pull request Slicer#905 from iburinoc/zstd
      Merge pull request Slicer#943 from jrmarino/master
      Issue Slicer#947: Reference archive_write_set_options in archive_write.3
      Merge pull request Slicer#912 from korli/libnetwork
      Merge pull request Slicer#961 from zweger/master
      Merge pull request Slicer#964 from legnaleurc/fix_fallthrough
      Merge pull request Slicer#970 from jstarks/zip_path_separator
      Merge pull request Slicer#966 from jstarks/windows_disk_read_error
      Merge pull request Slicer#962 from spanglerco/existing-dir-owner
      Merge pull request Slicer#986 from BenjaminTrapani/windows-fixes
      Merge pull request Slicer#989 from trollixx/cmake-fix-lzo2
      Merge pull request Slicer#993 from ArshanKhanifar/issue-991
      Merge pull request Slicer#1005 from Sp1l/master
      Merge pull request Slicer#1024 from phcerdan/fix_strncpy_warning

Tony Theodore (1):
      libarchive.pc.in: add Cflags.private for static linking

Wei-Cheng Pan (1):
      Fix -Werror=implicit-fallthrough= for GCC 7.

Zack Weger (1):
      Clear the ZIP format name before constructing a new one, so it isn't appended to the format name of the previous entry

jrmarino (1):
      Recognize ".tzst" extension as ".tar.zst"

ngie (1):
      Fix a potential NULL pointer dereference of `tar` in archive_read_support_format_tar when HAVE_COPYFILE_H is defined (Slicer#959)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.