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

Several Performance Improvements #13

Merged
merged 7 commits into from
Nov 4, 2020

Conversation

akaszynski
Copy link
Contributor

@akaszynski akaszynski commented Nov 4, 2020

This is an excellent module and I've already made some use of it on a closed source project, but it's speed leaves much to be desired. I've cleaned up the interface a bit and changed the implementation somewhat to improve the performance. Namely:

  • Use put_image_data with the image array rather using BytesIO and writing a fake file. About a 5-10% performance improvement depending on the size of the window and the base render time.
  • Access the pixel data directly with GetRGBACharPixelData. I saw a mention of this on SO and investigated it. Turns out it's way faster than vtk.vtkWindowToImageFilter, and I'm seeing an extra 30% performance improvement on VTK 9 and minimal improvement on v8. It's good enough that we should consider rewriting the screenshot saver within pyvista unless I'm missing something.

Miscellaneous Changes

  • Set the minimum render based on the first render. Helps to make sure that the quick renders on million point surface mesh don't block mouse movement or make the lag feel too intense.
  • Allow wheel zooming. It's not going to be an issue when people are scrolling through notebooks as they can just move their mouse to the side, and we really should try to duplicate the standard window within our Canvas. Lots of people are comfortable with mouse wheel zoom, and I was really missing this when playing around with it. It's an option in the init of ViewInteractiveWidget
  • Very minor refactoring
  • Use weakref.ref instead of a direct reference to the render window to avoid gc issues.
  • Scale mouse movement so movement in the canvas corresponds to the render window (Resolve Fix canvas widget sizing issue #2)

@akaszynski
Copy link
Contributor Author

akaszynski commented Nov 4, 2020

Pinging @banesullivan for his thoughts...

If possible, it would be great to have this released as 0.1.0.

The biggest performance bottleneck right now is having to use software rendering as there seems to be no way to render off-screen with a GPU. It doesn't matter at the moment for me as I'm primarily using docker containers on a kubernetes cluster, but in the future, off-screen rendering would be ideal.

I understand there's a big push in Kitware to move towards a lightweight java client, and I think that will work in most situations, but when you have a gigantic model you'd like to render, it's tough to send across 100 MB (or more) of data and expect the client to render it on a potato. Server side render does still have its niche.

@banesullivan banesullivan self-requested a review November 4, 2020 16:23
Copy link
Contributor

@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 work! Thanks, @akaszynski!!


self.draw_image(self.get_image(force_render=True))
tstart = time.time()
self.put_image_data(self.get_image(force_render=True))
Copy link
Contributor

Choose a reason for hiding this comment

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

Good find!

Comment on lines +138 to +139
self.render_window.GetRGBACharPixelData(0, 0, self.width - 1,
self.height - 1, 0, arr)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome 🤯

Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely need to send that over to PyVista

@banesullivan banesullivan merged commit f7b8c17 into Kitware:master Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix canvas widget sizing issue
2 participants