patchGNU Octave - Patches: patch #9014, Add "camlight" function

 
 

patch #9014: Add "camlight" function

Submitter:  Colin Macdonald <cbm>
Submitted:  Wed 01 Jun 2016 03:17:25 PM UTC
   
 
Category:  Core : new function Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 14 Aug 2016 03:00:59 PM UTC, comment #14: 

I added camlight to Octave here (http://hg.savannah.gnu.org/hgweb/octave/rev/57fded74ee22).  Since _rotate_around_axis_ was only used by camlight I made it a private function in the draw directory.

Rik <rik5>
Group administrator
Wed 20 Jul 2016 08:10:57 PM UTC, comment #13: 

Demo works fine in Windows.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 18 Jul 2016 02:26:21 PM UTC, comment #12: 

Just here to confirm that all points seem to be addressed. Needless to say that the tests and demos still run without any problems and the docu builds ok.

Let's hope that someone with rights to push the change has sufficient time for a final review and push.

Markus Mützel <mmuetzel>
Group administrator
Tue 12 Jul 2016 06:39:18 AM UTC, comment #11: 

Thank you Markus, very thorough.  I attached the latest which I think addresses those points.

For the tests, I just did "clf" at the start of some tests.

I remember when I started this I thought about "unwind_protect" not working over multiple test: but I don't recall if I actually tried it or not.  Personally if tests fail, I don't mind if they don't clean up after themselves.  (xtests being exceptions).


(file #37819)

Colin Macdonald <cbm>
Mon 11 Jul 2016 07:26:40 PM UTC, comment #10: 

Colin, I did not have time to test your function thoroughly. Maybe this should be done by someone that is more experienced and can push changes anyway.
I found only some very minor issues while reading the code and running the demos and tests (which btw. all pass for me on Ubuntu 16.04 and Windows 10).

During the tests you add up to 10 lights to the same axes. That might trigger unrelated OpenGL warnings (or other noise) on some systems. You might consider using "unwind_protect" constructs with "unwind_protect_cleanup" blocks to close the hidden figure you use in your tests. However, I do not know whether that can (or should) span several tests.
Or you could delete the lights when you no longer need them in your tests?

Errors should start with the function name + ": ".

You should add a few more words to the error messages (or the complete one) in the tests to make it clearer for which error you are testing in which test.

You should add "__rotate_around_axis__.m" to scripts/plot/util/module.mk.

You could add a test for the error "if first arg is a handle, it must be a light handle".

And finally a little bit of nitpicking: Comments that start at the beginning of a line should start with ##. I think this also holds for comments in demos and tests.

I am looking forward to seeing this in 4.2.0.

Markus Mützel <mmuetzel>
Group administrator
Mon 11 Jul 2016 06:13:10 AM UTC, comment #9: 

I looked at this again.

  • I think I've fixed Markus's comments (I worked on top of the  v2 patch).


  • I made a new _rotate_around_axis_ helper function (its split out from scripts/util/rotate/m; I also need this in some upcoming cam* functions).


  • I wrote _plt_get_light_arg_ to detect light handles and trim varargin.  This is a private function in camlight.m for now (but could easily be split out if useful elsewhere).


  • Input handling is simpler, although I a special-case to deal with double-is-coincidently-a-handle problem discussed below.


  • I include an optional separate patch to make "scripts/util/rotate.m" use the new private _rotate_around_axis_ helper.  This avoids duplicated code.  The performance penalty is extra function calls, plus some redundent scalar computations of sind/cosd (which are now done inside a loop), neither of which seems like a big deal to me.  But anyway, its not required for camlight.


(file #37796, file #37797)

Colin Macdonald <cbm>
Fri 03 Jun 2016 09:53:16 AM UTC, comment #8: 

Please, ignore the part about handles always being positive doubles. That is for sure not the case.
Maybe it is save to take a negative integer. This at least fails:

figure (-3)

And I haven't yet come across any other handle (than figure) being of integer value...

Markus Mützel <mmuetzel>
Group administrator
Fri 03 Jun 2016 08:13:10 AM UTC, comment #7: 

I was a bit late in realizing that "ishghandle" silently ignores the second input argument in Octave. I am going to file a bug report about this. (Fixing this should not be too hard.)

Until this is fixed, you could additionally check for

strcmpi (get (varargin{1}, "type"), "light")


Instead of the loop to find a double that is not a valid handle, you could maybe use a negative double in the tests for errors. I believe handles always have positive values.

Markus Mützel <mmuetzel>
Group administrator
Fri 03 Jun 2016 08:00:24 AM UTC, comment #6: 

The function _plt_get_axis_arg_ looks for a handle to an axes object. (At least, I assumed judging by its name.)
Maybe something in the lines of the following would work:

if ( nargin > 0 && isscalar (varargin{1}) && ishghandle (varargin{1}, "light") )
    h = varargin{1};
    varargin(1) = [];
endif


The chance that a given value for AZ collides with a valid light handle should be rather low.
Matlab changed in recent version to use objects rather than double values to represent handles. That would eliminate the last chance for a collision. However, I am not aware of any plans to do the same step for Octave.

Maybe you could also check for "isscalar" when you check for "isnumeric". That could help to give more specific error messages.

Markus Mützel <mmuetzel>
Group administrator
Thu 02 Jun 2016 11:54:20 PM UTC, comment #5: 

Perhaps the odds of a collision are much smaller: I was mixing-up 'gcf' and 'gca'.  'gca' tends to be a double like "-14.662".

So maybe the odds of a collision are vanishingly small....

I looked at your "light" function: should I stop worrying about the above and use _plt_get_axis_arg_ as well?


>> hax = gca
hax = -8.6363
>> [H, VARARGIN, NARG] = __plt_get_axis_arg__ ("camlight", hax, 40)
H = -8.6363
VARARGIN =
{
  [1,1] =  40
}
NARG =  1


Colin Macdonald <cbm>
Thu 02 Jun 2016 09:48:40 PM UTC, comment #4: 

oops, s/"to change the view"/"to move the camera"

Colin Macdonald <cbm>
Thu 02 Jun 2016 09:47:07 PM UTC, comment #3: 

Great, thanks for the review!

Re:


hl = light;
camlight (hl, 45);


Thank you, I wondered what they did.  However, I think that behaviour is a bug:

1. suppose I have a figure(1).
2. then ishandle(1) returns true (at least on my computer.
3. now suppose I try to camlight(1, 30) to change the view.
4. Boom! error, but camlight(2, 30) is just fine.

Indeed, avoiding this is one of my test cases.

re: camup, campos, camtarget: oops, I was doing those next.  Maybe this patch should not mention those at all.

Colin Macdonald <cbm>
Thu 02 Jun 2016 07:21:41 PM UTC, comment #2: 

Thank you Colin for your patch.
However, I had some problems applying it locally. I think it is generally preferred if a patch consists of only one changeset.

I run some test with your "camlight" and it looks very complete. I found only one minor issue:
The following throws an error in Matlab but interprets the handle as the azimuth in Octave:

hl = light;
camlight (hl, 45);


I guess you could first detect whether the first argument is a valid light handle. After that step, you only have to deal with three arguments which might be a little bit easier to implement.

I also tried to add the docstring to the manual which failed with the following errors:

  MAKEINFO ../doc/interpreter/octave.info
doc/interpreter/plot.texi:3339: @pxref reference to nonexistent node `camup'
doc/interpreter/plot.texi:3340: @pxref reference to nonexistent node `campos'
doc/interpreter/plot.texi:3342: @pxref reference to nonexistent node `camtarget'
doc/interpreter/plot.texi:3365: @ref reference to nonexistent node `XREFmaterial' (possibly involving @seealso)


Attached please find your patch condensed to one changeset with some minor changes to the formatting of the docstring to make the manual compile.
I changed the user to your credits. I hope this is OK.

(file #37362)

Markus Mützel <mmuetzel>
Group administrator
Thu 02 Jun 2016 05:56:48 PM UTC, comment #1: 

Ok, I've attached camlight_v1.patch.  Please review.

1. The commits look in the wrong (non-chronological) order within the patch: I generated it with:

hg export -r 21813 21814 21815 21816 > camlight_v1.patch

Let me know if I've done something wrong.  I probably want something like "hg export default..cbm_camlight" where cbm_camlight is my bookmark...

2. I could squash if you want, but its a reasonable history of changes.

3. Dealing with the various possible inputs is... non-trivial.



(file #37360)

Colin Macdonald <cbm>
Wed 01 Jun 2016 03:17:25 PM UTC, original submission:  

Here's a work-in-progress camlight.m

I thought this might be useful, in cases others are playing with the new light.m (even if its not polished yet and not in the form of patches).

Patch to follow, but feedback of course welcome.

Colin Macdonald <cbm>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #37819:  camlight_v5.patch added by cbm (13KiB - text/x-patch)
file #37798:  camlight_v4.patch added by cbm (12KiB - text/x-patch - (very minor changes))
file #37796:  camlight_v3.patch added by cbm (12KiB - text/x-patch)
file #37797:  rotate_use_splitoff_v1.patch added by cbm (2KiB - text/x-patch)
file #37362:  camlight_v2.patch added by mmuetzel (11KiB - text/x-diff)
file #37360:  camlight_v1.patch added by cbm (27KiB - text/x-patch)
file #37333:  camlight.m added by cbm (5KiB - application/vnd.wolfram.mathematica.package - WIP (file itself, not a patch))

 

Depends on the following items: None found

Digest:
   patch dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by cbm (Submitted the item)
  • -email is unavailable- added by cbm
  •  

    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 logged-in users can vote.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-14 rik5 StatusNone Done
        Open/ClosedOpen Closed
    2016-07-12 siko1056 Dependencies- patch #9046 is dependent
    2016-07-12 cbm Attached File- Added camlight_v5.patch, #37819
    2016-07-11 cbm Attached File- Added camlight_v4.patch, #37798
    2016-07-11 cbm Attached File- Added camlight_v3.patch, #37796
        Attached File- Added rotate_use_splitoff_v1.patch, #37797
    2016-07-07 lachlan CategoryNone Core : new function
    2016-06-02 mmuetzel Attached File- Added camlight_v2.patch, #37362
    2016-06-02 cbm Attached File- Added camlight_v1.patch, #37360
    2016-06-01 cbm Attached File- Added camlight.m, #37333
        Carbon-Copy- Added mmuetzel

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code