SetSize needs to be called as an unbound method (ref: Kitware/VTK) #304

Merged
merged 4 commits into from Apr 1, 2016

Conversation

Projects
None yet
3 participants
@kitchoi
Member

kitchoi commented Mar 1, 2016

Fix #302
The solution is found here: https://github.com/Kitware/VTK/blob/v6.3.0/Wrapping/Python/vtk/qt4/QVTKRenderWindowInteractor.py

Should we refactor the changes we make as an extension such that we can import QVTKRenderWindowInteractor from vtk directly (and then subclass etc)?

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Mar 2, 2016

Member

This is very strange and I am not sure why this is needed. While updating our version of the QtRWI, I noticed this but didn't realize it made such a big difference. I will have to take a look at the sources to see why this makes a difference at all.

I think it might be good to refactor the RWI. I thought of doing this when making the changes in #250 but it seemed to require a fair bit of work. One way to do this would be to coordinate and refactor the upstream QtRWI. I am happy to send a PR upstream but don't have the time to do the actual refactoring now. The main problem was that some of the upstream changes did not work well for me at all.

The wxRWI was a lot simpler and I have removed our out-of-date version and simply import the upstream version.

Member

prabhuramachandran commented Mar 2, 2016

This is very strange and I am not sure why this is needed. While updating our version of the QtRWI, I noticed this but didn't realize it made such a big difference. I will have to take a look at the sources to see why this makes a difference at all.

I think it might be good to refactor the RWI. I thought of doing this when making the changes in #250 but it seemed to require a fair bit of work. One way to do this would be to coordinate and refactor the upstream QtRWI. I am happy to send a PR upstream but don't have the time to do the actual refactoring now. The main problem was that some of the upstream changes did not work well for me at all.

The wxRWI was a lot simpler and I have removed our out-of-date version and simply import the upstream version.

@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi Mar 2, 2016

Member

May I attempt the refactoring? While you say upstream, do you mean the repository under Kitware/vtk?

Member

kitchoi commented Mar 2, 2016

May I attempt the refactoring? While you say upstream, do you mean the repository under Kitware/vtk?

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Mar 2, 2016

Member

The upstream links do not clearly document why one has to use the unbound method? For example this link in the commit that fixes it

http://www.vtk.org/pipermail/vtk-developers/2011-August/010284.html

and it seems unrelated to the change.

Member

prabhuramachandran commented Mar 2, 2016

The upstream links do not clearly document why one has to use the unbound method? For example this link in the commit that fixes it

http://www.vtk.org/pipermail/vtk-developers/2011-August/010284.html

and it seems unrelated to the change.

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Mar 2, 2016

Member

@kitchoi -- Sure you may certainly try to do it if you have the time (and patience) for it! Yes I mean Kitware's gitlab repo: https://gitlab.kitware.com/vtk/vtk

Member

prabhuramachandran commented Mar 2, 2016

@kitchoi -- Sure you may certainly try to do it if you have the time (and patience) for it! Yes I mean Kitware's gitlab repo: https://gitlab.kitware.com/vtk/vtk

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Mar 9, 2016

Member

Could you please add a small test case for this? It would also be good to reference this thread. I still don't quite understand why this fails with VTK 6.3.0. Thanks!

Member

prabhuramachandran commented Mar 9, 2016

Could you please add a small test case for this? It would also be good to reference this thread. I still don't quite understand why this fails with VTK 6.3.0. Thanks!

@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi Mar 23, 2016

Member

@prabhuramachandran I have added a test case for this issue. The test case in this PR will have conflicts with #316 and #317 should they be merged. This is because they are all related to mlab.savefig.

Member

kitchoi commented Mar 23, 2016

@prabhuramachandran I have added a test case for this issue. The test case in this PR will have conflicts with #316 and #317 should they be merged. This is because they are all related to mlab.savefig.

+import tempfile
+
+import numpy
+from PIL import Image

This comment has been minimized.

@prabhuramachandran

prabhuramachandran Mar 28, 2016

Member

This doesn't seem necessary?

@prabhuramachandran

prabhuramachandran Mar 28, 2016

Member

This doesn't seem necessary?

This comment has been minimized.

@kitchoi

kitchoi Mar 29, 2016

Member

Yes you're right. This was copied from related PRs; I just removed the import here.

@kitchoi

kitchoi Mar 29, 2016

Member

Yes you're right. This was copied from related PRs; I just removed the import here.

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Apr 1, 2016

Member

Thanks! Merging.

Member

prabhuramachandran commented Apr 1, 2016

Thanks! Merging.

@prabhuramachandran prabhuramachandran merged commit 1e1dcd3 into master Apr 1, 2016

5 checks passed

codecov/project 45.37% (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

@prabhuramachandran prabhuramachandran deleted the issue-302 branch Apr 1, 2016

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