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

Add vtkAlgorithm support to add_mesh for pipelining #3318

Merged
merged 44 commits into from
Jan 13, 2023
Merged

Conversation

banesullivan
Copy link
Member

@banesullivan banesullivan commented Sep 14, 2022

These changes add better support for pipelining with a PyVista Plotter.

Often, VTK power-users want to leverage PyVista's convenient plotting API while creating custom VTK algorithm pipelines that can be dynamically updated. Up until now, PyVista's plotting API has been entirely data-centric, leaving out support for VTK's input/output connection mechanisms. Since PyVista is just VTK, we can pretty easily tap into the input/output connection infrastructure to support pipelining with the Plotter API.

At the core of the add_mesh method, we don't really need to do much with the passed mesh object itself except pass it to the mapper (we set active scalars or maybe extract a surface in some cases, otherwise, not much). Since the vtkMapper used in add_mesh already supports the pipeline infrastructure, we can add a few lines of code to check if the passed object is a vtkAlgorithm and if so, hook up the input/output connections rather than setting inputs as explicit data objects that are subject to change. (Note that I had to implement a custom algorithm to dynamically set the active scalars as well).

These modifications can potentially improve any workflows that need to modify the data being plotted (think animations or adjusting the parameters of a filter).

Here is an example of what this enables: use a vtkConeSource algorithm as th input, add a slider to dynamically set it's Resolution, and then perform a box clip.

import vtk

import pyvista as pv

algo = vtk.vtkConeSource()

def update_resolution(value):
    res = int(value)
    algo.SetResolution(res)
    return

p = pv.Plotter()
p.add_mesh_clip_box(algo, color='red')
p.add_slider_widget(update_resolution, [5, 100], title='Resolution')
p.show()
Screen.Recording.2022-09-13.at.9.12.04.PM.mov

By supporting vtkAlgorithm objects in the API, we can hook into VTK pipelining fairly easily with massive opportunities.

cc @jourdain

To do

  • Find other BasePlotter methds that accept mesh objects and add support for vtkAlgorithms where appropriate
    • add_points
    • add_point_labels (not doing add_point_scalar_labels as that's a special case for now)
    • add_silhouette
    • Widget add_mesh_* helpers
  • Improve add_mesh's handling of vtkAlgorithms
    • Setting the active scalars when a vtkAlgorithm is passed may not work if the mesh generated by GetOutput() changes (very likely)
    • Address TODO comments added in code
    • Address TypeErrors added in code
  • Write an example using basic VTK sources and filter pipelines (more examples with custom algorithms will need to come in a later PR)
  • Gracefully handle PointSet issues
  • Fix issue with cell_ids scalar bar appearing when using add_ids_algorithm but mesh not being colored
  • Fix add_mesh_cip_box(alg, invert=True) with algorithms

Known Issues

  • Volume rendering is not yet supported -- this will need to come in a follow up PR that refactors volume rendering significantly
  • Composite/MultiBlock datasets will have to come later
  • vtkPointSet/pyvista.PointSet is incompatible with VTKPythonAlgorithmBase. This has mostly been addressed in this PR with the custom algorithms added. See https://gitlab.kitware.com/vtk/vtk/-/issues/18771

@github-actions github-actions bot added the enhancement Changes that enhance the library label Sep 14, 2022
@MatthewFlamm
Copy link
Contributor

I haven't read this in detail yet, but it might be useful to have Algorithm classes for this. We could reuse them inside the filter methods too. This would further bring PyVista closer to vtk while still providing the ease of use.

This is a lot of work, but I'm not sure if it would influence the design of this PR. I'm not suggesting you include it here

algo = pv.ConeSource()

def update_resolution(value):
    res = int(value)
    algo.set_resolution(res)
    return

p = pv.Plotter()
p.add_mesh_clip_box(algo, color='red')
p.add_slider_widget(update_resolution, [5, 100], title='Resolution')
p.show()

@banesullivan
Copy link
Member Author

banesullivan commented Sep 14, 2022

It's been a while since I've overridden VTK algorithms in Python, but if I remember correctly, it is not as easy as wrapping the data classes as the methods of the VTK algorithm class have to be executed on the C++ side.

This is what PVGeo is all about by overriding the vtk.util.vtkAlgorithm.VTKPythonAlgorithmBase class. I believe things may have changed in the last couple of years since I wrote PVGeo, but the gist is probably the same. The AlgorithmBase class in PVGeo is a good start for how this has to be implemented: https://github.com/OpenGeoVis/PVGeo/blob/cddb6eff57e43c294fbe30143c8f474770d7336f/PVGeo/base.py#L28

Overriding things like vtkConeSource by adding nice properties, etc is simple and straightforward, but I wanted to add a note about VTKPythonAlgorithmBase in case there is any need for custom algorithms or overriding the base class's methods (like, RequestData(), RequestInformation(), etc.

@adeak
Copy link
Member

adeak commented Sep 14, 2022

the methods of the VTK algorithm class have to be executed on the C++ side.

Could you explain what this means? It went over my head.

@banesullivan
Copy link
Member Author

the methods of the VTK algorithm class have to be executed on the C++ side.

Could you explain what this means? It went over my head.

Yeah, that statement is loaded with a lot. You can implement a VTK Algorithm in some fancy ways where you might want to override/implement the RequestData() method (this is the meat behind a VTK Algorithm). So that if you have some custom algorithm written in Python and pass it to a pipeline (also defined in Python but really running on the C++ side as you're using bound Python objects), it will execute the Python code from the C++ side. Specifically, this subclass makes it so the VTK pipeline becomes aware of the Python code so that you can override/implement some methods on the algorithm, and calls from the C++ side to those methods actually execute on the Python side.

For example, we cannot actually override the Modified() method on a vtkDataSet subclass as that code will only ever get executed if .Modified() is called on the Python side (but it is called on the C++ side all the time and our Python override does not get executed). This is the same for VTKPythonAlgorithmBase but with some exceptions. VTKPythonAlgorithmBase makes it so that certain methods are known by the C++ side and can be executed. For instance, .Modified() cannot be overridden, but RequestData() can be.

The implementations are here:

I don't want to derail this PR thread too much on this topic, so perhaps if people are more interested, we can start a discussion and talk about how these algorithms might fit into PyVista (like what I wrote in PVGeo)

@banesullivan
Copy link
Member Author

This is still a work in progress, but I'd love to open this up for feedback and reviews from @pyvista/developers

@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Merging #3318 (79b545b) into main (880d172) will decrease coverage by 0.07%.
The diff coverage is 90.54%.

@@            Coverage Diff             @@
##             main    #3318      +/-   ##
==========================================
- Coverage   94.18%   94.11%   -0.08%     
==========================================
  Files          86       87       +1     
  Lines       18914    19108     +194     
==========================================
+ Hits        17815    17984     +169     
- Misses       1099     1124      +25     

@banesullivan banesullivan marked this pull request as ready for review November 16, 2022 06:20
@banesullivan
Copy link
Member Author

It seems like a lot of the plotting tests are giving warnings for the image regression comparison. Then one of the tests added here (plot_algorithm_scalars_1) is giving this on Windows CI:

Truth Windows CI
plot_algorithm_scalars_1 test_plot_algorithm_scalars_1

This is incredibly unexpected and I cannot reproduce this on my local Windows machine but it is failing for all Windows CI runners.

@pyvista/developers, has anyone else seen this big of a difference on the Windows runner before?

@akaszynski, I'm wondering if there is a bug on Windows with lookup tables/color mapping as it seems the data are correct but that the cmap is messed up

@adeak
Copy link
Member

adeak commented Nov 16, 2022

I don't know if we might have any "skip on flaky windows" tests hiding a similar issue. I still only use linux for pyvista. That image diff looks super weird.

@adeak
Copy link
Member

adeak commented Nov 16, 2022

it seems the data are correct but that the cmap is messed up

Wouldn't the bottom of the cube have to be correct in that case?

@banesullivan
Copy link
Member Author

Talked to Alex and reproduced the issue in #3609. Apparently, Windows with OSMesa is just bad... For this PR, I'm going to skip that test on windows and we'll have to deal with these OSMesa issues later

@banesullivan
Copy link
Member Author

Alright! :phew: @adeak, I think I've addressed everything in your review. Thank you for bringing up some excellent points and feedback. Feeling a lot better about this PR now

pyvista/plotting/widgets.py Outdated Show resolved Hide resolved
@adeak
Copy link
Member

adeak commented Jan 12, 2023

Almost done, thanks for bearing with me @banesullivan. One comment posted just now, and #3318 (comment) still seems open. Neither are blockers.

@banesullivan
Copy link
Member Author

banesullivan commented Jan 12, 2023

Seeing integration failures from MNE, which appear to be failing on main too. Since these were passing for this PR up until this morning, I think they are safe to ignore

ref mne-tools/mne-python#11415

@larsoner
Copy link
Contributor

We treat warnings as errors so in theory just adding a new ignore line should fix it -- mne-tools/mne-python#11416 should land within an hour, then you can restart CIs here...

@adeak
Copy link
Member

adeak commented Jan 12, 2023

I've also seen some doc build errors but couldn't investigate from mobile yet.

Aah, you fixed it in c374454.

Copy link
Member

@adeak adeak 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 great, thanks for fixing up everything @banesullivan! Here's hoping #3318 (comment) indeed won't break CI.

@banesullivan
Copy link
Member Author

Thanks for the thorough review and for helping me clean this up quite a bit!

@akaszynski, any final thoughts? Or can I merge soon here?

Copy link
Member

@akaszynski akaszynski 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 good to go. It works even on my closed source application and it's going to open doors for us in the future, but as an opt-in feature that doesn't change the fundamentals of PyVista.

Merge at will. maybe follow the 24 hour rule.

@banesullivan
Copy link
Member Author

Will merge tomorrow sometime! Thanks, @akaszynski and @adeak!

@banesullivan banesullivan merged commit c0d5fc7 into main Jan 13, 2023
@banesullivan banesullivan deleted the feat/algorithms branch January 13, 2023 16:20
@banesullivan banesullivan mentioned this pull request Nov 2, 2023
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Changes that enhance the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants