bugGNU Octave - Bugs: bug #54922, demo surfl 1 fails

 
 

bug #54922: demo surfl 1 fails

Submitter:  Rik <rik5>
Submitted:  Tue 30 Oct 2018 06:59:10 PM UTC
   
 
Category:  Plotting with OpenGL Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
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

Sun 04 Nov 2018 02:46:21 PM UTC, comment #9: 

@Markus: Thanks for the patch.  I reviewed and pushed it here (https://hg.savannah.gnu.org/hgweb/octave/rev/50583f514ae4).

I simplified the _update_normals_ function by removing some of the input validation.  There wasn't a need to use ishghandle, for example, because the next line was


  graphics_object go = gh_manager::get_object (val);


If val was not a proper handle then this will not return a valid graphics_object and the subsequent tests for go.isa ("XXXX") will fail and the default else clause


    error ("__update_normals__: "
           "H must be a handle to a valid surface or patch object.");


will eventually be taken.

I also aligned tests in if () conditionals for better readability.  Instead of


  if (force || ((facelighting_is ("flat") || edgelighting_is ("flat")) &&
      get_do_lighting ()))


I used


  if (force || ((facelighting_is ("flat") || edgelighting_is ("flat"))
                && get_do_lighting ()))


Marking as fixed and closing report.

Rik <rik5>
Group administrator
Sat 03 Nov 2018 02:54:37 PM UTC, comment #8: 

I opted for leaving the delayed calculation as it is but adding a function to manually trigger the calculation of normals for surfaces or patches without the pre-conditions.

Please, see the attached patch.

(file #45354)

Markus Mützel <mmuetzel>
Group administrator
Fri 02 Nov 2018 05:52:20 PM UTC, comment #7: 

Interesting.  Octave is, in effect, even more performance-oriented in that it does not calculate the normals for surface objects unless required.  I would actually be inclined to keep the Octave-behavior, unless there is only a small performance benefit.

This was my attempt to check that


octave:22> Z = peaks (100);
octave:23> h = surf (Z, 'facelighting', 'none');
octave:24> hl = light;
octave:25> tic; set (h, 'facelighting', 'gouraud'); toc
Elapsed time is 0.00257993 seconds.


2.6 milliseconds doesn't seem like very much.  I guess I'm okay making the change, or leaving it as is if that would be simpler.

Rik <rik5>
Group administrator
Fri 02 Nov 2018 05:04:01 PM UTC, comment #6: 

Here the results with Matlab R2016a for a surface object:

>> format compact
>> clf;
>> Z = peaks ();
>> h = surf (Z, 'facelighting', 'none');
>> get (h, 'facelighting')
ans =
none
>> isempty(get (h, 'vertexnormals'))
ans =
     0
>> set (h, 'facelighting', 'gouraud')
>> isempty(get (h, 'vertexnormals'))
ans =
     0
>> hl = light ();
>> isempty(get (h, 'vertexnormals'))
ans =
     0


And for the sake of completeness with flat lighting:

>> clf;
>> Z = peaks ();
>> h = surf (Z, 'facelighting', 'none');
>> get (h, 'facelighting')
ans =
none
>> isempty(get (h, 'facenormals'))
ans =
     0
>> set(h, 'facelighting', 'flat')
>> isempty(get (h, 'facenormals'))
ans =
     0
>> hl = light ();
>> isempty(get (h, 'facenormals'))
ans =
     0


Similarly, for a patch object:

>> clf;
>> h = patch('facelighting', 'none');
>> get (h, 'facelighting')
ans =
none
>> isempty(get (h, 'vertexnormals'))
ans =
     1
>> set (h, 'facelighting', 'gouraud')
>> isempty(get (h, 'vertexnormals'))
ans =
     1
>> hl = light ();
>> drawnow
>> isempty(get (h, 'vertexnormals'))
ans =
     0


And with flat lighting:

>> clf;
>> h = patch('facelighting', 'none');
>> get (h, 'facelighting')
ans =
none
>> isempty(get (h, 'facenormals'))
ans =
     1
>> set(h, 'facelighting', 'flat')
>> isempty(get (h, 'facenormals'))
ans =
     1
>> hl = light ();
>> drawnow
>> isempty(get (h, 'facenormals'))
ans =
     0


So it looks like that delayed calculation of normals only applies to patch objects. Both kinds of normals seem to be calculated for surfaces immediately.

Calculating the normals for surfaces is easier because all vertices are on a regular grid (no self-intersection, ...). The logic involved for patches is more complex. That might be a motivation for the difference in behavior.
Is this what we want to do as well? If yes, I can try to create a patch over the weekend.

This would also fix surfl without any further changes to the function itself.

Markus Mützel <mmuetzel>
Group administrator
Wed 31 Oct 2018 10:50:52 PM UTC, comment #5: 

I filed a new Feature Request to implement the "light" option.  See bug #54930.

Rik <rik5>
Group administrator
Wed 31 Oct 2018 10:20:08 PM UTC, comment #4: 

You are right. That should have read "VertexNormals" (not "EdgeNormals").

If I remember correctly the behaviour was as described when I tested back in May 2016 with Matlab R2012a for patch #8951. But that might have changed. (We are using R2016a now at work.)
I'll probably be able to test that code snippet on Friday.
Depending on the results, I'll try and adapt the patch and re-word that section in the NEWS file.

I thought the same when I read that part about the "light" option. And I concur with you that this can be fixed in a separate patch or bug report.

Markus Mützel <mmuetzel>
Group administrator
Wed 31 Oct 2018 09:21:29 PM UTC, comment #3: 

I agree that we don't want to calculate the normals unless it is necessary.

Matlab (http://www.mathworks.com/help/matlab/ref/matlab.graphics.primitive.surface-properties.html) is a little unclear about when it calculates the normals.  "If you do not specify normal vectors, then the surface generates this data for lighting calculations."  Does that mean only when a light object is added?  Only when the "facelighting" property changes?

If you have access to Matlab the following code would be useful to test.


Z = peaks ();
h = surf (Z, 'facelighting', 'none');
get (h, 'facelighting')
get (h, 'vertexnormals')
set (h, 'facelighting', 'gouraud')
get (h, 'vertexnormals')
hl = light ();
get (h, 'vertexnormals')


In the NEWS section, the description begins


 ** "FaceNormals" and "EdgeNormals" for patch and surface graphic


But, I don't see any graphic property called "EdgeNormals" for patches or surfaces.  Should this be "VertexNormals"?

I think it would be a good idea to add to the description in NEWS the conditions required before the "*Normals" will be calculated.  Something, like 'The "FaceLighting" property must be set to "flat" (FaceNormals) or "gouraud" (VertexNormals), AND a light object must be present in the axes, before any normals will be calculated.

I would add a FIXME note in surfl.m above where you create a light object and then immediately delete it.  Without any explanation, it looks weird, and someone at a later date might come along and try to clean up the code by removing it.

Finally, and this might be the subject of a separate bug report, the documentation for surfl says


The default lighting mode "cdata", changes the cdata property of
the surface object to give the impression of a lighted surface.
*Warning:* The alternative mode "light" mode which creates a light
object to illuminate the surface is not implemented (yet).


Since light objects now exist, couldn't this option be implemented?

Rik <rik5>
Group administrator
Wed 31 Oct 2018 08:16:40 PM UTC, comment #2: 

Thanks Rik for pointing this to me.

With that change, we only calculate face or vertex normals "if they are needed". That means, "facenormals" are calculated only for flat lighting and only if a light object is present in the same axes. Similarly, "vertexnormals" are calculated only for gouraud lighting and if a light object is present in the same axes.
This was done to avoid calculating these properties when they aren't necessary (could be costly if there were many faces and vertices). This is also what Matlab seems to do.

The attached patch sets the "facelighting" property of the surface that is created in surfl and temporarily adds a light object to its parent axes.

It also adds an "only" to the respective sentence in the "NEWS" file. Please let me know if we should be any more wordy there. This might be a compatibility consideration for existing user code as well.

Alternatively, we could always calculate the face and vertex normals (not only if certain pre-conditions are met).

(file #45340)

Markus Mützel <mmuetzel>
Group administrator
Wed 31 Oct 2018 04:28:25 AM UTC, comment #1: 

Using hg bisect, the changeset which introduced this problem was


changeset:   25867:49ffa78f9243
user:        Markus Mützel <markus.muetzel@gmx.de>
date:        Sat Sep 08 20:36:30 2018 +0200
summary:     Use "facenormals" for flat lighting on surfaces (bug #54024).


Adding Markus to the CC list for this bug report.

Rik <rik5>
Group administrator
Tue 30 Oct 2018 06:59:10 PM UTC, original submission:  

I get an error when running the demos for surfl.  See session log below.


octave:1> demo surfl 1
surfl example 1:
 clf;
 [X,Y,Z] = sombrero ();
 colormap (copper (64));
 surfl (X,Y,Z);
 shading interp;
 title ("surfl() with defaults");

surfl example 1: failed
vn(_,_,2): but vn has size 0x0


The problem is in surfl.m


    vn = get (htmp, "vertexnormals");
    dar = get (hax, "dataaspectratio");
    vn(:,:,1) *= dar(1);
--> vn(:,:,2) *= dar(2);
    vn(:,:,3) *= dar(3);


The issue is that vn is empty.  I went and checked with Octave 4.2.1 and apparently there should be data there.

Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45354:  bug54922_surfl_vertexnormals_v2.patch added by mmuetzel (8KiB - application/octet-stream)
file #45340:  bug54922_surfl_vertexnormals.patch added by mmuetzel (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-11-04 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2018-11-03 mmuetzel Attached File- Added bug54922_surfl_vertexnormals_v2.patch, #45354
        StatusNeed Info Patch Submitted
    2018-10-31 rik5 StatusPatch Submitted Need Info
    2018-10-31 mmuetzel StatusPatch Reviewed Patch Submitted
    2018-10-31 rik5 StatusPatch Submitted Patch Reviewed
    2018-10-31 mmuetzel Attached File- Added bug54922_surfl_vertexnormals.patch, #45340
        StatusConfirmed Patch Submitted
        Operating SystemGNU/Linux Any
    2018-10-31 rik5 StatusNone Confirmed
    2018-10-31 rik5 Carbon-Copy- Added mmuetzel

    Back to the top

    Powered by Savane 3.13-aa77.
    Corresponding source code