bugGNU Octave - Bugs: bug #53778, waitbar produces extra figure in...

 
 

bug #53778: waitbar produces extra figure in 4.3.91

Submitter:  Avinoam Kalma <avinoam>
Submitted:  Sat 28 Apr 2018 11:19:53 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Avinoam Open/Closed:  * Closed
Release:  * 4.3.91 Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 30 Apr 2018 04:10:45 PM UTC, comment #7: 

The examples I've seen on the Internet show a blank title unless "'name', 'my title'" property pair is specified.

Dan Sebald <sebald>
Mon 30 Apr 2018 01:58:54 PM UTC, comment #6: 

I pushed the minimal fix, adding "ishold (hax)" to patch.m and surface.m.  See http://hg.savannah.gnu.org/hgweb/octave/rev/c8fc547ab5dd.

Marking as fixed and closing report.

@Dan: For the figure naming issues, can you query the Octave Maintainers list about what the desired behavior should be?  Then we can sync all of the toolkits to the same title strategy.

Rik <rik5>
Group administrator
Sun 29 Apr 2018 04:35:30 AM UTC, comment #5: 

OK...

On the topic of time, I'm seeing:


>> tic; for i=1:10000; ishold(); endfor; toc
Elapsed time is 3.0081 seconds.
>> tic; fig = gcf(); for i=1:10000; ishold(fig); endfor; toc
Elapsed time is 3.11939 seconds.
>> tic; hax = gca(); for i=1:10000; ishold(hax); endfor; toc
Elapsed time is 8.5717 seconds.


So, not too much difference between using the gcf-input form compared agains the no-input form.  However, the gca-input is almost three times slower.

I have a vague memory of either you or John speeding up some aspect of this.  Maybe somewhere along the way a change decreased the drag due to ishold(hax).  But still, why three times the CPU usage?  It's because of the use of ancestor() script.  Code reuse is good practice, but sometimes code reuse ends up repeating various tests like ishghandle() or isfigure() which can be costly in an interpreted language.  The following is a step in reducing the CPU consumption for ishold(hax):


diff --git a/scripts/plot/util/ishold.m b/scripts/plot/util/ishold.m
--- a/scripts/plot/util/ishold.m
+++ b/scripts/plot/util/ishold.m
@@ -49,7 +49,13 @@ function retval = ishold (h)

       case "axes"
         ax = h;
-        fig = ancestor (ax, "figure");
+        fig = h;
+        while (! isempty (fig))
+          fig = get (fig, "parent");
+          if (strcmp (get (fig, "type"), "figure"))
+            break;
+          endif
+        endwhile

       otherwise
         error ("ishold: H must be an axes or figure graphics handle");


With that change, I'm seeing:


>> tic; hax = gca(); for i=1:10000; ishold(hax); endfor; toc
Elapsed time is 4.22686 seconds.


which isn't too bad, 40% CPU cost.

Nontheless, in the grand scheme, that cost of using ishold(hax) vs. ishold() may be small compared against whatever else is happening.  That is, the graphics of the patch code may be a factor greater than the ishold(hax) cost.

Dan Sebald <sebald>
Sun 29 Apr 2018 03:18:11 AM UTC, comment #4: 

I have to get some further clarification about how figure() and axes() do or do not set the root graphics object property of "CurrentFigure".

I believe we used to use "ishold (hax)", but I removed the hax because that calling form is much slower than just ishold(), like 500% slower in my testing.  So, even if we do have to re-introduce "ishold (hax)" it should only be for the primitive objects which do not create their own axes: image, light, line, patch, surface, text.


Rik <rik5>
Group administrator
Sun 29 Apr 2018 01:34:17 AM UTC, comment #3: 

Actually, I think the issue isn't whether ishold() should open a new figure, but instead whether ishold() should be treated as a global setting or a figure-specific property.  In patch (and about twenty five other locations) ishold() is used without an input argument:


  ## FIXME: This is a hack to get 'layer' command to work for 2D patches
  ##        Alternative is much more complicated surgery in graphics.cc.
  ##        of get_children_limits() for 'z' axis and 'patch' object type.
  if (! ishold ())
    if (isempty (get (htmp, "zdata")))
      set (hax, "zlim", [-1 1]);
    endif
  endif


That means ishold() is referring to the presumably some global setting because there is nothing indicating what ishold() should refer to.  In actuality, ishold() refers to current axes; so technically ishold() on its own in this context is wrong unless the current axes is the figure axes in question.

To see that "ishold()" refers to current figure and is a figure/axes property, note the following:


>> figure(1)
>> ishold()
ans = 0
>> figure(2)
>> hold on
>> ishold()
ans = 1
>> figure(1)
>> ishold()
ans = 0
>> figure(3)
>> ishold()
ans = 0
>> figure(2)
>> ishold()
ans = 1


So, I believe in most cases we want to use "ishold (hax)" in these plotting script files.  Attached is a patch which makes that change and seems to fix the problem.  Think it over; seems right to me.

There is one exception in which an "ishold ()" is used by


@linux ~/octave/octave/octave $ grep "ishold ()" */*/* -s
scripts/sparse/treeplot.m:  hold_is_on = ishold ();


but that treeplot routine does not accept an axes handle so can only be applied to the current axes.

In the process of making these changes, I also notice some inconsistencies (and then bugs) in the way that figure title is generated amongst qt, gnuplot and FLTK toolkits.  Qt toolkit was generating "GNU Octave" when neither "name" nor "numbertitle" were set.  The other two weren't doing that, so I made them do that as well.  (That means a name of " " must be used to make the title bar blank.)  Then I notice that set(wb, "numbertitle", true) was making a bogus number appear.  So I fixed these items in the patch as well.

@Rik: Maybe the preference is that Qt toolkit should be more like FLTK and not show anytihng in the title bar if there is no 'name' or 'numbertitle' properties set.  If so, I can change that easily.

(file #44057)

Dan Sebald <sebald>
Sat 28 Apr 2018 06:23:34 PM UTC, comment #2: 

The root cause is an issue with ishold().  The question is whether


close all
ishold ()


should open a new figure if none exists.  I'll ask someone to test on the Maintainer's list.

Rik <rik5>
Group administrator
Sat 28 Apr 2018 05:24:14 PM UTC, comment #1: 

Confirmed.  The problem is originating in the patch function, but it may be deeper than that.

Rik <rik5>
Group administrator
Sat 28 Apr 2018 11:19:53 AM UTC, original submission:  


For example:


close all
wb = waitbar(0, 'waitbar test');


Avinoam Kalma <avinoam>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by sebald (Updated the item)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by avinoam (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-04-30 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2018-04-29 sebald Attached File- Added octave-ishold_axes_dependent-djs2018apr28.patch, #44057
    2018-04-28 rik5 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code