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

Redraw ChartMPL on Plotter render #3731

Merged
merged 11 commits into from
Jan 10, 2023
Merged

Conversation

dcbr
Copy link
Contributor

@dcbr dcbr commented Dec 19, 2022

Overview

As requested in #3380 this PR adds functionality to automatically redraw ChartMPL charts whenever the Plotter.update() method is called. Furthermore, a bug in the chart's _resize method is fixed. Resolves #3380.

Details

  • Adds a redraw_on_render property for ChartMPL (default True), which enables automatic redrawing of these charts when the Plotter.render() method is called. To support this feature, extra on_plotter_render methods to the Renderer and Renderers classes are needed to discern between a manual call to Plotter.render and manual/automatic calls to Renderer.Render (which invoke VTK StartEvent, also used by charts).
  • Previously the logic in the _resize method of charts used exact comparisons of floats, which lead to unnecessary resizings (and redrawings) of charts. This PR converts all current and target dimensions to integers to resolve this bug.

@github-actions github-actions bot added the enhancement Changes that enhance the library label Dec 19, 2022
banesullivan
banesullivan previously approved these changes Dec 19, 2022
Copy link
Member

@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.

LGTM... the general updates to the BasePlotter look good to me (calling update() on the renderers when BasePlotter.update() is called).

Otherwise, I trust you, @dcbr, as I'm totally unfamiliar with what's going on with the charting

@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Merging #3731 (e64bd35) into main (3db44d7) will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3731      +/-   ##
==========================================
+ Coverage   93.99%   94.16%   +0.16%     
==========================================
  Files          82       86       +4     
  Lines       18553    18906     +353     
==========================================
+ Hits        17439    17802     +363     
+ Misses       1114     1104      -10     

@dcbr
Copy link
Contributor Author

dcbr commented Dec 19, 2022

Alright, I tested locally on Windows, but I'll add a test tomorrow to improve coverage as well :)

@dcbr dcbr marked this pull request as draft December 19, 2022 20:36
@dcbr dcbr added the bug Uh-oh! Something isn't working as expected. label Dec 19, 2022
@dcbr dcbr marked this pull request as ready for review December 20, 2022 17:23
@dcbr
Copy link
Contributor Author

dcbr commented Dec 20, 2022

I added the test, ready for review (again) if everything turns green :)

@akaszynski
Copy link
Member

I added the test, ready for review (again) if everything turns green :)

Failures will be fixed in #3736 and #3737.

Copy link
Member

@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.

I know I gave this an initial approval but now I have some counter thoughts on adding an update() method to the Renderer class - IMO, this should be in the render() calls

pyvista/plotting/renderer.py Outdated Show resolved Hide resolved
pyvista/plotting/renderers.py Outdated Show resolved Hide resolved
pyvista/plotting/plotting.py Outdated Show resolved Hide resolved
pyvista/plotting/plotting.py Outdated Show resolved Hide resolved
@dcbr
Copy link
Contributor Author

dcbr commented Dec 27, 2022

Hmm yea I thought of adding it to the render() method instead. The reason I chose not to, is because render is already called in various other plotting methods that are unrelated to charts (and thus cause unnecessary overhead of redrawing the ChartMPL figures).
Maybe a workable middleground is to split the rendering step (internally) in two parts: render_viewport for rendering the 3D parts (and calling vtkRenderer.Render) and render_scene for rendering the 2D parts (and updating ChartMPL figures). The existing calls to render from other plotting methods can then be converted to render_viewport such that Charts are not impacted by them. And the existing render method then just calls both render_viewport and render_scene. What do you think?

@dcbr
Copy link
Contributor Author

dcbr commented Dec 27, 2022

Maybe for now I can just move it to the render method and if we ever want to improve performance (or in case there are complaints), we can still do this split later.

@dcbr dcbr changed the title Redraw ChartMPL on Plotter update Redraw ChartMPL on Plotter render Dec 28, 2022
banesullivan
banesullivan previously approved these changes Jan 6, 2023
Copy link
Member

@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.

Overall, I'm happy with this! but I think Renderer.render() needs to call self.Render() too

pyvista/plotting/renderer.py Outdated Show resolved Hide resolved
Copy link
Member

@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.

on_plotter_render() is a good choice. LGTM!

@banesullivan banesullivan merged commit 21e87f6 into pyvista:main Jan 10, 2023
@dcbr dcbr deleted the feat/chartmpl_redraw branch January 11, 2023 19:10
@banesullivan banesullivan mentioned this pull request Feb 1, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected. enhancement Changes that enhance the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plotter should redraw chartMPL instances if axes is modified with set_xxxx
3 participants