bugGNU Octave - Bugs: bug #51645, zoom to singular region dumps core

 
 

bug #51645: zoom to singular region dumps core

Submitter:  Dmitri A. Sergatskov <dasergatskov>
Submitted:  Wed 02 Aug 2017 09:52:30 PM UTC
   
 
Category:  Plotting with OpenGL Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
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

Sun 06 Aug 2017 09:48:15 PM UTC, comment #14: 

Could have used


        select_object (obj, event, currentObj, axesObj,
                       newMouseMode != NoMode);


rather than


        if (newMouseMode == NoMode)
            select_object (obj, event, currentObj, axesObj);
        else
            select_object (obj, event, currentObj, axesObj, true);


I suppose.

Dan Sebald <sebald>
Sun 06 Aug 2017 08:13:40 AM UTC, comment #13: 

Attached is a changeset for suppressing that

warning: opengl_selector::select: selection buffer overflow

message in mousePressEvent() when the mode has no interest in the object that was selected by the mouse press.

Before and after applying the patch and compiling, create a plot that has multiple curves, such as the


graphics_toolkit qt
plot(randn(30))


example.  Go into Z- mode and keep single-clicking on the group of figures.  Without the changeset the warning message will eventually appear.  With the change set, there should never be a warning message in zoom mode.

(file #41436)

Dan Sebald <sebald>
Sat 05 Aug 2017 09:18:37 PM UTC, comment #12: 

After trying some of this zooming stuff, I'm leaning toward removing the double-click-returns-to-autoscale operation.  I think it might be better that way.  Think about applying the patch and then open this to the discussion list to get feedback.

Dan Sebald <sebald>
Sat 05 Aug 2017 09:16:04 PM UTC, comment #11: 

And the follow-up comment.  Here's another arcane warning message:


octave:4> warning: opengl_selector::select: selection buffer overflow
warning: opengl_selector::select: selection buffer overflow


To generate this, go into Z- mode.  Keep pressing the mouse button to unzoom.  I think once that tiny plot becomes like a blob (i.e., no space between discretized lines), we effectively "select" a whole lot of lines via:


    int hits = glRenderMode (GL_RENDER);


in gl-select.cc, which returns a negative number.  [Note that the returned value of glRenderMode() pertains to the state it is currently in, not the mode passed in, i.e., GL_RENDER.]  That is, because the lines are so scrunched together, one pixel might select several plot objects.

In some sense we don't care about this because the mode is zoom, not any kind of select.  There is a "select" mode, which is the white pointer cursor button.  If we are in that mode, then it might make sense to issue such a warning.

Dan Sebald <sebald>
Sat 05 Aug 2017 09:01:23 PM UTC, comment #10: 

OK.  If users complain about the warning message messing up their screen output (because I myself often highlight text in shell window and use center mouse button to paste), we'll just remove the message.  In a follow-up post I will point out another warning message.

Attached are two changesets.

The first one--with the tag "INTOLERABLE_LAG"--is the implementation which uses QTimers to confirm there is no double click before drawing.  Because the timer needs a slot, there has to be something derived from QObject on the receiving end.  Hence , this sort of construct has to go in GLCanvas, not Canvas.  Anyway, it works, and I've included it here more for reference of how to do such a thing, but honestly it doesn't make the grade.  Double click time is a LONG time.  Do a single click and wait 0.4 seconds; yikes, unresponsive.  Add to that the fact that this sort of delay effectively re-arranges the order of events, which could be very important with regard to key-qualifiers.  This isn't the way to go.

However, in the sequence of PRESS | RELEASE | PRESS | DOUBLE-CLICK | RELEASE, one can ignore that second RELEASE event using an m_doubleClickActive state variable (and it need not be in GLCanvas).  So I've included a second, much smaller changeset to do just that.  I'd say this is much better and tolerable.  In some sense having DOUBLE-CLICK action in Z+ and Z- mode is the source of problem.  There is an Autoscale button easily near by after all.  It might be nicer and faster if someone could quickly repeat zooming without the double click behavior getting in the way.

So, I'd say let's make the smaller of these two changesets the tentative modification, but hold off a little bit and let's think about this.  I mean, I've pushed this new state variable to the lowest level so that it only affects the repeated zooming.  But who knows?  Maybe there are other double-click operations in the plot modes that also suffer from the follow-up RELEASE event.

(file #41426, file #41427)

Dan Sebald <sebald>
Sat 05 Aug 2017 03:40:00 AM UTC, comment #9: 

The double-click event just calls the axis autoscale function.  And if you can take a look into a Qt timer function that would be useful.  I knew when I last looked at this that such a timer was required, but I don't know the Qt API well enough and didn't want to go digging to find the required function.

In the mean time, I'll apply the patch here to stop the segfaults.

Patch applied here (http://hg.savannah.gnu.org/hgweb/octave/rev/a62e03d7d53a).

Rik <rik5>
Group administrator
Sat 05 Aug 2017 01:23:09 AM UTC, comment #8: 

Actually, I think the zoom action described isn't quite correct.  Thinking of it this way


DOUBLE-CLICK = SINGLE-CLICK/RELEASE + DOUBLE-CLICK + SINGLE-CLICK/RELEASE


makes more sense.  The DOUBLE-CLICK causes the zoom to go back to Autoscale, but then a follow-up zoom goes to a seemingly random location.

When I find some time, I'll see if I can put a Qt timer in there and prevent the SINGLE-CLICK/RELEASE until the double-click interval has expired and ignore it if a double-click occurs.

Dan Sebald <sebald>
Sat 05 Aug 2017 12:52:18 AM UTC, comment #7: 

I suppose by moving the mouse and doing multiple clicking one is generating 0 x 0 boxes somehow that aren't visible because they are obscured by the mouse cursor.

One option is to remove the warning message.  Then neither the 100 x 0 nor the 0 x 0 boxes would do anything, it would seem.

Yes, it is possible to move the check for zero width sooner at the mouseRelease level.  But from a proper coding perspective, since the axes::properties::zoom inputs aren't prevented from being zero width or height in some obvious way, it should allow crashing the program.  Who knows when ::zoom might find some other application?

Could still have a check sooner, and maybe move that warning message to Canvas::canvasMouseReleaseEvent() rather than the lower-level zoom routine.

Dan Sebald <sebald>
Sat 05 Aug 2017 12:38:56 AM UTC, comment #6: 

The patch seems to work, but I also am able to trigger the warning on what I believe is acceptable behavior.

A single click should cause a zoom in action.  If I click once and then rapidly move the mouse to a new point, and click again, and move to a new point, and click again then I can usually provoke a warning message.  It seems about every 3 or 4 clicks I get the message.

Maybe the issue is back in the Qt handling of the ButtonUp event in Canvas.cc


  void
  Canvas::canvasMouseReleaseEvent (QMouseEvent *event)
  {
    if ((m_mouseMode == ZoomInMode || m_mouseMode == ZoomOutMode)
        && m_mouseAxes.ok ())
      {
        gh_manager::auto_lock lock;
        graphics_object ax = gh_manager::get_object (m_mouseAxes);

        if (ax.valid_object ())
          {
            axes::properties& ap = Utils::properties<axes> (ax);

            graphics_object obj = gh_manager::get_object (m_handle);

            graphics_object figObj (obj.get_ancestor ("figure"));

            std::string zm = zoom_mode (figObj);

            if (m_mouseAnchor == event->pos ())
              {
                double factor = (m_clickMode ? 2.0 : 0.5);

                ColumnVector p1 = ap.pixel2coord (event->x (), event->y ());

                ap.zoom_about_point (zm, p1(0), p1(1), factor);
              }
            else if (m_mouseMode == ZoomInMode)
              {
                ColumnVector p0 = ap.pixel2coord (m_mouseAnchor.x (),
                                                  m_mouseAnchor.y ());
                ColumnVector p1 = ap.pixel2coord (event->x (),
                                                  event->y ());

                Matrix xl (1, 2, 0.0);
                Matrix yl (1, 2, 0.0);

                xl(0) = std::min (p0(0), p1(0));
                xl(1) = std::max (p0(0), p1(0));
                yl(0) = std::min (p0(1), p1(1));
                yl(1) = std::max (p0(1), p1(1));

                ap.zoom (zm, xl, yl);
              }

            redraw (false);
          }
      }


Maybe the same test should be moved here and if one of the dimensions is 0, then the behavior for a single button press is used (the code for m_mouseAnchor == event->pos).


Rik <rik5>
Group administrator
Sat 05 Aug 2017 12:35:44 AM UTC, comment #5: 

In general what you are saying is true, but I don't know if the double-click is causing a return to the original plot exactly.  It seems to me the plot is return to the first zoomed plot, not the original.  That is, with Z+ active:


<press "Autoscale">
plot goes to original
DOUBLE-CLICK somewhere on plot
plot zooms, then redraws to the same location (call that ZOOM1)
DOUBLE-CLICK
plot zooms to ZOOM2, then goes back to ZOOM1
SINGLE-CLICK
plot zooms to ZOOM2
DOUBLE-CLICK
plot zooms to ZOOM3, then goes back to ZOOM1
<press "Autoscale">
plot goes back to original


I suppose there could be a more useful zooming pattern.  Nonetheless, if you find this inability to differentiate the single click from a pending double click nauseating, I think the way to handle that is to use a Qt timer with doubleClickInterval() to call the single-click operation, and if the doubleclick event comes just cancel the timer so that the single-click operation isn't done.  (Clearly the single click operation is done on the Release event because pressing without releasing goes into zoom-box mode.)

https://wiki.python.org/moin/PyQt/Distinguishing%20between%20click%20and%20double%20click

Can you think of a preferred behavior?

Dan Sebald <sebald>
Fri 04 Aug 2017 11:53:43 PM UTC, comment #4: 

Boy, that's a nasty segfault.  It isn't present in 4.2.1 so this is something new.  I'll look at Dan's patch for this.

Also, even though I kind of hate it, the double-click behavior is to reset the Zoom state.  Try clicking slowly 3 times (to get single clicks) and you will be zoomed in quite far on the graph.  Now do a double click and you will return to the original view of the plot.

What is weird about it is that Octave doesn't know until it gets the second click that this is going to be a double click operation.  So the first click causes the plot to zoom in before the second click causes it to zoom back out.  It is definitely nausea-inducing.

Rik <rik5>
Group administrator
Thu 03 Aug 2017 04:23:53 AM UTC, comment #3: 

The attached patch works for me.  There are a couple of FIXMEs associated with zoom routines asking


  // FIXME: Do we need error checking here?


Perhaps this is the type of bug that the question was getting at.

BTW, what is happening when in Z+ mode and one double-clicks?  It's as if the zoom box is randomly jumping around, or maybe it is supposed to be unzooming or something.

(file #41399)

Dan Sebald <sebald>
Thu 03 Aug 2017 03:40:54 AM UTC, comment #2: 

I changed several of the error() to warning() to see what is trying to be done.  It looks like the zoom attempts to set the two figure properties


warning: set: "dataaspectratio" must be finite
warning: set: "dataaspectratio" must be finite
warning: invalid value for array property "dataaspectratio"
warning: set: "cameraviewangle" must be finite


It seems that this is a general property-setting construct, so I don't know if changing the errors/warnings at this low level makes sense.  Instead, it probably makes sense to find the location those boundary numbers are put into the "set prop" code and first check that it is reasonable there.

Dan Sebald <sebald>
Thu 03 Aug 2017 03:01:04 AM UTC, comment #1: 

Confirmed here on linux mint.  Both vertical and horizontal line regions fail with the same error message.

The error message originates in graphics.cc inside array_property::validate() and is called via the array_property::do_set() function in graphics.in.h.  This doesn't need to be a critical error(...), so I wonder if array_property could return a boolean of xok = false.  But it appears that do_set() also issues an error(...) if the boolean returned from validate is false.

Dan Sebald <sebald>
Wed 02 Aug 2017 09:52:30 PM UTC, original submission:  

When ploting with qt backend, choose zoom (Z+) on plot menu and then select a zoom region which is a straight line:


octave:1> plot(randn(30))
octave:2> error: set: "dataaspectratio" must be finite
terminate called after throwing an instance of 'octave::execution_exception'
Aborted (core dumped)


I cannot reproduce this with fltk backend -- it seems to ignore
such a zoom region.

Dmitri.
--


Dmitri A. Sergatskov <dasergatskov>

 

(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 rik5 (Posted a comment)
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by dasergatskov (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-08-06 sebald Attached File- Added octave-qt_toolkit_nomode_only_warning-djs2017aug06.patch, #41436
    2017-08-05 sebald Attached File- Added octave-qt_toolkit_debounce_doubleclick_INTOLERABLE_LAG-djs2017aug05.patch, #41426
        Attached File- Added octave-qt_toolkit_debounce_doubleclick-djs2017aug05.patch, #41427
    2017-08-05 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2017-08-04 rik5 StatusNone Confirmed
    2017-08-03 sebald Attached File- Added octave-qt_toolkit_zoom_check-djs2017aug02.patch, #41399

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code