bugGNU Octave - Bugs: bug #47791, "surface" and...

 
 

bug #47791: "surface" and "patch" do not support "FaceNormals" and "*NormalsMode"

Submitter:  Markus Mützel <mmuetzel>
Submitted:  Wed 27 Apr 2016 08:33:17 AM UTC
   
 
Category:  Plotting Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  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

Mon 06 Jun 2016 05:58:13 PM UTC, comment #10: 
Pantxo Diribarne <pantxo>
Group Member
Fri 03 Jun 2016 10:03:22 AM UTC, comment #9: 

The latest patch is actually what I wanted to propose in comment #6. I'll test it and eventually push it in the beginning of next week. Thanks Markus for following up.

Pantxo Diribarne <pantxo>
Group Member
Fri 03 Jun 2016 09:36:27 AM UTC, comment #8: 

Yet another patch where the change of warning states is moved inside base_graphics_object::reset_default_properties.

(file #37371)

Markus Mützel <mmuetzel>
Group administrator
Fri 03 Jun 2016 09:16:38 AM UTC, comment #7: 

I tried a similar approach by changing the warning states in "reset" and it works, too (see attached patch). However, I remembered some discussion about scopes ("local", "global", ...) of warning and error states. I did not find anything related to it in error.h and did not want to cause yet another side effect by removing those scopes while changing the warning state.
So I changed over to taking the same approach as taken for some of the properties of the figure go.

But maybe you are right and there might be some properties in the future that would be deprecated but should be re-set anyway...

Putting this inside the loop should probably not be an issue. And that would prevent that effect from re-surfacing wherever reset_default_properties might be called.
I'll post a patch taking your approach from comment #6 as well and someone that has better insight than me can decide which should be taken...

(file #37370)

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

If we want to extend the use of this warning (e.g. in root figure properties) a simpler approach could be to disable the "deprecated-property" warning locally in base_properties::reset_default_properties. Did you try this?

Pantxo Diribarne <pantxo>
Group Member
Thu 02 Jun 2016 06:17:32 PM UTC, comment #5: 

That patch had an unexpected side effect: When "reset" is called on a patch or surface, the warning that "normalmode" was deprecated is issued. That warning could possibly confuse unaware users. (Or it might throw an error for "reset" if the warning state of "Octave:deprecated-property" was set to "error".)

Steps to reproduce:

hp = patch;
reset (hp)

hs = surface;
reset (hs)


Attached please find a patch that skips "normalmode" for patch and surface objects when reset.
As "normalmode" is always shadowed from "vertexnormalmode", there is no need to reset it anyway.

(file #37361)

Markus Mützel <mmuetzel>
Group administrator
Thu 28 Apr 2016 09:18:38 PM UTC, comment #4: 

Thanks Markus. I pushed your patch here:
http://hg.savannah.gnu.org/hgweb/octave/rev/a0ebc922fd52

Closing report.

Pantxo Diribarne <pantxo>
Group Member
Thu 28 Apr 2016 11:15:54 AM UTC, comment #3: 

Thank you, Pantxo, for reviewing the patch and for your feedback.

Attached please find an updated patch which adds a description for Octave:deprecated-property to warning_ids.m.
There was no item for Octave:deprecated-function either so I added one for that warning id as well. I hope this is OK even though this is a little off scope of this bug.

(file #37010)

Markus Mützel <mmuetzel>
Group administrator
Thu 28 Apr 2016 06:08:33 AM UTC, comment #2: 

Confirmed. The patch looks good and I think the addition of this new warning is a good thing as well.
Could you add a description in warning_ids.m so that it is mentioned in the manual?

Pantxo Diribarne <pantxo>
Group Member
Wed 27 Apr 2016 12:00:11 PM UTC, comment #1: 

That fix was faster than expected.


>> hs = surface;
>> get (hs, "VertexNormals")
ans =

ans(:,:,1) =

  -0.00000   0.25000   0.50000
  -0.25000  -0.00000   0.25000
  -0.50000  -0.25000  -0.00000

ans(:,:,2) =

  -0.00000  -0.25000  -0.50000
   0.25000  -0.00000  -0.25000
   0.50000   0.25000  -0.00000

ans(:,:,3) =

   1   1   1
   1   1   1
   1   1   1

>> get (hs, "FaceNormals")
ans = [](0x0)
>> get (hs, "FaceNormalsMode")
ans = auto
>> get (hs, "VertexNormalsMode")
ans = auto
>> get (hs, "NormalMode")
warning: surface: Property 'normalmode' is deprecated and will be removed from a future version of Octave. Use 'vertexnormalsmode' instead.
ans = auto
>> set (hs, "NormalMode", "manual")
warning: surface: Property 'normalmode' is deprecated and will be removed from a future version of Octave. Use 'vertexnormalsmode' instead.
>> get (hs, "VertexNormalsMode")
ans = manual
>> hp = patch;
>> get (hp, "VertexNormals")
ans = [](0x0)
>> get (hp, "FaceNormals")
ans = [](0x0)
>> get (hp, "FaceNormalsMode")
ans = auto
>> get (hp, "VertexNormalsMode")
ans = auto
>> set (hp, "NormalMode", "manual")
warning: patch: Property 'normalmode' is deprecated and will be removed from a future version of Octave. Use 'vertexnormalsmode' instead.
>> get (hp, "NormalMode")
warning: patch: Property 'normalmode' is deprecated and will be removed from a future version of Octave. Use 'vertexnormalsmode' instead.
ans = manual
>> get (hp, "VertexNormalsMode")
ans = manual


The id of the warnings is "Octave:deprecated-property". I did not find any other references to it in the source code but felt that "Octave:deprecated-function" did not quite fit here...

Is this the right approach to face out graphics properties?

(file #37003)

Markus Mützel <mmuetzel>
Group administrator
Wed 27 Apr 2016 08:33:17 AM UTC, original submission:  

In recent versions of Matlab, "FaceNormals" was added to surface and patch objects. Additionally, "NormalMode" was renamed to "VertexNormalsMode" and "FaceNormalsMode" was added [1].

In Octave, scripts or functions that try to get or set one of the new properties terminate with an error:

>> hs = surface;
>> get (hs, "VertexNormals")
ans =
ans(:,:,1) =
  -0.00000   0.25000   0.50000
  -0.25000  -0.00000   0.25000
  -0.50000  -0.25000  -0.00000
ans(:,:,2) =
  -0.00000  -0.25000  -0.50000
   0.25000  -0.00000  -0.25000
   0.50000   0.25000  -0.00000
ans(:,:,3) =
   1   1   1
   1   1   1
   1   1   1
>> get (hs, "FaceNormals")
error: get: unknown surface property FaceNormals
>> get (hs, "FaceNormalsMode")
error: get: unknown surface property FaceNormalsMode
>> get (hs, "VertexNormalsMode")
error: get: unknown surface property VertexNormalsMode

>> hp = patch;
>> get (hp, "VertexNormals")
ans = [](0x0)
>> get (hp, "FaceNormals")
error: get: unknown patch property FaceNormals
>> get (hp, "FaceNormalsMode")
error: get: unknown patch property FaceNormalsMode
>> get (hp, "VertexNormalsMode")
error: get: unknown patch property VertexNormalsMode


A minimal patch should at least allow to get and set these properties without support for automatic calculation.

I will try and submit a minimal patch when time permits.

[1] http://de.mathworks.com/help/matlab/examples/changing-surface-properties.html#zmw57dd0e5545

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 #37361:  normalmode_reset.patch added by mmuetzel (2KiB - text/x-diff)
file #37010:  normals_v2.patch added by mmuetzel (9KiB - text/x-diff)
file #37003:  normals_v1.patch added by mmuetzel (7KiB - 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 pantxo (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 group members can vote.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-06-03 mmuetzel Attached File- Added normalmode_reset_v3.patch, #37371
    2016-06-03 mmuetzel Attached File- Added normalmode_reset_v2.patch, #37370
    2016-06-02 mmuetzel Attached File- Added normalmode_reset.patch, #37361
    2016-04-28 pantxo StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2016-04-28 mmuetzel Attached File- Added normals_v2.patch, #37010
    2016-04-28 pantxo StatusNone Patch Submitted
    2016-04-27 mmuetzel Attached File- Added normals_v1.patch, #37003

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code