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

 
 

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

Submitted by:  Markus Mützel <mmuetzel>
Submitted on:  Wed 27 Apr 2016 08:33:17 AM UTC  
 
Category: PlottingSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Matlab Compatibility
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating System: Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 06 Jun 2016 05:58:13 PM UTC, comment #10:

I pushed the patch as is:

http://hg.savannah.gnu.org/hgweb/octave/rev/d9fa4558d2ed

Thanks

Pantxo Diribarne <pantxo>
Project 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>
Project 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>
Project Member
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>
Project Member
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>
Project 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:

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>
Project Member
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>
Project 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>
Project Member
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>
Project Member
Wed 27 Apr 2016 12:00:11 PM UTC, comment #1:

That fix was faster than expected.

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>
Project Member
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:

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>
Project Member

 

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

Attach File(s):
   
   
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
  • -unavailable- added by pantxo (Posted a comment)
  • -unavailable- added by mmuetzel (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 03 Jun 2016 09:36:27 AM UTCmmuetzelAttached File-=>Added normalmode_reset_v3.patch, #37371
    Fri 03 Jun 2016 09:16:38 AM UTCmmuetzelAttached File-=>Added normalmode_reset_v2.patch, #37370
    Thu 02 Jun 2016 06:17:32 PM UTCmmuetzelAttached File-=>Added normalmode_reset.patch, #37361
    Thu 28 Apr 2016 09:18:38 PM UTCpantxoStatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Thu 28 Apr 2016 11:15:54 AM UTCmmuetzelAttached File-=>Added normals_v2.patch, #37010
    Thu 28 Apr 2016 06:08:33 AM UTCpantxoStatusNone=>Patch Submitted
    Wed 27 Apr 2016 12:00:11 PM UTCmmuetzelAttached File-=>Added normals_v1.patch, #37003

    Back to the top


    Powered by Savane 3.1-cleanup1