-
Notifications
You must be signed in to change notification settings - Fork 519
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
Conversation
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() |
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 Overriding things like |
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 For example, we cannot actually override the 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) |
This is still a work in progress, but I'd love to open this up for feedback and reviews from @pyvista/developers |
Codecov Report
@@ 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 |
18011f5
to
48cf9d9
Compare
It seems like a lot of the plotting tests are giving warnings for the image regression comparison. Then one of the tests added here (
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 |
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. |
Wouldn't the bottom of the cube have to be correct in that case? |
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 |
9e7efc8
to
403e8d4
Compare
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 |
Almost done, thanks for bearing with me @banesullivan. One comment posted just now, and #3318 (comment) still seems open. Neither are blockers. |
7c8c62d
to
c374454
Compare
Seeing integration failures from MNE, which appear to be failing on |
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... |
Aah, you fixed it in c374454. |
There was a problem hiding this 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.
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? |
There was a problem hiding this 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.
Will merge tomorrow sometime! Thanks, @akaszynski and @adeak! |
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 thevtkMapper
used inadd_mesh
already supports the pipeline infrastructure, we can add a few lines of code to check if the passed object is avtkAlgorithm
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'sResolution
, and then perform a box clip.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
BasePlotter
methds that accept mesh objects and add support forvtkAlgorithm
s where appropriateadd_points
add_point_labels
(not doingadd_point_scalar_labels
as that's a special case for now)add_silhouette
add_mesh_*
helpersadd_mesh
's handling ofvtkAlgorithm
svtkAlgorithm
is passed may not work if the mesh generated byGetOutput()
changes (very likely)TODO
comments added in codeTypeError
s added in codePointSet
issuescell_ids
scalar bar appearing when usingadd_ids_algorithm
but mesh not being coloredadd_mesh_cip_box(alg, invert=True)
with algorithmsKnown Issues
vtkPointSet
/pyvista.PointSet
is incompatible withVTKPythonAlgorithmBase
. This has mostly been addressed in this PR with the custom algorithms added. See https://gitlab.kitware.com/vtk/vtk/-/issues/18771