patchGNU Octave - Patches: patch #9040, Add function "lighting"

 
 

patch #9040: Add function "lighting"

Submitter:  Markus Mützel <mmuetzel>
Submitted:  Tue 05 Jul 2016 07:15:13 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

Mon 18 Jul 2016 07:22:28 PM UTC, comment #8: 

Thank you, Rik, for pushing, for including the necessary changes and for the feedback.

I changed my local hg setup a few days ago to replace the ü with ue. So newer patches should hopefully apply with less trouble.

Markus Mützel <mmuetzel>
Group administrator
Mon 18 Jul 2016 06:24:14 PM UTC, comment #7: 

@Markus: I pushed your patch here (http://hg.savannah.gnu.org/hgweb/octave/rev/18eae417ffec).

I had to remove the u umlaut in your name and replace with 'ue' because I was getting some crazy encoding error that prevented Mercurial from applying the patch


applying lighting
transaction abort!
rollback completed
cleaning up working directory...done
abort: decoding near 'Markus M�tzel <mar': 'utf8' codec can't decode byte 0xfc in position 8: invalid start byte!


Basically, I think you understand the Octave coding conventions, but here is what I changed:

1) "!CONDITION" => "! CONDITION".  In English, one can read this as two words "NOT" "CONDITION" so it makes it easier to understand.

2) "function_name (...)".  I found one instance of "cellfun(...)" which I changed to "cellfun (...)".

3) strcmp instead of strcmpi for all comparisons to graphics properties retrieved from a graphic object.  Internally, Octave normalizes all graphics properties to lower case so it is shorter code, and slightly less work for the CPU work, touse strcmp.  For properties that a user might enter, such as inputs to a function, there is no guarantee and there strcmpi is useful.

Finally, I added a BIST test for


figure
lighting flat


This failed in the original code because there were no mesh or patch objects, but strcmp ([], 'none') returns 0, not an empty matrix.  This then caused trouble later on.  I fixed the code by adding an early return if there are no objects to process.


if (isempty (hlist))
   return;
endif



Rik <rik5>
Group administrator
Mon 11 Jul 2016 05:03:51 PM UTC, comment #6: 

Oops. Thank you, Colin, for spotting that one. I must have missed it after copying over most part of "shading.m".

Attached is a changeset with a fix for this.

(file #37808)

Markus Mützel <mmuetzel>
Group administrator
Mon 11 Jul 2016 06:40:50 AM UTC, comment #5: 

texinfo function name is "shading" but should be lighting.

(I haven't tested, just read through the doc)

Colin Macdonald <cbm>
Thu 07 Jul 2016 02:40:14 AM UTC, comment #4: 

Oops.  I was misreading as "nargin != 0 || narg*in* != 1"

Lachlan Andrew <lachlan>
Wed 06 Jul 2016 05:32:39 PM UTC, comment #3: 

Lachlan, thank you for taking a look at that patch.

"lighting" must be called with one input variable (apart from the axes handle already stripped by _plt_get_axis_arg_) and no output variable. Everything else should get caught with the if statement you mentioned.

It seems to work as expected for me...

Markus Mützel <mmuetzel>
Group administrator
Wed 06 Jul 2016 11:03:21 AM UTC, comment #2: 

Thanks for the patch, Markus.

Have you tried it?  I think "if ( nargin != 1 || nargout != 0 )" should always be true, and so the patch should always exit through print_usage.

Lachlan Andrew <lachlan>
Tue 05 Jul 2016 07:17:02 PM UTC, comment #1: 

Attached please find the patch.

(file #37724)

Markus Mützel <mmuetzel>
Group administrator
Tue 05 Jul 2016 07:15:13 PM UTC, original submission:  

The function "lighting" can be used to change the "facelighting" and "edgelighting" properties of all patch and surface objects in one axes.

Most of the function is very similar to "shading" including the changes from bug #48392.

I will attach the patch as soon as I have the patch number.

Markus Mützel <mmuetzel>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #37808:  lighting_v2.patch added by mmuetzel (8KiB - text/x-diff)
file #37724:  lighting.patch added by mmuetzel (8KiB - text/x-diff)

 

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 cbm (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by mmuetzel (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 logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-18 rik5 StatusNone Done
        Open/ClosedOpen Closed
    2016-07-11 mmuetzel Attached File- Added lighting_v2.patch, #37808
    2016-07-07 lachlan CategoryNone Core : new function
    2016-07-05 mmuetzel Attached File- Added lighting.patch, #37724

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code