Fix savefig size and black image #331

Merged
merged 36 commits into from Apr 21, 2016

Conversation

Projects
None yet
2 participants
@kitchoi
Member

kitchoi commented Mar 31, 2016

Fix simphony/simphony-mayavi#52, fix #257, potentially related to #75

Supersede #316 and #317 (the two issues are highly related that it has become impossible to separate the two)

Upon saving an image (snapshot), a new tvtkRenderWindow is created. This is motivated by the fact that under Mac OS, vtkCocoaRenderWindow is the render window being used and if the vtkRenderWindow is contained by an interactor, resizing the renderwindow leads to the resulting image to be rendered only within the initial size. Therefore a new tvtkRenderWindow is needed for saving an image in Mac.

This fix for Mac causes a side effect for Linux which is that when offscreen is off, saving an image will create a new window. Forcing offscreen to true for the dummy tvtkRenderWindow would enhance the user experience but render the savefig function completely unusable if the machine does not support offscreen.

Tested platforms:
Mac OS 10.11 + wxPython/PyQt + VTK 6.2
Ubuntu 12.02 LTS + VTK 5.10/6.2.0/6.3.0 + wxPython/PyQt

kitchoi and others added some commits Mar 15, 2016

set_size was doing the right thing, instead it was because the window…
… is not lifted when

saving an image + off_screen_rendering + wx
Revert "mend"
This reverts commit 4d85baf.
Kit Kit

@kitchoi kitchoi referenced this pull request Mar 31, 2016

Closed

Fix savefig size #316

kitchoi added some commits Mar 31, 2016

@kitchoi kitchoi closed this Apr 1, 2016

@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi Apr 1, 2016

Member

There may be a better way.

Member

kitchoi commented Apr 1, 2016

There may be a better way.

@kitchoi kitchoi reopened this Apr 1, 2016

@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi Apr 1, 2016

Member

The "better" way doesn't work. Never mind.

Member

kitchoi commented Apr 1, 2016

The "better" way doesn't work. Never mind.

@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi Apr 1, 2016

Member

This is good for review.

Member

kitchoi commented Apr 1, 2016

This is good for review.

kitchoi added some commits Apr 1, 2016

@stefanoborini

This comment has been minimized.

Show comment
Hide comment
@stefanoborini

stefanoborini Apr 21, 2016

Contributor

Reviewing this one

Contributor

stefanoborini commented Apr 21, 2016

Reviewing this one

+ # Remove OnPaint handler for PaintEvent, otherwise
+ # OnPaint tries to reset the size of the nonexisting
+ # renderwindow
+ wx.EVT_PAINT(self._vtk_control, None)

This comment has been minimized.

@stefanoborini

stefanoborini Apr 21, 2016

Contributor

This one makes me a bit nervous because I checked the rest of the class, and nowhere it is enforced that self._vtk_control is not None. We have the same problem pretty much everywhere. I am generally fine with throwing an exception, but if None is an allowed value, we should react appropriately without throwing. If it's not, it's better to enforce the presence of a control at construction time.

@stefanoborini

stefanoborini Apr 21, 2016

Contributor

This one makes me a bit nervous because I checked the rest of the class, and nowhere it is enforced that self._vtk_control is not None. We have the same problem pretty much everywhere. I am generally fine with throwing an exception, but if None is an allowed value, we should react appropriately without throwing. If it's not, it's better to enforce the presence of a control at construction time.

This comment has been minimized.

@kitchoi

kitchoi Apr 21, 2016

Member

_create_control is called here https://github.com/enthought/mayavi/blob/fix-savefig-size-black-image/tvtk/pyface/tvtk_scene.py#L205 which assigns _vtk_control. Unless self._vtk_control = None is done somewhere outside of this function (which I have not checked), we can ensure _vtk_control is not None?

@kitchoi

kitchoi Apr 21, 2016

Member

_create_control is called here https://github.com/enthought/mayavi/blob/fix-savefig-size-black-image/tvtk/pyface/tvtk_scene.py#L205 which assigns _vtk_control. Unless self._vtk_control = None is done somewhere outside of this function (which I have not checked), we can ensure _vtk_control is not None?

This comment has been minimized.

@kitchoi

kitchoi Apr 21, 2016

Member

On other hand, this change is meant to silent error message on Linux + wx 2.8.10 (see message in #346)
The same error does not occur on OSX with wxpython 3.0.2, so I have omitted this change in the reimplementation #346

@kitchoi

kitchoi Apr 21, 2016

Member

On other hand, this change is meant to silent error message on Linux + wx 2.8.10 (see message in #346)
The same error does not occur on OSX with wxpython 3.0.2, so I have omitted this change in the reimplementation #346

This comment has been minimized.

@stefanoborini

stefanoborini Apr 21, 2016

Contributor

Good. Merging.

@stefanoborini

stefanoborini Apr 21, 2016

Contributor

Good. Merging.

@stefanoborini stefanoborini merged commit 3bccaaf into master Apr 21, 2016

5 checks passed

codecov/project 45.36% (target 40.00%)
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@stefanoborini stefanoborini deleted the fix-savefig-size-black-image branch Apr 21, 2016

@kitchoi kitchoi restored the fix-savefig-size-black-image branch Apr 21, 2016

@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi Apr 21, 2016

Member

This fix requires more investigation as the implementation is not ideal (too many dummy renderer/window).
See #346, #347

Member

kitchoi commented Apr 21, 2016

This fix requires more investigation as the implementation is not ideal (too many dummy renderer/window).
See #346, #347

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