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

[VTK] Converter is buggy - propose move to PyVista for maintainability #408

Closed
banesullivan opened this issue May 1, 2019 · 10 comments
Closed
Labels
type: enhancement Minor feature or improvement to an existing feature

Comments

@banesullivan
Copy link
Contributor

banesullivan commented May 1, 2019

First, I love what you have created for converting VTK render windows to VTKjs scenes that can be embedded in a panel! It's absolutely awesome!

I've been working towards incorporating panel as an optional dependency for vtki to allow users to view scenes they create via vtki directly in a notebook - I have examples live at http://playground.vtki.org

Unfortunately, there are a lot of nuances to the conversion between VTK render scenes and VTKjs scenes that aren't covered in your panel/pane/vtk.py script - also I've been experiencing lots of bugs regarding scalar mapping and vtkStructuredGrids outright don't work with your script. These issues bring me hesitation in fully supporting panel as the default notebook viewer for vtki

A while back in pyvista/pyvista#57 (with many later bug fixes), I created the same VTKjs export script directly in vtki that handles a lot of the VTK nuances... I'm curious if you all would be interested in migrating your VTKjs conversion needs over to vtki? This way the developers for vtki could maintain the nitty-gritty details of how VTK scenes are converted to VTKjs and panel could simply depend on vtki for that conversion.

The exact details of this dependency might get complicated since panel would rely on vtki for conversion and vtki would rely on panel for displaying scenes in a notebook - we'll have to figure out a clean solution for this...

@xavArtley
Copy link
Collaborator

I cannot work on this for the moment (I'm in holidays for the next weeks) however we could refactor a little bit the class to set custom vtkjs serializers. We could keep the panel serializer as default for backward compatibility and not add extra dependencies. And in vtki you could set your own serializer

@philippjfr
Copy link
Member

A while back, I created the same VTKjs export script directly in vtki that handles a lot of the VTK nuances... I'm curious if you all would be interested in migrating your VTKjs conversion needs over to vtki? This way the developers for vtki could maintain the nitty-gritty details of how VTK scenes are converted to VTKjs and panel could simply depend on vtki for that conversion.

I think that was the long term plan anyway, we just needed something to be merged in the shorter term for some internal demos so I pushed ahead with merging @xavArtley's implementation.

We could keep the panel serializer as default for backward compatibility and not add extra dependencies. And in vtki you could set your own serializer

This seems like a decent plan to me, it would probably default to the vtki serializer if installed and otherwise fall back to the existing one.

@philippjfr
Copy link
Member

The exact details of this dependency might get complicated since panel would rely on vtki for conversion and vtki would rely on panel for displaying scenes in a notebook - we'll have to figure out a clean solution for this...

Panel relies on dynamic imports in many places to avoid hard dependencies on external libraries so I think that won't present major issues.

@banesullivan
Copy link
Contributor Author

however we could refactor a little bit the class to set custom vtkjs serializers ... And in vtki you could set your own serializer

This seems like a decent plan to me, it would probably default to the vtki serializer if installed and otherwise fall back to the existing one.

Awesome! I'll try to move forward with this before too long as most of the code is already implemented in vtki - just need to do a few bug fixes.

we could refactor a little bit the class to set custom vtkjs serializers.

This would be great! I'll try to do all the prep work on a branch in vtki to mimic how you all are serializing to memory and not to a file then wait until you have time work on enabling a custom serializer over here in panel, @xavArtley

@banesullivan banesullivan changed the title [VTK] Converter is buggy - propose move to vtki for maintainability [VTK] Converter is buggy - propose move to PyVista for maintainability May 13, 2019
@banesullivan
Copy link
Contributor Author

Just an FYI for when we get rolling on this, we had to rename vtki to PyVista: https://github.com/pyvista/pyvista

@philippjfr
Copy link
Member

@banesullivan Now that the converter is pluggable how do we proceed from here to optionally use the converter in PyVista?

@banesullivan
Copy link
Contributor Author

Now that the converter is pluggable how do we proceed from here to optionally use the converter in PyVista?

Could you point me to a demo/psuedo code on how to use a pluggable converter? I still need to clean up our converter in PyVista then I'll try to handle setting up all the "wiring" to make PyVista an optional converter here in panel

@philippjfr
Copy link
Member

@xavArtley Any chance you could get an update on this? I have to admit I haven't really followed the current status.

@xavArtley
Copy link
Collaborator

I added possibility to register custom serilizers in #475. I don't know what to do more.

@banesullivan
Copy link
Contributor Author

I think this issue is no longer valid. Custom serializers can be implemented and it looks like you all are moving towards using the scene graph exporter that was added directly to VTK: https://github.com/Kitware/VTK/blob/master/IO/Export/Testing/Cxx/TestJSONRenderWindowExporter.cxx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants