Skip to content

ENH: Update VTK backporting selected Rendering, OpenVR and OpenXR patches#7487

Merged
jcfr merged 1 commit intoSlicer:mainfrom
jcfr:update-vtk-backport-selected-rendering-and-vr-patches
Dec 18, 2023
Merged

ENH: Update VTK backporting selected Rendering, OpenVR and OpenXR patches#7487
jcfr merged 1 commit intoSlicer:mainfrom
jcfr:update-vtk-backport-selected-rendering-and-vr-patches

Conversation

@jcfr
Copy link
Member

@jcfr jcfr commented Dec 18, 2023

This update focuses on backporting specific commits related to the Rendering, OpenVR, and OpenXR modules, aiming to support the ColorizeVolume module in the Sandbox extension and integrate OpenXR and OpenXRRemoting capabilities in the SlicerVirtualReality extension.

The selection of backported commits was curated by reviewing changes in the following VTK modules:

  • Rendering/Core
  • Rendering/OpenGL2
  • Rendering/Volume
  • Rendering//VolumeOpenGL
  • Rendering/VR
  • Rendering/OpenVR
  • Rendering/OpenXR
  • Rendering/OpenXRRemoting

In addition to these, the backport includes the following improvements:

  • Addition of the ForceVerticalTitle option to vtkScalarBarActor. See Slicer/VTK@c8cfd917 and upstream MR-103061

  • Enhancement to vtkOpenGLPolyDataMapper by adding line opacity control. See Slicer/VTK@92021a6bc and upstream MR-103072

During testing, a performance regression in the volume rendering module was identified, attributable to Kitware/VTK@cff3d9a60 (vtkRenderer: Fix prop picking in GetZ method). Consequently, this commit has been reverted from the list of backports. For further details, refer to MR-103913 and VTK issue 185034, which it was originally intended to address.

For reference, the VTK changes listed below were integrated into our Slicer/VTK fork through:

List of VTK changes:

$ git shortlog 46201478cd..d5d688b473 --no-merges
Alexy Pellegrini (8):
      [Backport] Add missing SIZE_HINT on vtkImageActor::GetDisplayBounds
      [Backport] Split vtkWin32OpenGLDXRenderWindow initialization
      [Backport] Enable switching from remoting XR runtime to default XR runtime dynamically
      [Backport] Add a utility function to query OpenXR instance version
      [Backport] OpenXR: Add missing EndInitialize in QueryInstanceVersion
      [Backport] VR: enable interaction methods to be overrideable
      [Backport] VR: Add VTKIS_USCALE support in VR interactor style
      [Backport] XR: Move device pose update in interactor

Ben Boeckel (1):
      [Backport] Rendering/VR: make `glew` a public dependency

Franck HOUSSEN (1):
      [Backport] Document FrameBlitMode in vtkOpenGLRenderWindow.

Jaswant Panchumarti (4):
      [Backport] SurfaceWithEdges: Fix mixed geometry rendering
      [Backport] Apply 1 suggestion(s) to 1 file(s)
      [Backport] Glyph3DMapper: Fix display attribute inheritance logic
      [Backport] ReadPixels: Check for errors only when VTK is configured to report errors

Jean-Christophe Fillion-Robin (6):
      Revert "[Backport MR-10450] OpenXRRemoting: Fix windows build using XrCheckOutput instead of XrCheckError"
      Revert "[Backport MR-10450] OpenXR: Improve XrCheckOutput API fixing constness of level parameter"
      Revert "[Backport MR-10449] OpenXRRemoting: Streamline integration as plugin"
      [Backport] OpenXRRemoting: Streamline integration as plugin
      [SlicerVirtualReality] ENH: Update OpenVR json binding to map right and left trigger with triggeraction
      Revert "[Backport] vtkRenderer: Fix prop picking in GetZ method"

Julien Fausty (3):
      [Backport] ABI: fix vtkValueFromString
      [Backport] ABI: fix vtkCompositeSurfaceLICMapper
      [Backport] ABI: fix vtkOpenGLBatchedPolyDataMapper

Julien Schueller (1):
      [Backport] vtkWin32OpenGLDXRenderWindow: Fix missing include

Lucas Gandel (16):
      [Backport] Support any texture with interpolate scalars
      [Backport] Add tests for texture coloring with interpolated scalars
      [Backport] Support both texture and interpolated scalars in composite polydata mapper
      [Backport] Remove the TestBlockOpacity test
      [Backport] Support 4-component RGBA inputs for multivolume
      [Backport] Test multivolume with RGBA inputs
      [Backport] Update vtkMultiVolume doc to mention RGBA support
      [Backport] Test vtkMultiVolume with RGBA input and gradient opacity function
      [Backport] Allow volumes to write to the depth buffer
      [Backport] Add VTK::ComputeLighting::Exit tag for volume shader replacement
      [Backport] Allow to specify other formats for the ssao pass depth texture
      [Backport] Allow vtkOpenGLRenderPass subclasses to add information keys efficiently
      [Backport] Support SSAO for volumes
      [Backport] Expose volume opacity threshold in SSAO pass
      [Backport] Test SSAO pass with GPU raycast volume mapper
      [Backport] OpenXR: Support depth buffer composition layer

Lucas Givord (1):
      [Backport] vtkOpenGLPolyDataMapper: add line opacity control

LucasGandel (1):
      [Backport] Support both texture and interpolated scalars in GLES polydata mapper

Mathieu Westphal (2):
      [Backport] Dolly3D: Remove incorrect check in Y axis only
      [Backport] OpenXRRemoting: Using XrCheckOutput

Michael Migliore (4):
      [Backport] Use index array for point gaussians
      [Backport] Use quad instead of triangle to bound the gaussian splat
      [Backport] Rename scale option and scale in the shader
      [Backport] Fix normal mapping with edge visibility

Mohamed Mssaouri (2):
      [Backport] Add teleportation to VR
      [Backport] Move teleportation to interaction modes

Paulo Roberto Moura de Carvalho (1):
      [Backport] vtkRenderer: Fix prop picking in GetZ method

Robert Lexmann (1):
      [Backport] RenderingOpenGL2: Fixed final blending of vtkOrderIndependentTranslucentPass.

Sankhesh Jhaveri (1):
      [Backport] rendering: Do not offset vertices for drawing polygon edges

Scott Wittenburg (1):
      [Backport] rendering: Add ForceVerticalTitle option to vtkScalarBarActor

Thibault Bruyère (1):
      [Backport] Add volume clipping to vtkVRInteractorStyle

Tiffany Chhim (1):
      [Backport] VR Renderer: Add cross markers at tip of controllers

Footnotes

  1. https://gitlab.kitware.com/vtk/vtk/-/merge_requests/10306

  2. https://gitlab.kitware.com/vtk/vtk/-/merge_requests/10307

  3. https://gitlab.kitware.com/vtk/vtk/-/merge_requests/10391

  4. https://gitlab.kitware.com/vtk/vtk/-/issues/18503

@jcfr jcfr force-pushed the update-vtk-backport-selected-rendering-and-vr-patches branch from 817e310 to ecd8b30 Compare December 18, 2023 04:57
…ches

This update focuses on backporting specific commits related to the
Rendering, OpenVR, and OpenXR modules, aiming to support the ColorizeVolume
module in the Sandbox extension and integrate OpenXR and OpenXRRemoting
capabilities in the SlicerVirtualReality extension.

The selection of backported commits was curated by reviewing changes
in the following VTK modules:
- Rendering/Core
- Rendering/OpenGL2
- Rendering/Volume
- Rendering//VolumeOpenGL
- Rendering/VR
- Rendering/OpenVR
- Rendering/OpenXR
- Rendering/OpenXRRemoting

In addition to these, the backport includes the following improvements:

- Addition of the ForceVerticalTitle option to vtkScalarBarActor
  See Slicer/VTK@c8cfd917 and upstream MR-10306[^1]

- Enhancement to vtkOpenGLPolyDataMapper by adding line opacity control
  See Slicer/VTK@92021a6bc and upstream MR-10307[^2]

During testing, a performance regression in the volume rendering module
was identified, attributable to Kitware/VTK@cff3d9a60 (vtkRenderer: Fix
prop picking in GetZ method). Consequently, this commit has been reverted
from the list of backports. For further details, refer to MR-10391[^3] and
VTK issue 18503[^4], which it was originally intended to address.

[^1]: https://gitlab.kitware.com/vtk/vtk/-/merge_requests/10306
[^2]: https://gitlab.kitware.com/vtk/vtk/-/merge_requests/10307
[^3]: https://gitlab.kitware.com/vtk/vtk/-/merge_requests/10391
[^4]: https://gitlab.kitware.com/vtk/vtk/-/issues/18503


List of VTK changes:

```
$ git shortlog 46201478cd..d5d688b473 --no-merges
Alexy Pellegrini (8):
      [Backport] Add missing SIZE_HINT on vtkImageActor::GetDisplayBounds
      [Backport] Split vtkWin32OpenGLDXRenderWindow initialization
      [Backport] Enable switching from remoting XR runtime to default XR runtime dynamically
      [Backport] Add a utility function to query OpenXR instance version
      [Backport] OpenXR: Add missing EndInitialize in QueryInstanceVersion
      [Backport] VR: enable interaction methods to be overrideable
      [Backport] VR: Add VTKIS_USCALE support in VR interactor style
      [Backport] XR: Move device pose update in interactor

Ben Boeckel (1):
      [Backport] Rendering/VR: make `glew` a public dependency

Franck HOUSSEN (1):
      [Backport] Document FrameBlitMode in vtkOpenGLRenderWindow.

Jaswant Panchumarti (4):
      [Backport] SurfaceWithEdges: Fix mixed geometry rendering
      [Backport] Apply 1 suggestion(s) to 1 file(s)
      [Backport] Glyph3DMapper: Fix display attribute inheritance logic
      [Backport] ReadPixels: Check for errors only when VTK is configured to report errors

Jean-Christophe Fillion-Robin (6):
      Revert "[Backport MR-10450] OpenXRRemoting: Fix windows build using XrCheckOutput instead of XrCheckError"
      Revert "[Backport MR-10450] OpenXR: Improve XrCheckOutput API fixing constness of level parameter"
      Revert "[Backport MR-10449] OpenXRRemoting: Streamline integration as plugin"
      [Backport] OpenXRRemoting: Streamline integration as plugin
      [SlicerVirtualReality] ENH: Update OpenVR json binding to map right and left trigger with triggeraction
      Revert "[Backport] vtkRenderer: Fix prop picking in GetZ method"

Julien Fausty (3):
      [Backport] ABI: fix vtkValueFromString
      [Backport] ABI: fix vtkCompositeSurfaceLICMapper
      [Backport] ABI: fix vtkOpenGLBatchedPolyDataMapper

Julien Schueller (1):
      [Backport] vtkWin32OpenGLDXRenderWindow: Fix missing include

Lucas Gandel (16):
      [Backport] Support any texture with interpolate scalars
      [Backport] Add tests for texture coloring with interpolated scalars
      [Backport] Support both texture and interpolated scalars in composite polydata mapper
      [Backport] Remove the TestBlockOpacity test
      [Backport] Support 4-component RGBA inputs for multivolume
      [Backport] Test multivolume with RGBA inputs
      [Backport] Update vtkMultiVolume doc to mention RGBA support
      [Backport] Test vtkMultiVolume with RGBA input and gradient opacity function
      [Backport] Allow volumes to write to the depth buffer
      [Backport] Add VTK::ComputeLighting::Exit tag for volume shader replacement
      [Backport] Allow to specify other formats for the ssao pass depth texture
      [Backport] Allow vtkOpenGLRenderPass subclasses to add information keys efficiently
      [Backport] Support SSAO for volumes
      [Backport] Expose volume opacity threshold in SSAO pass
      [Backport] Test SSAO pass with GPU raycast volume mapper
      [Backport] OpenXR: Support depth buffer composition layer

Lucas Givord (1):
      [Backport] vtkOpenGLPolyDataMapper: add line opacity control

LucasGandel (1):
      [Backport] Support both texture and interpolated scalars in GLES polydata mapper

Mathieu Westphal (2):
      [Backport] Dolly3D: Remove incorrect check in Y axis only
      [Backport] OpenXRRemoting: Using XrCheckOutput

Michael Migliore (4):
      [Backport] Use index array for point gaussians
      [Backport] Use quad instead of triangle to bound the gaussian splat
      [Backport] Rename scale option and scale in the shader
      [Backport] Fix normal mapping with edge visibility

Mohamed Mssaouri (2):
      [Backport] Add teleportation to VR
      [Backport] Move teleportation to interaction modes

Paulo Roberto Moura de Carvalho (1):
      [Backport] vtkRenderer: Fix prop picking in GetZ method

Robert Lexmann (1):
      [Backport] RenderingOpenGL2: Fixed final blending of vtkOrderIndependentTranslucentPass.

Sankhesh Jhaveri (1):
      [Backport] rendering: Do not offset vertices for drawing polygon edges

Scott Wittenburg (1):
      [Backport] rendering: Add ForceVerticalTitle option to vtkScalarBarActor

Thibault Bruyère (1):
      [Backport] Add volume clipping to vtkVRInteractorStyle

Tiffany Chhim (1):
      [Backport] VR Renderer: Add cross markers at tip of controllers
```
@jcfr jcfr force-pushed the update-vtk-backport-selected-rendering-and-vr-patches branch from ecd8b30 to 8032014 Compare December 18, 2023 04:58
@jcfr
Copy link
Member Author

jcfr commented Dec 18, 2023

Performance Regression Analysis

Background: Details about the git bisect process leading to the identification of Kitware/VTK@cff3d9a60 (vtkRenderer: Fix prop picking in GetZ method) as the culprit are available at Slicer/VTK#46 (comment).

Analysis: While in the short term reverting the commit allowed to recover the original performance, further analysis helped understand that our use of vtkRenderer:GetZ() happens with every event.

vtkMRMLThreeDViewInteractorStyle::DelegateInteractionEventToDisplayableManagers
  -> `vtkMRMLThreeDViewInteractorStyle::QuickPick`
    -> this->QuickPicker->Pick  (vtkWorldPointPicker::Pick)
      -> vtkRenderer:GetZ()

References:

  • vtkWorldPointPicker.h1
  • vtkWorldPointPicker.cxx2
  • Kitware/VTK@cff3d9a60 (vtkRenderer: Fix prop picking in GetZ method)

Footnotes

  1. https://github.com/Slicer/VTK/blob/slicer-v9.2.20230607-1ff325c54-2/Rendering/Core/vtkWorldPointPicker.h

  2. https://github.com/Slicer/VTK/blob/slicer-v9.2.20230607-1ff325c54-2/Rendering/Core/vtkWorldPointPicker.cxx#L27-L93

@lassoan
Copy link
Contributor

lassoan commented Dec 18, 2023

The change in Kitware/VTK@cff3d9a60 is not very useful, as it is extremely expensive to instantiate a new hardware picker for every position. This is also something that anyone can easily do outside the renderer and actually do that picker more efficiently (e.g., cache the Z buffer content). I think the original behavior of vtkRenderer::GetZ should be restored, and its documentation should be amended with a note that the method should be called at the right time (when the Z buffer is not cleared). The proposed GetZ implementation might be considered to be added to the vtkRenderer as some convenience function if Paulo Carvalho found it useful, but it should be reviewed, because for example creating a new hardware selector from scratch for every point position looks terribly wasteful, especially when the renderer already has a cached selector (and maybe vtkRenderer::PickProp could be enough). I think overall what happened is not Paulo's fault but that VTK deveopers did not review his proposed change for a really long time and then just rushed a merge without anyone really thinking about the implications of this change. Probably the change looked insignificant because nobody realized that the method was heavily used in vtkWorldPointPicker and vtkSelectVisiblePoints (and it worked correctly there because the method was used at the right time).

Copy link
Contributor

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

I could not review all the VTK changes in detail but overall it looked good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants