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

Improve add_volume and support RectilinearGrid #3794

Merged
merged 9 commits into from
Jan 9, 2023

Conversation

akaszynski
Copy link
Member

@akaszynski akaszynski commented Jan 9, 2023

Fix add_volume

add_volume is broken in several ways. Here's why.

Scalars are not properly added to the dataset.

First, scalars are not added and activated when added to the volume. This is clearly demonstrated in:

>>> import pyvista as pv
>>> from pyvista import examples
>>> vol = pv.UniformGrid(dimensions=(10, 10, 10))
>>> scalars = vol.z
>>> pl = pv.Plotter()
>>> actor = pl.add_volume(vol, scalars=scalars)
>>> pl.show()

2023-01-08 23:50:20.702 (   0.935s) [        DB26E1C0]vtkSmartVolumeMapper.cx:281    ERR| vtkSmartVolumeMapper (0x1e1ad40): Could not find the requested vtkDataArray! 0, 0, -1, 
ERROR:root:Could not find the requested vtkDataArray! 0, 0, -1,

This is because adding an array no longer makes it active, and hasn't for at least a minor release or two. The resulting image will be empty if scalars do not exist, or the scalars will not be properly scaled if they do. For example, scalars do not get scaled properly in this MWE:

import pyvista as pv
from pyvista import examples
pv.set_plot_theme('document')

vol = pv.UniformGrid(dimensions=(20, 20, 20))
vol['scalars'] = vol.z
pl = pv.Plotter()
actor = pl.add_volume(vol)
pl.show()

main branch

tmp

This branch fix/volume_scalars

tmp

Doc issues

This issue shows up with our documentation. For example, in examples/02-plot/volume.py in the last image:

main branch

image
No scaling

This branch fix/volume_scalars

image
Scales as expected

Additional issues

The implementation in main modifies input scalars in-situ, meaning that if scalars are scaled by clim, there will be unintended side effects as the scalars array will be modified

@github-actions github-actions bot added documentation Anything related to the documentation/website bug Uh-oh! Something isn't working as expected. labels Jan 9, 2023
@akaszynski akaszynski marked this pull request as ready for review January 9, 2023 00:27
@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Merging #3794 (9755037) into main (936e7d0) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3794      +/-   ##
==========================================
+ Coverage   94.15%   94.18%   +0.02%     
==========================================
  Files          86       86              
  Lines       18891    18899       +8     
==========================================
+ Hits        17787    17800      +13     
+ Misses       1104     1099       -5     

@banesullivan banesullivan self-requested a review January 9, 2023 01:54
Copy link
Member

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

This is excellent and much needed. Thanks, @akaszynski

Add Support for RectilinearGrid

There is one more thing I would like to add within this PR: support volume rendering for RectilinearGrid. In the type check for UniformGrid, this can be expanded to include RectilinearGrid and a simple check that the mapper type is not 'fixed_point'. After which, that is all that is needed to support RectilinearGrid

All but the vtkFixedPointVolumeRayCastMapper volume mappers support both vtkImageData and vtkRectilinearGrid. So this will work for the thevtkSmartVolumeMapper. vtkGPUVolumeRayCastMapper, and vtkOpenGLGPUVolumeRayCastMapper mappers.

ParaView supports volume rendering for more data types (including StructuredGrid and even UnstructuredGrid in some cases), but in my experience, with mixed results and underwhelming performance (at least on my laptop). Supporting at least both UniformGrid and RectilinearGrid is quite good enough IMO.

Remaining Issue

If there are no active scalars on the mesh, add_volume() fails and errors in a non-informative manner

import pyvista as pv
from pyvista import examples

vol = examples.load_channels()
vol.set_active_scalars(None)

p = pv.Plotter(notebook=0)
p.add_volume(vol, cmap="coolwarm", opacity="linear")
p.show()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In [23], line 2
      1 p = pv.Plotter(notebook=0)
----> 2 p.add_volume(vol, cmap="coolwarm", opacity="linear")
      3 p.show()

File ~/Software/pyvista/pyvista/pyvista/plotting/plotting.py:3614, in BasePlotter.add_volume(self, volume, scalars, clim, resolution, opacity, n_colors, cmap, flip_scalars, reset_camera, name, ambient, categories, culling, multi_colors, blending, mapper, scalar_bar_args, show_scalar_bar, annotations, pickable, preference, opacity_unit_distance, shade, diffuse, specular, specular_power, render, **kwargs)
   3610     opacity_unit_distance = volume.length / (np.mean(volume.dimensions) - 1)
   3612 if scalars is None:
   3613     # Make sure scalars components are not vectors/tuples
-> 3614     scalars = volume.active_scalars.copy()
   3615     # Don't allow plotting of string arrays by default
   3616     if scalars is not None and np.issubdtype(scalars.dtype, np.number):

AttributeError: 'NoneType' object has no attribute 'copy'

@akaszynski
Copy link
Member Author

akaszynski commented Jan 9, 2023

  • If there are no active scalars on the mesh, add_volume() fails and errors in a non-informative manner
  • Add Support for pyvista.RectilinearGrid

@banesullivan
Copy link
Member

I just pushed support for RectilinearGrid, please review/double check

@banesullivan
Copy link
Member

Let's make codecov happy, then I think this is good to merge

@akaszynski
Copy link
Member Author

We have the most unforgiving CI and I love it.

Copy link
Member

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM

We may want to add volume support for other data types like UnstructuredGrid via the vtkUnstructuredGridVolumeMapper but that should come in a separate PR as it would be a more significant change

@banesullivan
Copy link
Member

I need to merge this to wrap up #3318

@banesullivan banesullivan changed the title Fix add_volume Improve add_volume and support RectilinearGrid Jan 9, 2023
@banesullivan banesullivan merged commit 803d3a3 into main Jan 9, 2023
@banesullivan banesullivan deleted the fix/volume_scalars_casting branch January 9, 2023 15:00
@banesullivan banesullivan mentioned this pull request Feb 1, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected. documentation Anything related to the documentation/website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants