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

BUG #1740: plotting with bg=0 produces labels off #1862

Merged
merged 3 commits into from Mar 7, 2016

Conversation

Projects
None yet
5 participants
@danlipsa
Contributor

danlipsa commented Mar 1, 2016

This also fixes BUG #78 in cdat-web.
For both bugs, VTKPlots.configureEvent is not called to reposition the labels.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 1, 2016

@danlipsa danlipsa force-pushed the labels-off branch from b469495 to 2136c2e Mar 1, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 1, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 1, 2016

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Mar 1, 2016

That is GREAT! Thanks @danlipsa! @mattben @jbeezley can you please double check it fixes your issue!

@@ -357,7 +358,7 @@ def createRenderer(self, *args, **kargs):
return ren
def update(self, *args, **kargs):
self._lastSize = -1
self._lastSize = None

This comment has been minimized.

@doutriaux1

doutriaux1 Mar 1, 2016

Member

@danlipsa have you tested on mac? Mac is VERY different about this, also we need to test within GUI. @chaosphere2112 can you please test on your mac and in your new GUI?

This comment has been minimized.

@danlipsa

danlipsa Mar 1, 2016

Contributor

@doutriaux1 No, I haven't. I have my mac with me and I can run the tests. I tried to be very conservative in what I change. :-)

This comment has been minimized.

@danlipsa

danlipsa Mar 1, 2016

Contributor

@chaosphere2112 I added a missing png for a test you added. Can you make sure it is the correct image? Thanks.

This comment has been minimized.

@danlipsa

danlipsa Mar 1, 2016

Contributor

@doutriaux1 There are a bunch of test failures because of small differences in the baselines - that happens on master branch as well. I will push _xx versions shortly. Everything else looks good.

This comment has been minimized.

@danlipsa

danlipsa Mar 1, 2016

Contributor

@chaosphere2112 For test_vcs_missing_colorname I get 'Cell index must be in the range 0 to 255'. I'll let you fix that. That happens only on mac, on linux it works fine.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 2, 2016

@doutriaux1 @aashish24 I've done some more digging on this. There are 3 cases when creating a new window: 1. if you specify a window size less than the screen size 2. if you don't specify a window size 3. if you specify a window size bigger than the screen size. 1. always worked. 2. was fixed by #1821 3. Still does not work. This seems to be a bug in VTK. renderWindow.GetSize() still returns the size larger than the screen even if the window was resized to the screen size. I will file a different issue for 3.

@jbeezley

This comment has been minimized.

Contributor

jbeezley commented Mar 2, 2016

I tried testing this, but my vagrant provisioning is no longer working. I get the following error while building:

CMake Error at /opt/uvcdat-build/HDF5-prefix/src/HDF5-stamp/HDF5-build.cmake:56 (message):
  CTEST_FULL_OUTPUT

   Command failed: 1
   '/usr/bin/cmake' '-Dmake=$(MAKE)' '-DWORKING_DIR=/opt/uvcdat-build/build/HDF5' '-P' '/opt/uvcdat-build/CMake/cdat_make_step.cmake'
   From /opt/uvcdat-build/HDF5-prefix/src/HDF5-stamp/HDF5-build-out.log

  -------------------------------------------------

  -------------------------------------------------

   From /opt/uvcdat-build/HDF5-prefix/src/HDF5-stamp/HDF5-build-err.log

  -------------------------------------------------

  [INFO] ADDITIONAL CFLAGS

  [ERROR] Make Errors detected:

  Making all in src

    CCLD     libhdf5.la



  make[3]: warning: jobserver unavailable: using -j1.  Add `+' to parent make
  rule.

  libtool: link: `H5Spoint.lo' is not a valid libtool object

  make[5]: *** [libhdf5.la] Error 1

  make[4]: *** [all] Error 2

  make[3]: *** [all-recursive] Error 1



  CMake Error at /opt/uvcdat-build/CMake/cdat_make_step.cmake:16 (message):

    Error in Make
@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 2, 2016

@jbeezley Try a fresh uvcdat build. That is, delete everything in the build dir, reconfigure and build.

@jbeezley

This comment has been minimized.

Contributor

jbeezley commented Mar 2, 2016

That was starting from a fresh VM. I'm guessing it is because I'm using Ubuntu 14.04.

@jbeezley

This comment has been minimized.

Contributor

jbeezley commented Mar 2, 2016

It appears to be working after I deleted the HDF5-prefix directory and restarted the build. I'll let you know how it goes.

@danlipsa danlipsa force-pushed the labels-off branch 3 times, most recently from bcb8ebe to 6477076 Mar 2, 2016

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Mar 2, 2016

@danlipsa I don't really like the _N.png approach, I'm always worried on some OS the changes are not working but the test suite will pass because the _N.png match. How off were the diff? I feel it's safer to up the threshold a bit if the diff isn't too much.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 2, 2016

@doutriaux1 We only add _n.png after we look at them and consider that they introduce irrelevant changes. Upping the threshold will let pass any changes below the threshold. I think it is a balance: Once N from _N become large we probably should start looking at fixing issues that introduce these small differences. The image differences were about 70.

@jbeezley

This comment has been minimized.

Contributor

jbeezley commented Mar 2, 2016

Now I'm getting

  In file included from
  /opt/uvcdat-build/build/VTK/ThirdParty/ftgl/src/FTBitmapGlyphRenderOpenGL.cpp:2:0:


  /opt/uvcdat-build/build/VTK/ThirdParty/ftgl/src/FTGLgl.h:25:29: fatal
  error: GL/gl.h: No such file or directory

             #include <GL/gl.h>
                               ^

  compilation terminated.

  make[5]: ***
  [ThirdParty/ftgl/CMakeFiles/vtkftgl.dir/src/FTBitmapGlyphRenderOpenGL.cpp.o]
  Error 1

  make[4]: *** [ThirdParty/ftgl/CMakeFiles/vtkftgl.dir/all] Error 2

  make[3]: *** [all] Error 2

I assume this has to do with the vtk update. Are there any other system packages I need to get the new vtk + mesa working on 14.04? This is the list of package I currently have installed: https://gist.github.com/783abb3daf706b28bfe1

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 2, 2016

@jbeezley The VTK update is not in master yet. Try deleting VTK-prefix (and maybe build/VTK-build as the next step) as well and rebuild.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Mar 2, 2016

I guess you're right, it is unlikely that some changes do not work one one process only.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Mar 2, 2016

@jbeezley in general it's not a great idea to start a build with left over of previous versions build.

@jbeezley

This comment has been minimized.

Contributor

jbeezley commented Mar 2, 2016

I started from a completely new VM, I don't know how it gets more clean than that. I just tried deleting the build/VTK-build and VTK-prefix directories, and I'm getting the same error.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Mar 2, 2016

oh ok... @aashish24 are we still supporting ubuntu 14? If it's a fresh VM i would say switch to ubuntu 15.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 2, 2016

@doutriaux1 @jbeezley I have ubuntu 14.04 and it compiles fine. @jbeezley Why do you have a VM? Is your machine a mac?

@jbeezley

This comment has been minimized.

Contributor

jbeezley commented Mar 2, 2016

Are you compiling against mesa? I use the VM because 1. I can keep the development environment reproducible and 2. I'm using Mac 10.11 with gfortran 5.3 as my main development machine.

I'm currently updating my vagrant script to start from 15.04. I'll let you know how it goes.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 2, 2016

@jbeezley No, I am not compile against mesa. I use the hardware driver. I did that in the past though.

BUG cdat-web #78: VTKPlots.configureEvent is not called
A handler for ModifiedEvent is added to the interactor which is replaced
by vtkWeb. A new handler is added on render window.

@danlipsa danlipsa force-pushed the labels-off branch from 6477076 to cb09883 Mar 2, 2016

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 2, 2016

@doutriaux1 @aashish24 @jbeezley Jon's results just came back: With mesa the resize fix does not work. I will compile my VTK with mesa/offscreen and debug it that way as well.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 2, 2016

@doutriaux1 @aashish24 @jbeezley This is not set yet, the virtual machine used master ...

@jbeezley

This comment has been minimized.

Contributor

jbeezley commented Mar 2, 2016

Okay, with the correct version built, this does fix the resizing bug. Thanks @danlipsa!

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 2, 2016

@jbeezley Great! Thanks for testing this. @aashish24 @doutriaux1 This is ready to merge.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Mar 2, 2016

ok @danlipsa will do soon. Thanks.

@@ -1076,9 +1077,11 @@ def png(self, file, width=None, height=None,
if width is not None and height is not None:
if self.renWin.GetSize() != (width, height):
user_dims = (self.canvas.bgX, self.canvas.bgY, sz[0], sz[1])
self.renWin.SetSize(width, height)
# We need to set canvas.bgX and canvas.bgY before we do renWin.SetSize
# otherwise, canvas.bgX,canvas.bgY will win

This comment has been minimized.

@aashish24

aashish24 Mar 2, 2016

Contributor

@danlipsa what do you mean by canvas.bgX,canvas.bgY will win? if you set it after they should be un-used right?

This comment has been minimized.

@danlipsa

danlipsa Mar 2, 2016

Contributor

@aashish24 In the original order, SetSize triggered code that reset the size to bgX, bgY. Not sure which test has shown this. It is easy to investigate if we want to.

This comment has been minimized.

@aashish24

aashish24 Mar 2, 2016

Contributor

@aashish24 In the original order, SetSize triggered code that reset the size to bgX, bgY. Not sure which test has shown this. It is easy to investigate if we want to.

if that's the case why not just call SetSize and not set bgX and bgY

This comment has been minimized.

@aashish24

aashish24 Mar 2, 2016

Contributor

I belive bgX and bgY is only used when bg=1? @doutriaux1 @danlipsa if yes, that would explain why it would fail in bg=1 and not in bg=0

This comment has been minimized.

@danlipsa

danlipsa Mar 2, 2016

Contributor

if that's the case why not just call SetSize and not set bgX and bgY

Because you will endup, in certain cases, with bgX, bgY, not with the parameters you pass in SetSize

This comment has been minimized.

@aashish24

aashish24 Mar 2, 2016

Contributor

Because you will endup, in certain cases, with bgX, bgY, not with the parameters you pass in SetSize

if SetSize triggers something which set bgX and bgY in some cases which won't be same as the origin args then I think we have problem. I think its worth looking into it. Thoughts?

This comment has been minimized.

@danlipsa

danlipsa Mar 2, 2016

Contributor

if SetSize triggers something which set bgX and bgY in some cases

No, this SetSize triggers code that, in certain cases, sets the size of the window to bgX, bgY rather than the parameters passed to SetSize. The test that fails with the order reversed is vcs_test_png_set_size.

This comment has been minimized.

@danlipsa

danlipsa Mar 2, 2016

Contributor

@aashish24 This is the stack trace that explains why this happens:
lastSize: 15, 11
size: 814, 606
File "/home/danlipsa/src/uvcdat/testing/vcs/test_png_set_size.py", line 24, in
x.png(fnm,width=15)
File "/home/danlipsa/build/uvcdat/install/lib/python2.7/site-packages/vcs/Canvas.py", line 4773, in png
file, W, H, units, draw_white_background, *_args)
File "/home/danlipsa/build/uvcdat/install/lib/python2.7/site-packages/vcs/VTKPlots.py", line 1085, in png
self.renWin.SetSize(width, height)
File "/home/danlipsa/build/uvcdat/install/lib/python2.7/site-packages/vcs/VTKPlots.py", line 267, in configureEvent
self.canvas.plot(_pargs, render=False, *_key_args[i])
File "/home/danlipsa/build/uvcdat/install/lib/python2.7/site-packages/vcs/Canvas.py", line 2424, in plot
a = self.__plot(arglist, keyargs)
File "/home/danlipsa/build/uvcdat/install/lib/python2.7/site-packages/vcs/Canvas.py", line 3747, in __plot
returned_kargs = self.backend.plot(_arglist, **keyargs)
File "/home/danlipsa/build/uvcdat/install/lib/python2.7/site-packages/vcs/VTKPlots.py", line 518, in plot
self.renWin.SetSize(self.canvas.bgX, self.canvas.bgY)
File "/home/danlipsa/build/uvcdat/install/lib/python2.7/site-packages/vcs/VTKPlots.py", line 227, in configureEvent

@@ -344,6 +344,7 @@ def createRenWin(self, *args, **kargs):
if not self.bg:
self.createDefaultInteractor(self.renderer)
self.renWin.AddRenderer(self.renderer)
self.renWin.AddObserver("ModifiedEvent", self.configureEvent)

This comment has been minimized.

@aashish24

aashish24 Mar 2, 2016

Contributor

this looks good to me.

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 3, 2016

@doutriaux1 On our side we are happy with the branch and ready to merge it.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Mar 3, 2016

@danlipsa updated the branch and will merge in, I'm happy with it as well

@durack1

This comment has been minimized.

Member

durack1 commented Mar 4, 2016

@danlipsa @doutriaux1 if you've been looking at bg=0 cases I wonder if it would also be worthwhile taking at look at #1397 - While you're peeking at this it might be an easy fix

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Mar 5, 2016

@aashish24 it now says: This branch is even with Kitware:master.

Did you trigger a rebuild? So that we know if the baselines need tweaking? Thanks.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Mar 5, 2016

@aashish24 it now says: This branch is even with Kitware:master.

@doutriaux1 which branch you are referring to? May be you posted this on the wrong issue?

@danlipsa

This comment has been minimized.

Contributor

danlipsa commented Mar 7, 2016

@doutriaux1 @aashish24 So, do I merge this?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Mar 7, 2016

@aashish24 I can't see the failed test for garant, can you please retrigger?

@jbeezley

This comment has been minimized.

Contributor

jbeezley commented Mar 7, 2016

It's here. You can usually find the report by looking for the sha. Note that the current master is failing as well

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Mar 7, 2016

ok does not look like anything related to this @chaosphere2112 looks like your PR broke flake8 and also somehow the baseline is missing for it, but I don't see it in PR of uvcdat-testdata.
@danlipsa I think you can merge in.

danlipsa added a commit that referenced this pull request Mar 7, 2016

Merge pull request #1862 from UV-CDAT/labels-off
BUG #1740: plotting with bg=0 produces labels off

@danlipsa danlipsa merged commit bce87e1 into master Mar 7, 2016

2 of 3 checks passed

continuous-integration/kitware-buildbot/uvcdat-garant-linux-release/ Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment