bugGNU Octave - Bugs: bug #52940, Regression with...

 
 

bug #52940: Regression with "PaperPosition" and printing

Submitter:  Rik <rik5>
Submitted:  Sat 20 Jan 2018 05:25:00 PM UTC
   
 
Category:  Plotting with OpenGL Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 26 Feb 2018 07:36:12 PM UTC, comment #50: 

I've pulled the hidden camera properties hunks from a changeset and reapplied in the attached patch.

(file #43401)

Dan Sebald <sebald>
Tue 06 Feb 2018 06:28:13 PM UTC, comment #49: 

@Dan: Stripping trailing whitespace is a periodic chore.  I just did it for all the files here (http://hg.savannah.gnu.org/hgweb/octave/rev/b658e0fa0cb3).

Rik <rik5>
Group administrator
Tue 06 Feb 2018 06:17:34 PM UTC, comment #48: 

@Dan: Before committing the cset in comment #46 which would remove certain hidden transform properties, can you post something to the Octave Maintainer's list about this?  I want to make sure we aren't accidentally overlooking something.  It make also be that we want to cache some of these transform values for performance reasons, but haven't gotten around to that yet.

Rik <rik5>
Group administrator
Tue 06 Feb 2018 12:30:53 AM UTC, comment #47: 

Also, the recent changesets had a few extra whitespace characters after the last non-whitespace character.  Attached is a patch to remove them.  I diffed back about fifty versions to find another one.  (In linux, my pager highlights these instances when doing "hg diff" before writing the changeset.)

(file #43187)

Dan Sebald <sebald>
Mon 05 Feb 2018 11:41:31 PM UTC, comment #46: 

As part of the previous changesets I attached was the removal of some superfluous figure properties for the transform.  Attached is a patch with just those changes.  I don't see any reason to keep unnecessary properties around.  If there is some package out there using these hidden variables, then it might be better to just recompute said variables from the camera angle information.

(file #43185)

Dan Sebald <sebald>
Mon 05 Feb 2018 10:06:22 PM UTC, comment #45: 

@Rik: I tried the following:


hf = figure ("papertype", "a0");
sz = get (hf, "papersize");
ndiv = 8;
xt = linspace (0, sz(1), ndiv)(2:end-1);
yt = linspace (0, sz(2), ndiv)(2:end-1);
pos = [xt(1) yt(1) (sz(1) - 2*xt(1)) (sz(2) - 2*yt(1))];
set (hf, "paperposition", pos)
hax = axes ("position", [1e-3 1e-3 1-3e-3 1-3e-3], "xlim", [xt(1) xt(end)], ...
            "ylim", [yt(1) yt(end)], "xtick", xt, "ytick", yt,
           "linewidth", 3);
grid on
box on
print toto.pdf


In this example, the corners size should be 1 x-division  by 1 y-division, which works for me (see attached figure).
I cheated a bit the axes position to workaround what looks like bug #52303 (invisible axes lines).

(file #43183)

Pantxo Diribarne <pantxo>
Group Member
Mon 05 Feb 2018 08:23:09 PM UTC, comment #44: 

I corrected some spelling mistakes in the commit message and then pushed the cset here http://hg.savannah.gnu.org/hgweb/octave/rev/8b935067a257.

The original issue of this report, "PaperPosition" has been fixed, so I will close this report.

@Pantxo: Go ahead and update bug #45285 and bug #49622 now that this is checked in.

Also, try testing a little bit with large papersizes.  I tried using A0 as the "papertype" and printing to PDF didn't exactly ignore the "paperposition" I set, but it seemed to take it more as a suggestion.  For example, I set the lower left corner at [0.25, 0.25] and the eventual position appeared larger (although still down in the lower right).

Rik <rik5>
Group administrator
Fri 02 Feb 2018 10:10:08 AM UTC, comment #43: 

Can someone test the patch from comment #41 ? Does it do what was foreseen in this report (no window resize at print time, partial support for off-screen rendering without osmesa, what else?) ?

At least preventing the window from having to be resized has very interesting side effects: the size of the printing canvas is not bound to the maximum achievable window size. This will solve  bug #45285 and workaround bug #49622. It will also be possible to print to large pages (currently you can't print to e.g. A3 paper with Qt) and make it much easier to implement a consistent (not screen dependent) "-bestfit" print option.

Pantxo Diribarne <pantxo>
Group Member
Thu 01 Feb 2018 10:05:42 AM UTC, comment #42: 

I did not test the patch in comment #41 thoroughly but the following works for me:


plot (1:10)
print /tmp/default.pdf # No flickering, pdf OK
set (gcf, "paperpositionmode", "auto")
## Stretch the figure taller so that the effect is visible
print /tmp/paperpositionmode_auto.pdf # No flickering, pdf OK
set (gcf, "visible", "off")
set (gcf, "paperpositionmode", "manual")
print /tmp/default_invisible.pdf # No flickering, pdf OK


As suggested by Dan, printing Qt invisible figures without the use of osmesa works but there is a restriction: the figure must have been visible once, so that the OpenGL context is OK.

This restriction could somewhat be lowered in the future by using a single QOpenGLWidget for all prints of all figures (we don't need any kind of decoration, just one valid opengl context that can be used by gl2ps). This way, there is only one window that has to flash at the beginning of an Octave session for off/on-screen printing to be enabled. The flashing QOpenGLWidget could even be hidden behind the main GUI window when the latter is run.

Unfortunately, this is not a proper replacement of osmesa when it comes to generating the manual: the whole point of using "visible"->"off" when generating the figures of the manual is to not have figures flashing on-screen during the build process.

Pantxo Diribarne <pantxo>
Group Member
Wed 31 Jan 2018 10:34:38 PM UTC, comment #41: 

I attached a patch in which I did the following:

  • prevent figure windows from being updated (no resize) at print time. For this I just added a "__printing__" hidden property which is checked when updating the figure window.
  • allow figures that have been visible at least once (with a well setup opengl context) to be printed without osmesa when they are invisible. For this I added a "__gl_window__" hidden property that is set "on" once the figure has been shown.






(file #43147)

Pantxo Diribarne <pantxo>
Group Member
Mon 29 Jan 2018 07:07:57 AM UTC, comment #40: 

I hadn't noticed that the previous small changeset was applied to the canonical version.  I'm attaching a new version with the main difference that the following is restored:


@@ -25,6 +25,9 @@ function opts = __opengl_print__ (opts)

   dos_shell = (ispc () && ! isunix ());

+  set (0, "currentfigure", opts.figure);
+  drawnow ("expose");
+
   if (! isempty (opts.fig2dev_binary))
     ## fig2dev is prefered for conversion to emf
     fig2dev_devices = {"pstex", "mf", "emf"};


because it helped for non-visible print.  I don't know why, but I didn't investigate either.

(file #43102)

Dan Sebald <sebald>
Mon 29 Jan 2018 05:41:11 AM UTC, comment #39: 

"At first glance, I am not fond of the idea of moving the routines that compute opengl matrices from axes objects, where they are executed only when some property change justifies it, to opengl_renderer, where those routines will be executed each and every time the graphics toolkit calls opengl_renderer::draw."

But this is not a real taxing piece of code relative to the rendering process (see new routine axes::properties::get_transform in the changeset).  The first thing the renderer does is get its transform.  And this is what allows the print to be done without any sort of set/reset of figure properties.  The opengl_renderer will default to using the "Position" property and the derivative gl2ps_renderer will instead pass in parent_pix_size (a coding paradigm that was already present in the axes::properties code) representing "PaperPosition".  Also, it's good object-oriented-programming practice to not put too much functionality into one object.  Instead, inheriting and building upon other objects little by little is better.

"Is this all just about avoiding the figure window being rescaled?
Why not simply implement something like graphics_tookit::block_size_updates () so that we have the graphics objects right and the figure window size is not updated ?"

That's not enough?  Sure, in some sense it doesn't matter, but it doesn't suggest quality software to have the figure momentarily scale half off the desktop.  But it is more than that.  Hopefully a bit of restructuring here can make non-visible work, so that Octave doesn't need OSMesa.  I'm not sure how active OSMesa development is, but the Nvidia driver failure is a lingering issue.

If you think some kind of block_size_updates for printing is straightforward, it might be an alternative.  Keep in mind though that there are listeners (some potentially set by the user) that could be activated whenever a set(gcf,"property",...) is done.  So, that might need to be addressed in such a scheme.  These listeners aren't supposed to be activated by a "print" command, I don't think.  To me, it seems easier to just print in the background, internally, with PaperPosition instead of the on-screen's use of Position.

Dan Sebald <sebald>
Mon 29 Jan 2018 05:21:25 AM UTC, comment #38: 

OK, I'm attaching a new changeset that implements what I had described.  The elements in axes::properties that seem more geared for the renderer have been moved to the graphics_xform.  That way each opengl_renderer and derivative object can get its own transform.  The key change here to make gl2ps_renderer work without the change in figure "position" property is the following:


  (opengl_renderer::setup_opengl_transformation): Move the storage of xform
  to the start of the function then use it to retrieve x_zlim, x_gl_mat1 and
  x_gl_mat2.  Note, parent_pix_size is passed into get_transform() to get
  transform appropriate for on-screen or print mode.


So, that's the basic design change.  All else is either a regrouping of parameters for convenience or a removal of things that no longer need to be done.  There is a lot of cleanup that anyone interested could do to make this approach better, but this should do.

I think there is a bug or two currently covered up for FLTK graphics toolkit by the pre-patch approach of setting and restoring figure properties.  I left a note in the changeset:


-    ## print() requires figure units to be "pixels"
-    props(end+1).h = opts.figure;
-    props(end).name = "units";
-    props(end).value = {get(opts.figure, "units")};
-    set (opts.figure, "units", "pixels");
-    nfig += 1;
-
     ## graphics toolkit translates figure position to eps bbox (points)
+    ## 1/28/18  This should no longer be necessary, but the FLTK toolkit
+    ## on-screen axes position shifts if this is not restored.
     fpos = get (opts.figure, "position");
     props(end+1).h = opts.figure;
     props(end).name = "position";
     props(end).value = {fpos};
-    fpos(3:4) = opts.canvas_size;
-    set (opts.figure, "position", fpos);


By my way of thinking, there should no longer be a need for changing figure "units" or "position".  The axes::properties bounding-box code seems to address this by converting any units to pixels.  However, although the position property no longer needs to be set for printing, for FLTK toolkit there is a shift in the on-screen plot without restoring.  The Qt toolkit seems fine though.  I don't want to concern myself with that though, as it isn't the real issue--so I left that in for now.

I did test the non-screen-visible printing for Qt and FLTK.  It seems to work for Qt, but not for FLTK.  So I've made a change that allows Qt toolkit to do non-visible printing rather than use _osmesa_print_.  Might this obviate the manual compilation issue for those with Nvidia drivers?

Rik, I'm tempted to drop the use of _guidata_ and just compute the parent position size internally using the PaperPosition property.  What do you think?

(file #43101)

Dan Sebald <sebald>
Mon 29 Jan 2018 03:13:56 AM UTC, comment #37: 

colorbar() is not really a problem.  There are already listeners on the axes that do the job:


addlistener (hax, "dataaspectratio", {@cb_colorbar_axis, hcb, props});
addlistener (hax, "dataaspectratiomode", {@cb_colorbar_axis, ...
                                          hcb, props});
addlistener (hax, "plotboxaspectratio", {@cb_colorbar_axis, hcb, props});
addlistener (hax, "plotboxaspectratiomode", {@cb_colorbar_axis, ...
                                             hcb, props});
addlistener (hax, "position", {@cb_colorbar_axis, hcb, props});


I have re-written a lot of colorbar and legend for the 3.4.0 release, but legend still needs a listener on position.  The reason I haven't done it is because legend itself will need a lot of re-writing.  Frankly, I grew tired and daunted by the amount of work there.

Rik <rik5>
Group administrator
Sun 28 Jan 2018 10:00:52 PM UTC, comment #36: 

@Rik, @Dan:

>> A useful reason to pursue Dan's strategy would be the printing of legend objects ...


Rescaling figures at print time will always have to happen unless the aspect ratio of "paperposition" and "position" figure properties match (at the very least). For this reason "position" needs to be correct so the listeners that rely on it (in user code or internal code) can do their job.
"legend" and "colorbar" have to be fixed, at least for on-screen experience (look how ridiculous legends become when you rescale a figure by hand) and no print-time trick will workaround this.


>> I agree with your comment #32 that the best course is probably " Move the update_aspectratios(), update_camera() and update_axes_layout() code from axes::properties to the opengl_renderer object"


At first glance, I am not fond of the idea of moving the routines that compute opengl matrices from axes objects, where they are executed only when some property change justifies it, to opengl_renderer, where those routines will be executed each and every time the graphics toolkit calls opengl_renderer::draw.

Is this all just about avoiding the figure window being rescaled?
Why not simply implement something like graphics_tookit::block_size_updates () so that we have the graphics objects right and the figure window size is not updated ?

I didn't take the time to read all the posts but my first impression is that if a big change has to happen then we should also try looking at what exists in the modern opengl API. There are now objects (framebuffer objects and associated renderbuffer objects) that allow rendering offscreen and without bothering about the size of the windowing system provided framebuffer (the default framebuffer object). I don't really know how big of a change this is, but it is definitely a route we should investigate.


Pantxo Diribarne <pantxo>
Group Member
Fri 26 Jan 2018 09:37:24 PM UTC, comment #35: 

On my fedora 27 system these changes change the plot
position by a very large amount for both qt and fltk.
It may be helpful to look at the determination of plot
position while making these changes. Ideally, fltk and qt
when plotting the same data, would produce the same
plot positions.
I use: ps = get(gcf, 'Position') to obtain the position
coordinates.

Michael Godfrey <godfrey>
Group Member
Fri 26 Jan 2018 05:55:09 PM UTC, comment #34: 

I have some mods I made, attempting to keep them short, but it's only in process right now as I've set it aside for this weekend.  My first step has been to group


+  Matrix x_gl_mat1;
+  Matrix x_gl_mat2;


with the graphics_xform and then move all those


  Matrix get_transform_matrix (void) const { return xform; }
  Matrix get_inverse_transform_matrix (void) const { return xform_inv; }
  Matrix get_opengl_matrix_1 (void) const { return x_gl_mat1; }
  Matrix get_opengl_matrix_2 (void) const { return x_gl_mat2; }
  Matrix get_transform_zlim (void) const { return zlim; }


from the axes object to the graphics_xform.  That way when the renderer gets its own unique graphics_xform it doesn't have to refer back to the axes properties.  I've also added a bounding box to axes.get_transform(Matrix& bb) so that gives a route into the transform computations without going through the "set(fig,"Position",bb)" route.

The second patch you are referring to (where the Figure doesn't change size with "print()") doesn't and can't be made to work for the reason's I described in #32.  So, it is only the very short patch that gets back to the original behavior, the first one you cleaned, which could be applied.  Anyway, this weekend I should have a changeset that isn't too drastic but gets the job done.

Dan Sebald <sebald>
Fri 26 Jan 2018 05:34:01 PM UTC, comment #33: 

The patch in comment #29 no longer cleanly applies.  I refreshed it and uploaded it as paperpos.hack.diff.

The patch works in the sense that the figure doesn't undergo any contortions when a print job is executed.  But, the bad news is that the output is not scaled appropriately.

I agree with your comment #32 that the best course is probably " Move the update_aspectratios(), update_camera() and update_axes_layout() code from axes::properties to the opengl_renderer object"

@Pantxo: You know the most at this point about printing through gl2ps.  Does this seem like the right idea?


(file #43088)

Rik <rik5>
Group administrator
Thu 25 Jan 2018 10:43:20 AM UTC, comment #32: 

After looking at the graphics code and gl2ps_print code for a while, I think we should probably apply the current small change as an intermediate step.  After that, there would need to be a bigger change--not huge, but it would be shaking the tree a bit.

I just couldn't find a way to make that gl2ps_print function work outside of the opengl graphics toolkit.  The reason is that the gl2ps_print() routine is fed a graphics_object reference that is const, i.e.:


  void
  gl2ps_print (const graphics_object& fig, const std::string& stream,
               const std::string& term)
  {


Hence there is no way in C++ land to modify "position" property and rerun the "update transform" that is important for making the draws have the proper scale during print.  The compiler won't allow changing the property values directly, and it also won't allow reruning any of the graphics routines complaining about a "const 'this' pointer" not suitable for member functions.  So, currently the only way of changing the layout and internally re-running the update-transform is from Octave command-line land.  Hence, as things are currently setup, there is no way of not modifying the on-screen figure to get the proper transform scaling.

Yes, there is some internal Booleans that disable updating the toolkit, etc.  But those don't seem the most convenient, and messing around with that I've found there were artifacts like the background of the plot outside the borders turning out black in the PDF file.  I also started down the route of dropping the "const" from "const graphics_object& fig", but that's really not the proper thing to do.  I then thought to pass another Boolean into the draw() routines to indicate that the PaperPosition should be used instead of Postion for the transform, but that too isn't very good.

Here's what the problem is (then I'll describe two ways of changing this).  As member functions of the axes::properties object are these functions:


    void update_transform (void)
    {
      update_aspectratios ();
      update_camera ();
      update_axes_layout ();
    }


which manage that important information about transformation matrices and so on.  Because the on-screen and print renderings are a different size, these routines must be run for both.  As described above, the only way of causing that code to run is by changing the figure's Position property.  Doing so causes an update of the toolkit figure.  With the current setup that code can't be run in a straightforward way from a C++ routine; that's the problem.

One main point is that it seems that the camera and layout internals (i.e., the transform matrices) really aren't part of the properties.  Instead, those seem more like something that should be associated with the renderer.  So that is the first approach:

1) Move the update_aspectratios(), update_camera() and update_axes_layout() code from axes::properties to the opengl_renderer object (of which gl2ps_render inherits).  That way, both the toolkit's opengl_renderer and the print's gl2ps_renderer can have their own associated transform formula (the former using Position, the latter using PaperPosition) independent of and not interfering with the other renderer.  They both use the same figure properties, but the transforms are different.

The above seems the proper approach simply because of the natural association of the transform and object.  Another approach that doesn't seem as elegant for some reason is

2) Move the gl2ps_print method to a member function of the graphics.cc hierarchy.  When the print is a member function of the graphics objects it would have non-const access to the internal data and member functions update_camera() and so on without having to go through the "set(fig,"position",...)" script interface.

Dan Sebald <sebald>
Wed 24 Jan 2018 09:23:24 PM UTC, comment #31: 

I changed the Severity from Blocker to Normal since we are now back to where we started. 

A useful reason to pursue Dan's strategy would be the printing of legend objects which have no listener on the axes size.  When Octave scales a plot for printing, and does so by directly modifying the axes or figure "position" property, a whole bunch of listener's get called that re-draw the figure.  This is slow, and doesn't always work.  It would be a great improvement to get rid of that approach if possible.


Rik <rik5>
Group administrator
Wed 24 Jan 2018 05:33:27 PM UTC, comment #30: 

I should add, my thought with __guidata__ being a structure is that perhaps we can get rid of all the store props() / set properties / restore properties code and instead pass all that info as elements of __guidata__.

Dan Sebald <sebald>
Wed 24 Jan 2018 05:30:00 PM UTC, comment #29: 

OK, that's more compact like I was looking for, thanks.

To follow up on your idea to use __guidata__, attached is a proof-of-concept sort of changeset with a lot of cruft to illustrate the idea.  Rather than just send a matrix via __guidata__, it sends a structure and "position" is one of the elements of that structure.  It could use a few more checks (is-a-matrix, has at least four elements, etc.) to ensure no crash from improper __guidata__ format.  Nonetheless, please try this and see if it has potential.

First, the on-screen figure doesn't change size (that's nice!).  However, although the paper size of the PDF output is correct, the figure itself is drawn at the on-screen figure size.  The next step would be to get/pass information about plot scaling from somewhere else other than the on-screen image.  My guess is that would be doable, but I haven't looked to deep into that.

(file #43067)

Dan Sebald <sebald>
Wed 24 Jan 2018 03:41:38 PM UTC, comment #28: 

Attached is 52940.cset that makes the simplification I suggested in comment #27.  It passes the tst_paperpos.m test.

(file #43065)

Rik <rik5>
Group administrator
Wed 24 Jan 2018 03:37:37 PM UTC, comment #27: 

Looking at your patch, I wonder if we can simplify it by borrowing from screen_size_pixels in graphics.cc?

Your code:


    const figure::properties& figProps =
      dynamic_cast<const figure::properties&> (fig.get_properties ());
    Matrix lpos = figProps.get_position ().matrix_value ();
    rend.set_viewport (lpos(2), lpos(3));


And here is screen_size_pixels in graphics.cc


// This function always returns the screensize in pixels
static Matrix
screen_size_pixels (void)
{
  graphics_object obj = gh_manager::get_object (0);
  Matrix sz = obj.get ("screensize").matrix_value ();
  return convert_position (sz, obj.get ("units").string_value (), "pixels",
                           sz.extract_n (0, 2, 1, 2)).extract_n (0, 2, 1, 2);
}


As a first simplification, you already have a graphics_object so I don't think you need to get the full property list and then use the special get_position() function to extract one of them.  Just use get() along with the object.


    Matrix pos = fig.get ("position").matrix_value ();
    rend.set_viewport (pos(2), pos(3));


The second simplification, might be to use convert_position in C++ to calculate the size you want rather than using the m-file code and the listeners on the position and units properties.

Rik <rik5>
Group administrator
Wed 24 Jan 2018 03:20:25 PM UTC, comment #26: 

If you want to experiment with an out-of-band communication protocol you could use the hidden figure property _guidata_ which is normally used to implement the guidata() function.

In libinterp/corefcn/graphics.in.h:3173


      any_property __guidata__ h , Matrix ()


To pass data,


set (gcf, "__guidata__", [0, 0, 1200, 800])


If this eventually works we could add a hidden property just for printing such as _canvas_size_.  Although my guess is that in the long term we might want to convert any calculations of size in to a C++ function and make that available to m-files if they really need it.

Rik <rik5>
Group administrator
Wed 24 Jan 2018 08:54:46 AM UTC, comment #25: 

On the matter of the OpenGL window (Qt or FLTK) momentarily changing size, it looks like this is simply a matter of having the computed coordinates done inside a script file (__print_parse_opts__.m) which must then be relayed to some C++ compiled op-code (gl2ps_print.cc).  That's a big part of the issue--the print.m routine is using some properties temporarily as data transfer mechanism.

Inside __print_parse_opts__.m is the following code:


function [papersize, paperposition] = gs_papersize (hfig, paperorientation)
  persistent papertypes papersizes;

  if (isempty (papertypes))
[snip]
  endif

  papertype = get (hfig, "papertype");
  paperunits = get (hfig, "paperunits");
  paperposition = get (hfig, "paperposition");
  if (strcmp (papertype, "<custom>"))
    papersize = get (hfig, "papersize");
    papersize = convert2points (papersize , paperunits);
  else
    papersize = papersizes (strcmp (papertypes, papertype), :);
  endif
[snip]
endfunction


which contains the important logic for translating printing paperposition to point-size values.  This script file will return that information in the opts.canvas_size field.

Here is how print() treats that information:


function print (varargin)

  opts = __print_parse_opts__ (varargin{:});

[snip]
    ## graphics toolkit translates figure position to eps bbox (points)
    fpos = get (opts.figure, "position");
    props(end+1).h = opts.figure;
    props(end).name = "position";
    props(end).value = {fpos};
[the above is temporary storage]
    fpos(3:4) = opts.canvas_size;
    set (opts.figure, "position", fpos);
[the above is using "position" as a means of communication]
[snip]


and further along after the off-screen print is complete all those "props" are restored.  The problem is that something like "position" has a listener that will redraw the on-screen figure in response to the above "set (opts.figure, "position", fpos)".  That's where the on-screen contortions are coming from.

This notion of using the properties temporarily to pass along info isn't so nice.  I'm sitting here thinking that instead of property "position" one could use "paperposition" as a temporary holder because that then won't affect the on-screen figure window.  But still, that's not the best because these things could have listeners and such as well.  Can anyone think of a better scheme?  Couldn't there be some Octave function interface that just passes "opts" information directly in rather than use figure properties?

Dan Sebald <sebald>
Wed 24 Jan 2018 05:44:19 AM UTC, comment #24: 

I'm attaching a changeset having some additional mods that always use drawnow() for printing regardless of figure visibility, i.e., no use of OSMesa.  For the Qt toolkit, the "print tst.pdf" will still produce the correct plot when the figure is not visible.  However, FLTK toolkit produces the following warning message:


      if (state == GL2PS_NO_FEEDBACK && props.is_visible ())
        warning ("gl2ps_renderer::draw_axes: empty feedback buffer and/or nothing else to print");


and produces and empty page in PDF.  It seems like a bit of progress worth pursuing though.  All that may be needed is getting the proper viewport dimensions to the gl2ps_print() routine without having to change the Figure size as an intermediary.

BTW, I sat wondering about the above warning for a bit.  That warning is in draw_axes() such that props is the axes properties.  Looking at the test, the axes "is_visible" even though the figure is not.  Does that seem correct?  I.e.,


octave:6> graphics_toolkit qt
octave:7> plot(1:5)
octave:8> ha = gca();
octave:9> set(1,'visible','off')
octave:10> get(ha,'visible')
ans = on
octave:11> get(1,'visible')
ans = off


(file #43047)

Dan Sebald <sebald>
Wed 24 Jan 2018 04:17:13 AM UTC, comment #23: 

OK, thanks for the reminder folks.  The crash is probably the Nvidia driver issue.  I thought OSMesa was a non-GUI use, but apparently it is non-visible use.

Dan Sebald <sebald>
Wed 24 Jan 2018 03:40:14 AM UTC, comment #22: 

Sorry, I need to do both
LIBGL_ALWAYS_SOFTWARE=1 LD_PRELOAD=/usr/lib64/libGLX_mesa.so.0

to get a correct pdf.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 24 Jan 2018 03:35:58 AM UTC, comment #21: 

If I LD_PRELOAD=/usr/lib64/libGLX_mesa.so.0
then test code in comment #18 works for me (produces a centered plot on a paper). If I do not preload, then plot is off center on screen and in tst.pdf. Also after set(1,'visible','off') I get
the usual:


octave:3> print tst.pdf
error: __osmesa_print__: Depth and stencil doesn't match, are you sure you are using OSMesa >= 9.0?
error: called from
    __opengl_print__ at line 161 column 7
    print at line 552 column 14
octave:3>


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 24 Jan 2018 03:20:23 AM UTC, comment #20: 

The test code in comment #18 works for me.  This may be due to differences in Qt, OpenGL, OSMesa, or video drivers.

Rik <rik5>
Group administrator
Wed 24 Jan 2018 02:28:05 AM UTC, comment #19: 

I spoke too soon.  That crash bug in https://savannah.gnu.org/bugs/index.php?52940#comment18 is regardless of any changes I made.  I pulled the latest code, built and tested; the crash is there.  Is this a known bug and the reason for the use of OSMesa?

Dan Sebald <sebald>
Wed 24 Jan 2018 02:01:22 AM UTC, comment #18: 

That "drawnow()" is actually currently more critical than just what I had described.  Before and after the patch, if I remove the drawnow() and turn visibility off, I get:


octave:1> graphics_toolkit qt
octave:2> tst_paperpos
octave:3> set(1,'visible','off')
octave:4> print tst.pdf
fatal: caught signal Segmentation fault -- stopping myself...
Segmentation fault


Obviously the issue is that without the visible figure there is no OpenGL redraw and something isn't correct for the gl2ps context.  I suspect it is because Octave is using the Figure position in some way.  Let's try to get away from that and obviate the above seg-fault.  Regardless of changing the viewport or not, there shouldn't be a crash bug in the code that is dependent upon something set in a script file.

Dan Sebald <sebald>
Wed 24 Jan 2018 01:40:42 AM UTC, comment #17: 

OK, I have a patch.  There will be another rev, but Rik please give the changeset a try.  Before even looking at the plot I knew it would be correct because the number's I was printing out looked correct, i.e., no more 485 but always 396.

Finding the location to set the viewport wasn't too bad.  Also, setting the viewport is pretty much the same as the GLCanvas.  Given the gl2ps_renderer is derived from opengl_renderer, we can use the same member function.  However, someone please tell me if there is a better way of getting the desired parameters (i.e., the 720 and 396) rather than having to get all those figure properties.  Also, it would be nice to use something other than the Figure's position because we might then be able to get away from having to do those contortions on the Figure before calling the gl2ps_renderer.  Is there some way we can use the PaperSize and still get the same info?  Would be nice.

Something else I did was remove that first "drawnow" at the start of _opengl_print_.m.  I believe that was covering up this bug.  That is, in order to get the proper viewport, this drawnow() command was setting up a viewport in OpenGL memory that later was cleared, but by chance happened to be in the right memory location so that when the next OpenGL context was created those viewport settings were correct.  Not a recommended coding technique.  :-)

Before specifically setting the viewport when I tested the removal of that drawnow(), FLTK toolkit produced an incorrect result.  But with the attached changeset, FLTK still looks correct.  Qt should look correct as well.

This may go a long way to fixing a few things that seemed like annoying bugs in gl2ps and hopefully we can get rid of the on-screen size change.  So, some feedback, and then I'll make another rev.

(file #43042)

Dan Sebald <sebald>
Tue 23 Jan 2018 10:31:55 PM UTC, comment #16: 

@Dan: Thanks, it looks like you've done quite a bit of nasty debug work to localize the problem.

Rik <rik5>
Group administrator
Tue 23 Jan 2018 10:23:54 PM UTC, comment #15: 

After quite a bit of printing out numbers, I think I'm getting close.  I just want to write a short note here so I can pause and come back to this.

The way that Qt toolkit is setup (in on-screen mode) there are some signal/slots that basically trigger a Figure::print, which calls GLCanvas::print which goes back to


  void
  opengl_renderer::draw_figure (const figure::properties& props)
  {
GLint vp[4];
glGetIntegerv (GL_VIEWPORT, vp);
std::cerr << "draw_figure viewport = " << vp[0] << " " << vp[1] << " " << vp[2] << " " << vp[3] << "\n";
    // Initialize OpenGL context


where I printed out the viewport.  The on-screen viewport looks correct.  But immediately when doing the gl2ps print that viewport is wrong (see previous posts for the numbers).

Following the program flow, I found this routine for which I printed out similar information:


  void
  GLCanvas::draw (const graphics_handle& gh)
  {
    gh_manager::auto_lock lock;
    graphics_object go = gh_manager::get_object (gh);

    if (go)
      {
        octave::opengl_renderer r;

std::cerr << "GLCanvas::draw width " << width() << " height " << height() << "\n";
        r.set_viewport (width (), height ());
        r.draw (go);
      }
  }


For the first portion of on-screen draws (what I've been calling contortions), I see:


GLCanvas::draw width 560 height 420
GLCanvas::draw width 720 height 396
GLCanvas::draw width 720 height 396
GLCanvas::draw width 720 height 396


at which point I had a bunch of pause() in place so that I knew the off-screen gl2ps_print hadn't occurred yet.  Then after hitting enter it is a bunch of printouts but no GLCanvas::draw appears; at least not until coming back to the normal size:


GLCanvas::draw width 560 height 420
GLCanvas::draw width 560 height 420
GLCanvas::draw width 560 height 420


OK, so my theory at the moment is that there is a missing


        r.set_viewport (width (), height ());


that needs to be introduced either through calling that   GLCanvas::draw or by introducing a similar function call in the gl2ps methodology.  When I have more time, I will look at how gl2ps and on-screen compare.  This bug has probably always been present, but it may be the case Octave was just lucky in the sense that the gl2ps context may have reused a block of OpenGL memory that happened to be the same context for the on-screen renderer and the viewport was still properly set (note how octave::opengl_renderer r is on the stack so goes away).  Then, with the new QOpenGLWidget that memory could be partially overwritten with some bogus number between the two contexts.

Anyway, in summary, find where that viewport is set for the gl2ps OpenGL context, and if it can't be found, set the viewport.

Dan Sebald <sebald>
Tue 23 Jan 2018 06:29:36 PM UTC, comment #14: 

Question: At the start of _opengl_print_() is a drawnow() call:


function opts = __opengl_print__ (opts)

  dos_shell = (ispc () && ! isunix ());

  set (0, "currentfigure", opts.figure);
  drawnow ("expose");


Any recollection why that is done?  I remove it and no longer does the FLTK graphics toolkit go through the on-screen contortions.  However, the FLTK PDF output then doesn't look correct (right edge is missing).  I'm just wondering why this can't be taken care of inside drawnow() so that it only needs to be called once when wanting the EPS/PDF output.

Commenting out that drawnow() doesn't affect the QT toolkit; the Qt window still goes through the contortions.  Probably there is some redundant action going on with Qt, i.e., draw on-screen twice then draw to file.

@Rik, I'm attaching a changeset that removes some old comments about "remove after we are certain the fix works".  I checked the Mercurial record and those comments are over a year old.  I hope things are stable in that regard.  Even if not, an arbitrary wait time to cover up some bug is not a good fix, so I'd hope that octave_sleep() call wouldn't be re-implemented.

(file #43037)

Dan Sebald <sebald>
Tue 23 Jan 2018 04:29:41 PM UTC, comment #13: 

Well at least it is consistent.  The FLTK toolkit is using neither QOpenGLWidget or QGLWidget so it shouldn't be subject to this variability.

For the moment I've worked around things by just commenting out the #define in config.h


/* Define to 1 if you have the <QOpenGLWidget> header file. */
//#define HAVE_QOPENGLWIDGET 1


which then proceeds to build with the old widget.

Rik <rik5>
Group administrator
Tue 23 Jan 2018 09:40:45 AM UTC, comment #12: 

It just occurred to me that I should check if FLTK consistently produces a centered plot.  It appears to:


octave:1> graphics_toolkit fltk
octave:2> tst_paperpos
viewport = 0 0 720 396
octave:3> tst_paperpos
viewport = 0 0 720 396
octave:4> close all
octave:5> graphics_toolkit qt
octave:6> tst_paperpos
viewport = 0 0 720 396
octave:7> tst_paperpos
viewport = 0 0 720 485


The last Qt toolkit plot is the one that is shifted upward.

Dan Sebald <sebald>
Mon 22 Jan 2018 09:57:24 PM UTC, comment #11: 

I too think rolling back probably isn't the thing to do.  It seems tempting, but as I see it the issue still exists for QGLWidget--which is the fact that the on-screen OpenGL rendering goes through those contortions when print is invoked; that shouldn't happen.

Regarding those size numbers, for QGLWidget, the numbers are

viewport = 0 0 560 420
viewport = 0 0 720 396
viewport = 0 0 720 396

but that first 560 420 doesn't seem to make a difference.  What's interesting is that for the QOpenGLWidget numbers

viewport = 0 0 720 396
viewport = 0 0 720 485
viewport = 0 0 720 485

the very first combination (0 0 720 396) works and matches the second set of number of the QGLWidget case.  So, likely the 720 396 is the proper size that is needed to make the nice centered plot.

In the remaining prints for QOpenGLWidget, the numbers are 720 485.  That 485 suggests a bigger view port in the y-direction, and the difference is roughly 90 pixels and yeah, that could be about 1" in PDF layout terms.  But why does the image shift, rather than scale?  Is it because we are just talking the view port and perhaps the coordinate system is the non-Euclidian "(0,0) in upper left corner" that is common to some graphics systems?

Clearly things are set up slightly differently between these two widgets.

Anyway, it seems there might need to be a GLCanvas::initializeGL() that changes the OpenGL context from on-screen to gl2psCONTEXT.  That might stop the redraw of the on-screen image, which is a first good step.  Right now it might be size-change, context-change when it should instead be context-change, size-change.

Dan Sebald <sebald>
Mon 22 Jan 2018 09:07:41 PM UTC, comment #10: 

I'd prefer to not undo the change.  I don't recall precisely, but I think it solved other problems.

Let's figure out what's causing the difference and try to fix that.

It is certainly suspicious that the size changes after the first call for both the new and the old Qt wrapper widgets.

John W. Eaton <jwe>
Group administrator
Mon 22 Jan 2018 05:48:04 PM UTC, comment #9: 

Hmmm.  It's clear that Qt is deprecating QGLWidget and wants everyone to use QOpenGLWidget.  But if they are not perfectly equivalent, maybe the cset in Octave should be backed out until the implications of substitution are more fully understood.

@jwe: Would an hg backout be okay for a while?

Rik <rik5>
Group administrator
Mon 22 Jan 2018 05:38:35 PM UTC, comment #8: 

I've a feeling that QOpenGLWidget isn't designed to be a free substitution for QGLWidget.  Here are some discussions with much more info than I care to know right now:

https://stackoverflow.com/questions/21722852/difference-in-opengl-speed-between-qt-4-5-and-opengl-api

But it does sound from the comment here:

https://stackoverflow.com/questions/9796444/using-opengl-on-qt-without-the-wrappers

that it is possible to use direct OpenGL functions even in the QOpenGLWidget context:

"
The QopenGL widget gives you the context, you then simply override the initialiseGL, paintGL methods and you can use raw openGL calls directly.

The Qt wrappers around PBO and VBOs are convenient to save doing all the manual DLL loading on Windows but aren't really necessary
"

The comment above refers to these functions from this link:


http://doc.qt.io/qt-5/qopenglwidget.html

Protected Functions
  virtual void         initializeGL()
  virtual void         paintGL()
  virtual void         resizeGL(int w, int h)


I see in GLCanvas.cc the implementation of paintGL():


  void
  GLCanvas::paintGL (void)
  {
    canvasPaintEvent ();
  }


but I don't see a GLCanvas::initializeGL() or a GLCanvas::resizeGL().  Perhaps that's what is missing.  After all, the page size changes, etc. to create the PDF file are a new OpenGL context for which sizing is important.  It could be that with the QGLWidget (which also has those virtual methods) it is a matter of luck that no initializeGL() or resizeGL() is needed...and that on the first draw under QOpenGLWidget the comes out correct by chance (at least for me).  It might be that when the OpenGL system switches back to on-screen context that some important re-initialization/re-size is missing.  Furthermore, hopefully with an appropriate GLCanvas::initializeGL() and GLCanvas::resizeGL(), the on-screen contortions of resize and redraw at the larger setup go away during the "print" command.

Dan Sebald <sebald>
Mon 22 Jan 2018 04:47:54 AM UTC, comment #7: 

Oh yeah, I forgot that I was going to give a reference link to the QOpenGLWidget class:

http://doc.qt.io/qt-5/qopenglwidget.html

There is plenty discussion there about subtle differences between QOpenGLWidget and QGLWidget.  One thing that has me wondering is the following comment:

"
When making OpenGL function calls, it is strongly recommended to avoid calling the functions directly. Instead, prefer using QOpenGLFunctions (when making portable applications) or the versioned variants (for example, QOpenGLFunctions_3_2_Core and similar, when targeting modern, desktop-only OpenGL). This way the application will work correctly in all Qt build configurations, including the ones that perform dynamic OpenGL implementation loading which means applications are not directly linking to an GL implementation and thus direct function calls are not feasible.
"

And here's an example:


    void paintGL()
    {
        // Draw the scene:
        QOpenGLFunctions *f = QOpenGLContext::currentContext()->functions();
        f->glClear(GL_COLOR_BUFFER_BIT);
        ...
    }


Not surprisingly, there is no use of QOpenGLFunctions in the Octave code.  Could it be that this offset is addressed in the association between QOpenGLWidget and the QOpenGLFunctions?  I can only guess why the Qt developers chose to do things that way; seems to obviate the idea of these portable OpenGL graphics library functions.

Dan Sebald <sebald>
Mon 22 Jan 2018 04:26:50 AM UTC, comment #6: 

I changed the conditional inside acinclude.m4 to verify it is the QOpenGLWidget object that makes a difference, and it is the difference.

The following may be a clue.  I printed out for both configurations some info I saw in gl2ps_renderer::draw_axes regarding the


      glGetIntegerv (GL_VIEWPORT, vp);
      gl2psBeginViewport (vp);
std::cerr << "viewport = " << vp[0] << " " << vp[1] << " " << vp[2] << " " << vp[3] << "\n";


and the


      gl2psGetOptions (&opts);
std::cerr << "opts = " << opts << "\n";


at various points along the way.  The results are:

QGLWidget

octave:1> graphics_toolkit qt
octave:2> tst_paperpos
viewport = 0 0 560 420
opts = 649
opts = 665
opts = 665
octave:3> tst_paperpos
viewport = 0 0 720 396
opts = 649
opts = 665
opts = 665
octave:4> tst_paperpos
viewport = 0 0 720 396
opts = 649
opts = 665
opts = 665



QOpenGLWidget

octave:1> graphics_toolkit qt
octave:2> tst_paperpos
viewport = 0 0 720 396
opts = 649
opts = 665
opts = 665
octave:3> tst_paperpos
viewport = 0 0 720 485
opts = 649
opts = 665
opts = 665
octave:4> tst_paperpos
viewport = 0 0 720 485
opts = 649
opts = 665
opts = 665


What exactly is happening with the first call versus the following calls, I'm not sure.  (Both QGLWidget and QOpenGLWidget show the same type of difference; it could be that the ViewPort at this stage of ::draw_axes() doesn't mean anything.  It might be in later drawing elements that it comes into play.  (One would think it should matter, though, seeing that gl2psBeginViewport(vp) is called.)  Certainly it doesn't seem that the size in the y-direction matters because the y-axis scale on the plot doesn't change physical scale between the first and second draw--I roughly measured the 0 to 6 distance and they are the same...just one offset from the other.  The translation from that view-port to the page dimensions is probably the important part.

Dan Sebald <sebald>
Mon 22 Jan 2018 12:27:15 AM UTC, comment #5: 

Adding jwe to the CC list as he was the author of cset 59cdf06c940e.

Rik <rik5>
Group administrator
Sun 21 Jan 2018 07:21:54 PM UTC, comment #4: 

A note of clarification, on my system (Qt 5.5.1).  The first time after launch I run the script and look at the resulting PDF file, the plot looks correct.  It's the second time that the plot is shifted upward one inch relative to the frame.  With successive plots the plot remains in the same location, shifted one inch upward.

Something else that is strange is the brief contortions that the on-screen figure window goes through during the "print tst.pdf".  All of the commands that change the figure size properties do nothing until the "print tst.pdf" command occurs.  Then the figure momentarily expands to the 10" x 5.5" size and comes back to the smaller size .  However, I do not see anything but a middle-centered plot when the window expands momentarily.  I would think that the output PDF file would look pretty much the same as what that momentarily expanded Qt figure looks like; so where that offset is coming from, I don't know.

Dan Sebald <sebald>
Sun 21 Jan 2018 05:32:07 PM UTC, comment #3: 

I just confirm Rik's findings...

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 21 Jan 2018 04:56:24 PM UTC, comment #2: 

On my system, which only has QT5.2 and hence no QOpenGLWidget, the original example still works. Maybe the origin of scaling issues I can't reproduce in bug #52193

Pantxo Diribarne <pantxo>
Group Member
Sat 20 Jan 2018 10:56:38 PM UTC, comment #1: 

Using hg bisect the problem changeset was introduced about three months ago with this change


parent: 24142:59cdf06c940e
 use QOpenGLWidget if it is available


My version of Qt is 5.6.1 if that makes a difference.  It seems like we used to pass certain QGLFormat options to the QGLWidget in the constructor, but we have no such options for QOpenGLWidget.  That might be an issue.  In any case, I'm wondering now whether multi-sampling and anti-aliasing are getting enabled with the new QOpenGLWidget.

Rik <rik5>
Group administrator
Sat 20 Jan 2018 05:25:00 PM UTC, original submission:  

The "PaperPosition" no longer works correctly on the development branch.  I attach a test script for producing a very simple plot.


plot (1:10, 'o-')
set (gcf, 'paperunits', 'inches')
set (gcf, 'papersize', [10 5.5])
set (gcf, 'paperorientation', 'landscape')
set (gcf, 'paperposition', [0 0 10 5.5])
print tst.pdf


With 4.2.1 the plot is correctly centered within the page.  With the development version the figure is truncated.  I have also attached the resulting pdf files.

Unfortunately this cost me a lot of time before I realized why figures I had been producing for publication were no longer sized correctly.

Rik <rik5>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by godfrey (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by rik5 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 23 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-02-26 sebald Attached File- Added octave-axes_unused_camera_properties-djs2018feb25.patch, #43401
    2018-02-06 sebald Attached File- Added octave-remove_line_ending_whitespace-djs2018jan05.patch, #43187
    2018-02-05 sebald Attached File- Added octave-unused_transform_figure_props-djs2018jan05.patch, #43185
    2018-02-05 pantxo Attached File- Added toto.pdf, #43183
    2018-02-05 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2018-02-01 pantxo StatusConfirmed Patch Submitted
    2018-01-31 pantxo Attached File- Added print_noresize_invisible.patch, #43147
    2018-01-29 sebald Attached File- Added octave-gl2ps_viewport_print_nonvisible-djs2018jan29.patch, #43102
    2018-01-29 sebald Attached File- Added octave-gl2ps_viewport_print_nonvisible-djs2018jan28.patch, #43101
    2018-01-26 rik5 Attached File- Added paperpos.hack.diff, #43088
        Assigned tojwe None
    2018-01-24 rik5 Severity5 - Blocker 3 - Normal
    2018-01-24 sebald Attached File- Added octave-gl2ps_proof_of_concept-djs2018jan24.diff, #43067
    2018-01-24 rik5 Attached File- Added 52940.cset, #43065
    2018-01-24 sebald Attached File- Added octave-gl2ps_viewport-djs2018jan24.patch, #43047
    2018-01-24 sebald Attached File- Added octave-gl2ps_viewport-djs2018jan23.patch, #43042
    2018-01-23 sebald Attached File- Added octave-drawnow_fixme_comment_removal-djs2018jan23.patch, #43037
    2018-01-22 rik5 Carbon-Copy- Added jwe
    2018-01-20 rik5 Assigned toNone jwe
    2018-01-20 rik5 Attached File- Added tst_paperpos.m, #42987
        Attached File- Added tst.4.3.0.pdf, #42988
        Attached File- Added tst.4.2.1.pdf, #42989

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code