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

Integrate widget from upstream ipyvtk_simple #53

Open
banesullivan opened this issue Jul 23, 2020 · 3 comments
Open

Integrate widget from upstream ipyvtk_simple #53

banesullivan opened this issue Jul 23, 2020 · 3 comments

Comments

@banesullivan
Copy link

banesullivan commented Jul 23, 2020

I borrowed much of the awesome work here in the ViewInteractiveWidget class to create ipyvtk_simple as a more minimal, general, and extensible version of the widget for use with any vtkRenderWindow backend (VTK or ParaView). It would be awesome refactor this project to utilize ipyvtk_simple for the widget and maintain it there so that the broader Jupyter VTK ecosystem can leverage the excellent work done in this project!

I can handle a PR for this, but I won't have time likely until mid-August at the earliest. I figured I'd raise the issue just so everyone is aware and if there needs to be any discussion on this work, that can happen here. cc @jcfr

Some notable changes:

@lassoan
Copy link
Contributor

lassoan commented Jul 23, 2020

Thank you for working on this. I'm very much in favor of sharing a single Jupyter VTK render window viewer!

A few comments/suggestions that would need to be addressed:

  1. Package name

We should not use dash in the name, as it just bring unnecessary complexity: we cannot use the same name everywhere (some places you need to use ipyvtk-simple, other times you need to use ipyvtk_simple).

Since the package offers a viewer (render window), it should be reflected in the name. "simple" usually refers to simplifying/abstracting another package, which is not the case here, as we wrap VTK features to create widgets. For example, these names would be much better: ipyvtkviewer, ipyvtkrenderwindow, ipyvtkwindow, ipyvtkrenderer. If you plan to add other Jupyter related features then the package could be ipyvtk or ipyvtkwidgets or vtkwidgets (similar to ITK's itkwidgets).

  1. Render timers

no more render timers and checking render times in favor of throttling from both ipyevents and ipywdigets (see Kitware/ipyvtklink#3 and linked PRs)

Do you mean there would be no more quick/full render anymore?

  1. Throttling

Javascript-side throttling would be nice. Have you managed to throttle events at that level?

I've tried Python-side throttling (@throttle(0.1)), too, but found the adaptive scheme to work much better than throttling with a hardcoded constant. There is no throttling rate that is optimal for local connections and work well for binder, so you either expose the throttling rate and put the burden on the user to tune that value in each environment, or make it adaptive. Maybe the throttle class can be updated to be adaptive but I decided to implement the throttling mechanism in the viewer class to make things simpler and more transparent (the syntactic sugar of a throttle decorator is elegant, and the fact that it makes things more obscure may not matter for simple things, but for a more complex implementation, the extra layer of abstraction is just too confusing).

  1. Timer mechanism

asyncio may not play well in applications that have their own event loop, such as Slicer. I think we tried it in Slicer and it did not work at all.

Did you need to do anything special for this to work in ParaView? What Qt wrapping Paraview uses? How Python is integrated into ParaView's event loop?

Probably we should add an option to switch between different timer mechanisms. We need to have at least PythonQt and asyncio options (but others may want to add PyQt).

@banesullivan
Copy link
Author

  1. Package name

We should not use dash in the name, as it just bring unnecessary complexity: we cannot use the same name everywhere (some places you need to use ipyvtk-simple, other times you need to use ipyvtk_simple).

The dash appears only in the repository name/URL - everywhere else, it uses an underscore. I am well aware of how confusing this can get with Python packages.

Since the package offers a viewer (render window), it should be reflected in the name. "simple" usually refers to simplifying/abstracting another package, which is not the case here, as we wrap VTK features to create widgets. For example, these names would be much better: ipyvtkviewer, ipyvtkrenderwindow, ipyvtkwindow, ipyvtkrenderer. If you plan to add other Jupyter related features then the package could be ipyvtk or ipyvtkwidgets or vtkwidgets (similar to ITK's itkwidgets).

You bring up an excellent point... we deliberately chose the name ipyvtk_simple because it is indeed a "simpler" implementation of a bigger toolkit we are working on - at least conceptually as it does not depend on that software (And that software does not yet exist). The name ipyvtk is reserved for some other work we are doing in the integration of itkwidgets and ipyparaview (we are having internal discussions across stakeholders within Kitware about these names). We actually do not plan on expanding the feature set of ipyvtk_simple much if any bit further and thus we will likely keep the name as ipyvtk_simple to imply that it is an incredibly simple tool (though we could remove the underscore). We are moving on to a more robust Jupyter widget toolkit based on the itkwidgets stack that will handle what ipyvtk_simple does and much, much more. Though we do think it is valuable to maintain ipyvtk_simple as it provides such an easy to use Jupyter interface for VTK/ParaView and clearly has impact for software like Slicer and PyVista.

  1. Render timers

Do you mean there would be no more quick/full render anymore?

No this is still there for the most part, but much of the timer code that went into it has gone away in favor of throttling on the server-side. I may have messed this up actually - I will need to revisit it.

  1. Throttling

Javascript-side throttling would be nice. Have you managed to throttle events at that level?

ipyevents now handles this. See Kitware/ipyvtklink#10 and mwcraig/ipyevents#55

... There is no throttling rate that is optimal for local connections and work well for binder, so you either expose the throttling rate and put the burden on the user to tune that value in each environment, or make it adaptive. Maybe the throttle class can be updated to be adaptive ...

This is a good point, we can open an issue in ipyvtk_simple about this to implement some sort of adaptive throttling in the server-side. As far as on the client/javascript side, its constant based which seems fine to me.

Indeed, the performance on Binder is not great. We have demos here: Binder

  1. Timer mechanism

Did you need to do anything special for this to work in ParaView? What Qt wrapping Paraview uses? How Python is integrated into ParaView's event loop?

Nothing special at all... I'm not sure what you are getting at/what issues you were having?

Probably we should add an option to switch between different timer mechanisms. We need to have at least PythonQt and asyncio options (but others may want to add PyQt).

That's fair. There shouldn't be a need to use PythonQt timer options... Everything should work without any special Qt stuff if you make the events thread-safe (I think). The widget works fine with PyVista's BackgroundPlotter last I checked which should be similar to using it with Slicer. Perhaps I just need to give it a try with Slicer and see what I broke.

@lassoan
Copy link
Contributor

lassoan commented Jul 23, 2020

  1. Package name

Thanks for the additional information. Since you plan to keep only the interactive viewer in the package, the name should reflect this: it is a viewer, not a simple interface to VTK. So, please rename it to something like ipyvtkviewer, ipyvtkrenderwindow, ipyvtkwindow, ipyvtkrenderer, ...

  1. Render timers

OK!

  1. Throttling

Slicer's refresh rate is much better than the current https://mybinder.org/v2/gh/Kitware/ipyvtk-simple/master - view rotation in SlicerJupyter on binder seems more smooth (except when congestion kicks in due to lack of throttling at javascript level). But maybe it is just due to render window size difference. Nevertheless, it seems that there is no high-speed rendering done anymore (at least I don't see it in pyvista.ipynb) and we always need to wait for the full-quality rendering to be sent over. Note that we could reach almost any frame rate in quick_render by not just altering compression options but also lowering resolution (rendering at 1/2 or 1/4 resolution, sending that image over, and displaying that with 2x or 4x zoom)

  1. Timer mechanism

asyncio's has an event loop and if you have a GUI application then it has an event loop, too, so they are not supposed to work together unless you make asyncio use the application's event loop instead of creating its own. There is nothing special about PythonQt, it is the same for PyQt, Tk, etc. Maybe you used a server implementation of ParaView that does not have a GUI.

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

No branches or pull requests

2 participants