bugGNU Octave - Bugs: bug #35511, reset (h) function doesn't work at...

 
 

bug #35511: reset (h) function doesn't work at all

Submitter:  Ben Abbott <bpabbott>
Submitted:  Sun 12 Feb 2012 01:10:42 AM UTC
   
 
Category:  Plotting Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Ben Abbott Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 09 Sep 2014 10:38:38 AM UTC, comment #44: 

I have played a bit with the reset function and didn't see any particular issue. I'm closing this bug report.

Pantxo Diribarne <pantxo>
Group Member
Tue 29 Jul 2014 12:16:15 AM UTC, comment #43: 

I had one crash while testing file #31592, but am not able to duplicate it. So ... it looks ok to me.

Ben Abbott <bpabbott>
Group Member
Mon 28 Jul 2014 02:57:55 PM UTC, comment #42: 

Hi,

I've pushed the changeset here http://hg.savannah.gnu.org/hgweb/octave/rev/a30e1d20fd3c

The sleep for figure reset is only called once each time a figure is reset which means a very small overhead.

I changed the status to "ready for test". As I'll be in vacations next 3 weeks, I will not be very responsive, but I don't think this changeset is very risky.

Pantxo Diribarne <pantxo>
Group Member
Sun 20 Jul 2014 09:45:35 PM UTC, comment #41: 

I think this is acceptable.  My hunch is that reset() isn't used very often so a 1ms delay is okay.  A quick check would be to put in a statement such as


std::cerr << "Using reset() delay" << std::newline;


near the call to Fsleep and then run 'make check' and see how many calls you get.  This would help determine whether the code is frequently used or not.


Rik <rik5>
Group administrator
Sun 20 Jul 2014 08:51:21 PM UTC, comment #40: 

As I said in comment #30, the only way I found to safely reset figure objects with qt toolkit is to add a small delay in figure::reset_default_properties. If it's ok to add the following FIXME I'll push the attached updated cset:


+  // FIXME: the following short sleep is needed in order
+  //        to avoid a crash when using qt toolkit
+  Fsleep (octave_value (0.001));


(file #31751)

Pantxo Diribarne <pantxo>
Group Member
Thu 10 Jul 2014 03:04:55 PM UTC, comment #39: 

I think it's up to you how to proceed.  We have two problem behaviors and we can either fix them one at a time or do them both together.  Offhand, it seems okay to commit the fix for this bug since we know bug #38449 is going to require some thought and then probably a fairly large bit of code rework.

Rik <rik5>
Group administrator
Thu 10 Jul 2014 08:31:12 AM UTC, comment #38: 

Hi,

As I was preparing to push the patch for this bug, I figured out that solving bug #38449 would certainly require refactoring property_list class (at least pval_map_type container). Depending on the choice for property_list container, the patch for reset will have to be also refactored.

Does someone have an opinion? Should I make this bug dependent on bug #38449 or push the patch and refactor the code later?

Pantxo Diribarne <pantxo>
Group Member
Sun 22 Jun 2014 03:17:14 AM UTC, comment #37: 

On Mac OS X the OpenGL toolkits no longer function (for the default branch).  As a result, I'm weary of pushing any graphic related change sets (if there is a problem I won't be able to debug).

Rik, do you have time to push patches for bugs #35511, #42123, and #42159?

In case it is useful, I'm also attaching a set of tests I collected from all the bug reports.

(file #31592)

Ben Abbott <bpabbott>
Group Member
Sun 22 Jun 2014 03:11:09 AM UTC, comment #36: 

I'm attaching Pantxo's patch with an improved doc-string.

(file #31591)

Ben Abbott <bpabbott>
Group Member
Sat 21 Jun 2014 03:31:12 PM UTC, comment #35: 

I forgot to say I had rewritten the docstring for reset function. Don't hesitate to rephrase my beautiful french-glish :-)

Pantxo Diribarne <pantxo>
Group Member
Sat 21 Jun 2014 03:17:22 PM UTC, comment #34: 

I think the following script shows a proper bug:


hi = image ();
cdat = get (hi, "cdata");
set (hi, "cdata", get (0, "defaultimagecdata"))
assert (cdat, get (hi, "cdata"))


What is in the default image, even its size, doesn't matter, I agree. However, for consistency with all other graphics primitive, the call to image without argument should display the default image.

Pantxo Diribarne <pantxo>
Group Member
Sat 21 Jun 2014 02:49:40 PM UTC, comment #33: 

The default.img file is something of an Easter egg.  See this amusing little blog entry about the development of the default image in Matlab (http://blogs.mathworks.com/steve/2006/10/17/the-story-behind-the-matlab-default-image/).

Octave's default.img is a grayscale of a baby--which might be jwe--I'm not quite sure.

Is it really a problem if the size of the default image is different from Matlab?  We don't, after all, maintain Matlab compatibility in our choice of the image.

Rik <rik5>
Group administrator
Sat 21 Jun 2014 01:44:01 PM UTC, comment #32: 

You may try in the following order:

  • bug #42159 . Test: try the example provided to show the bug, try "demo patch" and any complicated plot function that involve patch objects.
  • bug #42121 . Test: there is a %!test included in the patch  which doesn't rely on "default.img" so the bug we just discussed doesn't interfere,
  • this bug. Test: I added tests related to "reset" function. I could verify "make check" passes for me.


About "default.img", I don't know where it comes from but it is stored in scripts/image and is used for image functions demos.
I attached a simple fix.



(file #31588)

Pantxo Diribarne <pantxo>
Group Member
Sat 21 Jun 2014 01:17:28 PM UTC, comment #31: 

Pantxo,

I took at look at why the size of the default image is wrong.  The default image ("default.img") isn't 64x64.  I'm not sure how to fix that (how was the image created?).

In any event, perhaps some of the changes are ready to be committed?  What is the preferred order to commit the change sets?

(I haven't tested the latest one, but hope to do that today)

Ben Abbott <bpabbott>
Group Member
Sat 21 Jun 2014 11:53:56 AM UTC, comment #30: 

I attached a slightly modified patch that includes %!tests for reset function. The test related to patch objects needs bug #42159 fixed.

As you'll see, I have commented out one of the tests that passes with both gnuplot and fltk graphics toolkits, which both operate sequentially with octave interpreter, but crashes octave with qt. There seems to be a race condition: if I slow down the reset process by inserting pauses in "xreset_defaults" there is no crash anymore.

@Ben: about the size of the image in bug #42121, this is due to octave not using the default image cdata when one calls "image ()" without arguments. Should I also correct this behavior or file a new bug report?

(file #31587)

Pantxo Diribarne <pantxo>
Group Member
Tue 17 Jun 2014 12:48:53 AM UTC, comment #29: 

I confirmed the tests in the comments of bug #35511 all pass.

The example for bug #42159 did not result in an error or warning. Matlab does


Warning: Values in patch Faces must be in [1 : rows(Vertices)] - not rendering


For bug #42121 the cdata for im=image() has size 53x40. However, for Matlab the size is 64x64.


Ben Abbott <bpabbott>
Group Member
Mon 16 Jun 2014 10:28:19 PM UTC, comment #28: 

I have no time right now but I'll write a comprehensive set of tests to help you review.


Pantxo Diribarne <pantxo>
Group Member
Mon 16 Jun 2014 06:31:49 PM UTC, comment #27: 

I've been working on getting JIT working on Mac OS X.  I'll try a clean build and then try out the patches.

@Pantxo, can you advise me on what tests I should run?

Ben Abbott <bpabbott>
Group Member
Mon 16 Jun 2014 04:10:54 PM UTC, comment #26: 

@Pantxo: Sorry that no one has gotten to this.  I have a day job during the week, but will try to review the patches this coming weekend unless Ben gets to it.

Rik <rik5>
Group administrator
Sun 15 Jun 2014 03:04:16 PM UTC, comment #25: 

Hi,

I have proposed a patch for bug #42159 (patch #31555).
Could someone test it along with patch for bug #42121 (patch #31173) and the last updated patch for this bug (patch #31470)?
Thanks in advance.

Pantxo Diribarne <pantxo>
Group Member
Fri 30 May 2014 10:20:50 AM UTC, comment #24: 

I forgot the attachment.

(file #31470)

Pantxo Diribarne <pantxo>
Group Member
Fri 30 May 2014 09:49:32 AM UTC, comment #23: 

I attached an updated patch:

  • fixes the issue shown by Ben in comment #20,
  • in order to be consistent with bug #42285 comments, listeners are not deleted when an object is reset,
  • axes hidden text children are reset instead of just having their "string" emptied


The summary of remaining issues (I know of :-)) is quite the same as in comment #15 and #16. The only difference is that trying to reset a patch leads to an error: the listener is called before all default (coherent) data are set. You have to reset twice to obtain a proper default patch.

As for the listeners translation, I'll give it a try in the next few days/weeks.

Pantxo Diribarne <pantxo>
Group Member
Thu 29 May 2014 04:34:32 PM UTC, comment #22: 

I think the patch listeners should be transformed to C++.  patch is a very important primitive object and we really need it to render as quickly as possible.  I think moving everything to C++ and out of m-files would be a good thing.  I've already spent a lot of time re-coding the patch rendering in gl-render.cc on the development branch and it now produces really nice, Matlab compatible renderings.

So it seems that the patch for bug #42121 needs to be committed, then the patch for this bug report.

And at some point in the future we need to file a bug report for the issue brought up in bug #42285 which is that listeners are not removed by default anymore.  This requires that anyone who issues an addlistener call, also needs to have a function to delete the listeners when the original object is destroyed.

Rik <rik5>
Group administrator
Thu 29 May 2014 01:33:37 PM UTC, comment #21: 

Yes this is due to my patch (a "||" instead of a "&&" in axes::properties::set_defaults). I'll fix this and modify the patch to take into account the fact that listeners shouldn't be removed (see bug #42285).

Pantxo Diribarne <pantxo>
Group Member
Thu 29 May 2014 12:22:11 PM UTC, comment #20: 

oh-oh. I'm seeing strange behavior.  The commands below render the same result as the plot() command by itself.


subplot(3,1,1)
subplot(3,1,2)
subplot(3,1,3)
plot (rand (3))


I'm not certain this is due to Pantxo's patch, but thought I'd comment before my day starts and I get distracted by other problems.

Ben Abbott <bpabbott>
Group Member
Thu 29 May 2014 11:39:37 AM UTC, comment #19: 

My first reaction was; "Why use c++ updaters for patches but not for other objects?"

However, after thinking about that, it occurred to me that listeners are normally used to update things like colobar(), legend(), etc. These are collections of objects. And updaters are already used for "axes", "text", etc.

Thus, after my initial skepticism, I think this is the consistent thing to do.

I also agree this looks intimidating, and since my activity here isn't very regular, perhaps Rik can comment?

Ben Abbott <bpabbott>
Group Member
Thu 29 May 2014 10:14:01 AM UTC, comment #18: 

Hi,

I am not sure how to proceed with this task. Should I wait until someone gets a chance to review the last patch I posted here or am I supposed to go ahead and move patch listeners to c++ updaters.

I admit I would prefer the former as moving listeners to updaters is quite a big and intimidating task (and I wouldn't like to do it and hear that it was not a good idea afterwards :-).

Pantxo Diribarne <pantxo>
Group Member
Mon 21 Apr 2014 06:25:08 PM UTC, comment #17: 

I posted bug #42159

Pantxo Diribarne <pantxo>
Group Member
Mon 21 Apr 2014 06:04:22 PM UTC, comment #16: 

Oups, I forgot :

  • patch: resetting removes all listeners so that the resulting patch can't be updated anymore. Patch property updates should be implemented as c++ updaters not listeners. I will file a bug report about this.
Pantxo Diribarne <pantxo>
Group Member
Mon 21 Apr 2014 05:59:27 PM UTC, comment #15: 

I attached a patch for test and review.

Here is a summary of the remaining issues I can see:

  • image: as factory x/ydata are empty, trying to reset an image won't work. This will be fixed with bug #42121
  • root: reseting root properties removes all default properties set not only by the user but also by the graphics_toolkit. This may cause troubles with subsequent figures.



(file #31231)

Pantxo Diribarne <pantxo>
Group Member
Sat 19 Apr 2014 11:02:30 PM UTC, comment #14: 

Yes, the main remaining issues I can see is patch object properties updates and also surface "x/ydata" updates (see bug #42121).
I think I'll have time next week to file a bug report about _patch_ and also to post a patch proposal here for testing and review of reset feature.

Pantxo Diribarne <pantxo>
Group Member
Sat 19 Apr 2014 04:36:03 PM UTC, comment #13: 

Pantxo, the two bugs that were blocking this one (#41770, #41629) have been fixed.  Did you want to post a patch for this bug [reset (h) doesn't work]?  You mentioned possibly needing to re-write the way patch objects are handled so that there are no listeners in _patch_, but rather update methods in graphics.cc.  Is this still a blocker?  If so, we should open up a new bug report for that.

Rik <rik5>
Group administrator
Tue 25 Mar 2014 02:25:36 PM UTC, comment #12: 

I can take a look at bug #41770, but not until the weekend when I have some free time.

It seems logical to me to treat all graphics primitives the same way.  If this means moving defaults into graphics.in.h that would be fine.  There is already a BEGIN_PROPERTIES (patch) section in graphics.in.h so I suppose it would just mean extending it for any missing properties.

Rik <rik5>
Group administrator
Mon 24 Mar 2014 05:13:28 PM UTC, comment #11: 

Ben, addproperty doesn't seem to offer a way to restrict property access so I am not too concerned about that.

I am progressing on this bug. The approach I proposed in comment #3 works quite well for me but there are still some issues.

First, I would like to use factory defaults when they are defined. For this, it would help if someone could review the patch in bug #41770.

Second, "patch" updaters and default data are defined in _patch_.m. I think, as all other base graphics objects, patches should not rely on property listeners (but rather on update_xxx methods) and should have all their default properties defined in graphics.cc(or .in.h). Any comment?

Pantxo Diribarne <pantxo>
Group Member
Sun 16 Mar 2014 10:58:29 PM UTC, comment #10: 

Does addproperty() allow read-only properties to be created?  If so would adding a read-only property to a figure cause an error?

Ben Abbott <bpabbott>
Group Member
Sun 16 Mar 2014 10:26:59 PM UTC, comment #9: 

Hello Rik,
I proposed a patch to at least have a way to determine programmatically (by name) if a property is read-only (see bug #41629, first patch). Do you think it should be done differently? Are there limitations with this approach?

Pantxo Diribarne <pantxo>
Group Member
Sun 16 Mar 2014 07:35:08 PM UTC, comment #8: 

Pantxo, you're approach seems about right.  I agree that it would be nice if there were a way to quickly remove the Read-Only properties rather than looking for the Read-Only properties by name.  But I don't have any handy trick to do that.

Rik <rik5>
Group administrator
Thu 20 Feb 2014 09:02:08 AM UTC, comment #7: 

Thank you Ben, that is what I expected from the doc.
Now I'll wait for someone to validate (or not :-) my approach as described in comment #3 (and the patch I attached to show my intents).

As it will make a big changeset, I'd be happy to have a go-ahead.

Pantxo Diribarne <pantxo>
Group Member
Wed 19 Feb 2014 03:50:47 PM UTC, comment #6: 

Hi Pantxo,

Sorry for the delay, my Mac is dying a slow/intermittent death.  If only it would give up its last breath I can get it replaced.  In any event, it is working nicely for the moment.  The window position after the second reset (). The result of your script is below.


ans =

     0

ans =

     1


Ben Abbott <bpabbott>
Group Member
Wed 19 Feb 2014 09:36:54 AM UTC, comment #5: 

Hi Ben,

At the first reset did the figure jump back to its default position?


figure (1);
set (1, 'position', [100 100 560 420])
set (1, 'color', [.5 .5 .5])
plot (5:10);
reset (1)
all (get (1, 'position') == get (0, 'defaultfigureposition'))  %% false?
all (get (1, 'color') == get (0, 'defaultfigurecolor'))        %% true?


I put the expected result as comments. If I'm correct, now I need to know if the work flow I proposed is acceptable as a first approach and if I should proceed in that direction.

Pantxo Diribarne <pantxo>
Group Member
Wed 19 Feb 2014 01:31:31 AM UTC, comment #4: 

I ran the script below with Matlab R2013b


figure (1);
set (1, 'position', [100 100 560 420])
set (1, 'color', [.5 .5 .5])
plot (5:10);
reset (1) % reset figure's color but not position as per ML doc
title ('First "reset (1)"')
pause
set (0, 'defaultfigurecolor', [.5 .5 .5])
reset (1) % reset figure's color to parent's default
title ('Second "reset (1)"')


The resulting figures are attached.


Ben Abbott <bpabbott>
Group Member
Tue 18 Feb 2014 01:42:40 PM UTC, comment #3: 

Dan, the default_properties attribute you are talking about, which only parent objects have (uitoolbar, root, figure, axes), is meant to store "local" defaults, i.e when you do:


figure (1);
set (1, "defaultaxeslinewidth", 1)
get (1, "default")


The default property is stored in figure(1)'s default_properties and will apply only to its axes children. The mechanism works well and is based on the use of override_defaults function.

The static function (reset_default_properties) that is called by reset_default_properties methods is not doing anything useful and should be removed.

I have attached a proof of concept of what I think should be done by reset_default_properties methods. Basically the work flow is:
 - local defaults must be emptied if they exist (parent objects only)
 - reset all properties but read only and those that should not be reset according to matlab doc. This may be done in the method itself or via properties::set_defaults when it's defined
 - override with parent defaults (except for root)

With this patch the following works as expected:


figure (1);
set (1, "position", [100 100 560 420])
set (1, "color", [.5 .5 .5])
plot (5:10);
reset (1) ## reset figure's color but not position as per ML doc
set (0, "defaultfigurecolor", [.5 .5 .5])
reset (1) ## reset figure's color to parent's default


Can someone confirm this is the good interpretation/approach ?


(file #30595)

Pantxo Diribarne <pantxo>
Group Member
Mon 10 Sep 2012 07:02:29 PM UTC, comment #2: 

I believe the problem is on the "set" side of things, not the "reset" side.  I've followed the code that does the reset and it appears to be run, but there isn't anything in the list of defaults.  I've looked at the property_list "set" for the defaults, and that doesn't appear to happen.  So whatever mechanism existed before for setting the defaults isn't happening.

Dan Sebald <sebald>
Tue 21 Feb 2012 10:15:01 PM UTC, comment #1: 

Re-titling the report as the function behavior is much worse than expected.  The embedded docstring does not match what Matlab says the function should do.  Nor does the function appear to work for any sample property.


hax = gca;
set (hax, 'linewidth', 2.3);
get (hax, 'linewidth')
 ans =  2.3000
reset (hax);
get (hax, 'linewidth')
 ans =  2.3000


The linewidth should have been reset to 0.5.

Rik <rik5>
Group administrator
Sun 12 Feb 2012 01:10:42 AM UTC, original submission:  

The commands below should result in a plot whose color map is jet()


peaks
colormap (copper)
reset (gcf)



Ben Abbott <bpabbott>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #31751:  reset_graphics_bug35511_v5.patch added by pantxo (15KiB - application/x-download)
file #31592:  reset_tests.m added by bpabbott (2KiB - application/octet-stream - tests for bugs #35511, #42121, and #42159)
file #31591:  reset_graphics_bug35511_v4.patch added by bpabbott (15KiB - application/octet-stream - improved doc-string)
file #31588:  image_defaultcdata.patch added by pantxo (742B - application/x-download)
file #31587:  reset_graphics_bug35511_v4.patch added by pantxo (15KiB - application/x-download)
file #31470:  reset_graphics_bug35511_v2.patch added by pantxo (10KiB - application/x-download)
file #30604:  first reset.png added by bpabbott (48KiB - image/png)
file #30605:  second reset.png added by bpabbott (48KiB - image/png)
file #30595:  reset_poc_bug35511.patch added by pantxo (4KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2014-09-09 pantxo StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2014-07-28 pantxo StatusPatch Submitted Ready For Test
    2014-07-20 bpabbott Carbon-Copy- Added -email is unavailable-
    2014-07-20 pantxo Attached File- Added reset_graphics_bug35511_v5.patch, #31751
    2014-06-22 bpabbott Attached File- Added reset_tests.m, #31592
    2014-06-22 bpabbott Attached File- Added reset_graphics_bug35511_v4.patch, #31591
    2014-06-21 pantxo Attached File- Added image_defaultcdata.patch, #31588
    2014-06-21 bpabbott StatusNone Patch Submitted
    2014-06-21 pantxo Attached File- Added reset_graphics_bug35511_v4.patch, #31587
    2014-05-30 pantxo Attached File- Added reset_graphics_bug35511_v2.patch, #31470
    2014-04-21 pantxo Attached File- Added reset_graphics_bug35511.patch, #31231
    2014-02-19 bpabbott Attached File- Added first reset.png, #30604
        Attached File- Added second reset.png, #30605
    2014-02-18 pantxo Attached File- Added reset_poc_bug35511.patch, #30595
    2012-02-21 rik5 Summaryreset(gcf) doesn\'t reset colormap to default reset (h) function doesn't work at all

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code