patchGNU Octave - Patches: patch #8951, Automatically calculate...


patch #8951: Automatically calculate "vertexnormals" and "facenormals" for patch objects

Submitted by:  Markus Mützel <mmuetzel>
Submitted on:  Thu 17 Mar 2016 06:48:44 PM UTC  
Category:  Core : other Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed

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)

Tue 21 Aug 2018 08:07:10 PM UTC, comment #33:

The part of bug #54024 that kept this patch open is now fixed.
Closing report.

Markus Mützel <mmuetzel>
Project Member
Fri 13 Jul 2018 05:46:11 PM UTC, comment #32:

I pushed the documentation change here:

I hope bug #54024 can be fixed before Octave 5.0 is releases. If not, some explanation should be added.

Markus Mützel <mmuetzel>
Project Member
Thu 12 Jul 2018 08:44:56 PM UTC, comment #31:

The attached patch adds some language to the "patch properties" section of the manual explaining in which cases normals are automatically calculated. I also already put in that facenormals will be used for "flat" lighting although this is not yet implemented (bug #54024).

(file #44543)

Markus Mützel <mmuetzel>
Project Member
Thu 12 Jul 2018 12:07:28 PM UTC, comment #30:

Since the docstrings for patch and surface now reference "Patch/Surface Poperties" as being the place where detailed help may be found, I'd say all those properties should be documented there in the first place.

Pantxo Diribarne <pantxo>
Project Member
Thu 12 Jul 2018 10:10:11 AM UTC, comment #29:

As cross-reference from bug #53958:
The "VertexNormals" are calculated only if "VertexNormalsMode" is "auto", "FaceLighting" or "EdgeLighting" are set to "gouraud" and a light object is present in the same axes.
For the "FaceNormals" to be calculated, "FaceLighting" or "EdgeLighting" has to be set to "flat" (which is the case with the default values) together with the other two conditions.
That is what Matlab seemed to do when I looked at it.

Maybe this should be documented somewhere. But I don't know where this would fit. In the docstrings for patch and surface maybe?

Markus Mützel <mmuetzel>
Project Member
Tue 05 Jun 2018 08:14:43 PM UTC, comment #28:

I pushed the patch here:

I hope that improves things.

Markus Mützel <mmuetzel>
Project Member
Mon 04 Jun 2018 09:36:38 PM UTC, comment #27:

The attached patch removes the listeners before traversing down the children tree.

Alternatively, we might be able to check in the callback function (gh_manager::do_execute_callback) whether we are operating on a valid object.

(file #44293)

Markus Mützel <mmuetzel>
Project Member
Mon 04 Jun 2018 09:22:43 PM UTC, comment #26:

It looks like the listeners are executed after the object was invalidated.
I'll try to find out why this is happening.

Markus Mützel <mmuetzel>
Project Member
Mon 04 Jun 2018 08:55:26 PM UTC, comment #25:

Happy to see you revive this old patch :-).

Though, I currently get several callback errors after closing the complicated "demo hold 6":

Those errors dissapear if I revert before the following cset

Pantxo Diribarne <pantxo>
Project Member
Mon 04 Jun 2018 08:03:29 PM UTC, comment #24:

I addressed Pantxo's comment #19 here:

And pushed an updated patch here:

Marking as ready for test.

Markus Mützel <mmuetzel>
Project Member
Thu 31 May 2018 06:29:26 PM UTC, comment #23:

The attached patch is updated to apply on a current tip.

(file #44264)

Markus Mützel <mmuetzel>
Project Member
Mon 18 Jul 2016 04:42:37 PM UTC, comment #22:

Pantxo, when you have some spare time, could you please review bug #47677? I would like to relie here on the non-planar patch detection logic added in the changeset there.

Markus Mützel <mmuetzel>
Project Member
Wed 08 Jun 2016 05:16:25 PM UTC, comment #21:

With the attached patch, a new light object triggers the axes object to call update_normals for all patch (and maybe in the future also surface) objects.

I think I now understand why "numlights" never decreased with your patch: In "delete_graphics_object", gh_manager::free (h) is called before the call to remove_child. Thus, the gh_manager could not find the light object by its handle.

Anyway, since it is quite complicated to keep track of which lights are directly within the axes or in (nested) hggroups and which of them are invisible or in invisible (nested) hggroups, I kept with the original approach of detecting one visible light that descends from the axes ancestor of the patch object.

I removed all of the debugging code and re-organized the BISTs so that the patch should be clean now.

If the current patch is more or less what we want, I will start to update the documentation.

(file #37433)

Markus Mützel <mmuetzel>
Project Member
Tue 31 May 2016 06:29:32 PM UTC, comment #20:

Pantxo, no worries about sparse answers. I did not have time for coding until last week anyway.

Thank you, for your patch. I tried to no avail with the constructors and destructors of the light object itself.
Your idea might work as well. However, lights that are children of a hggroup also have to be counted/detected (and registered in the parent axes?).
I might have done something wrong while merging your patch with my changes (see attached patch). But for me, numlights never decreases. (I added a line to axes::properties::set_xlabel to print its current value.)

The signs of the normals now match your test results.

With patch #8943 pushed, I removed the setting "always" for the *normalsmodes.

If I understood correctly, adding a light to the same axes triggers the calculation of the normals corresponding to the settings of "edgelighting" or "facelighting".
While the normals are correctly calculated when the light is already present when adding the patch object, I do not know how to efficiently trigger the calculation when adding the light after the patch object.

This is working ("light" before "patch"):

With the following ("light" is called after "patch"), the vertexnormals are updated after setting "facelighting" to "gouraud". The facenormals are always empty:

Do you have an idea how to solve that issue?

Please, do not push the attached patch. It is a wip with unfinished functionality, debug messages and possible errors...

(file #37325)

Markus Mützel <mmuetzel>
Project Member
Mon 30 May 2016 08:49:48 PM UTC, comment #19:

Sorry Markus for such sparse answers.

Counting parent axes light objects: did you try the attached approach (overloading base::properties::adopt/remove_child for axes object should be enough).

Algorithm for normals computation: you seam to have tested the accuracy of the face normals more then ever I will so let's adopt your approach :-). Did you solve the reverse sign as compared to ML normals?

(file #37312)

Pantxo Diribarne <pantxo>
Project Member
Wed 04 May 2016 02:31:52 PM UTC, comment #18:

I tried to implement counting lights in patch #8943. But taking reparenting and so on into account it got quite messy very fast. So maybe it is better to check whether there is a light object by iterating through the children of the axes object.
This is what is done in the attached patch.

(file #37065)

Markus Mützel <mmuetzel>
Project Member
Tue 03 May 2016 03:13:52 PM UTC, comment #17:

The normals for surface objects are always calculated for the constituting rectangles. Patches can consist of many different polygons. Probably, it would be easier to match the implementation for surfaces to that special case (as it is right now for vertexnormals for surfaces). But I will keep that in mind.

I will write public get and set methods for num_lights in axes. Thank you for a pointer to that. At the moment num_lights is always 0. When "light" is implemented in Octave, its values has to increase when a light object is added and decrease when it is deleted. Probably, that value has to be changed also when the "Visible" property of the light object is changed. (Still to be added to patch #8943). Maybe add_light and remove_light methods would be better in that case?

The Newill method does not always calculate a good normal, e.g.:

The normal at any one corner is [0 0 1] (or [0 0 -1]).

Or a more obfuscated example (above vertices rotated around axis [1 0 1] by 0.3rad):

Due to numerical effects (double precission), the normal calculated by the Newill method (fn) is quite different from the normals calculated at any corner: +/- [0.0224, 0.2090, 0.9777].

I did not yet time svd nor the Newill method. But it might be an idea to check the length of the normal that results from the Newill method and decide on that whether we want to calculate the normal at one corner of the polygon instead.

However, we might have to find the non-planar polygons anyway to specially handle them in (bug #47677). So I had the vague idea to store that information somehow in the graphics object to re-use it later. If we need to calculate the svd anyway (do we?), I figured it would not matter when it would be calculated and fall back to the faster cross-product (instead of the Newill method) in the cases where possible.

Markus Mützel <mmuetzel>
Project Member
Mon 02 May 2016 09:00:10 PM UTC, comment #16:

Just in case, I don't think surface normals should be part of this patch, just preparing the future.

Pantxo Diribarne <pantxo>
Project Member
Mon 02 May 2016 08:58:12 PM UTC, comment #15:

I still didn't look at your patch carefully, sorry for the sparse hints, but here are a few comments:

  • I suppose vertex/facenormals will be computed for surfaces also in the future. To avoid duplicate code, you could do most of the normals computation in static functions so that they are accessible from both patch and surface objects.
  • num_lights: We generally don't use public attributes and prefer methods that return the value of private internals. You could define a axes::get_num_lights public method. Also in the current state of the patch I can't see anything that increments/decrements num_lights. Am I wrong?
  • As I understand, the Newill method is the most general and robust, right? Can you demonstrate that Newill method is such a performance hit (or whatever reason) that we must use it only for non planar polygons. I find those planarity checks complicate the code, but if it is needed I am fine.
Pantxo Diribarne <pantxo>
Project Member
Mon 02 May 2016 07:01:49 PM UTC, comment #14:

Pantxo, your reasoning for when to calculate which normals makes perfect sence.

At the moment, the normal of the first edge is used as the face normal in glrender. When this one here finally works, glrender can be adapted, too.

Attached is another update which fixes a bunch of issues with the previous ones.

(file #37052)

Markus Mützel <mmuetzel>
Project Member
Mon 02 May 2016 12:19:35 PM UTC, comment #13:

As you are now a frequent contributor, it would be even better if you asked for tests on the help/maintainers mailing lists. This is the best way to do clean reverse engineering. Generally answers come in a few hours.

>>Do I understand correctly that Matlab only triggers the calculation of FaceNormals if the patches have polygons with more than 3 corners?

Yes, that is what I mean, but I concluded too fast, the test in comment 10 shows that ML computes normals even for flat lighting. So I don't know what went wrong with those tests.

On the other hand, I think the reason why vertexnormals are not computed for "flat" lighting is because vertexnormals don't have a unique value here: I'd bet when two faces share vertices - say [abc] and [bcd] are triangular faces with "b" and "c" common vertices - the rendering in opengl uses actually 6 vertices (no triangle strip/fan) - [abc] and [b'c'd]. In flat lighting "b" and "c" inherit the facenormals of face "abc" while "b'" and "c'" inherit from "bcd" facenormals (hence vertexnormals has no sense).

Pantxo Diribarne <pantxo>
Project Member
Mon 02 May 2016 09:35:31 AM UTC, comment #12:

Pantxo, thank you for your tests.

I am a little bit puzzled why they did not trigger the calculation of any of the two types of normals.
I mis-understood your previous comment #8 in that "flat" would trigger the calculation of both normal types. After your comment #10, I forgot to update the tests.

Do I understand correctly that Matlab only triggers the calculation of FaceNormals if the patches have polygons with more than 3 corners?

Deprecating NormalMode and introducing FaceNormals and the respective *Modes was done in bug #47791. (Thank you for pushing that one.) Octave and Matlab seem to be behave quite similar wrt this now.

I will try and see whether I can get purchased a newer Matlab version so that I do not have to steal so much of your time.

Markus Mützel <mmuetzel>
Project Member
Mon 02 May 2016 08:05:10 AM UTC, comment #11:

Sorry for the delay. Here are the answers for comment #9:

  • Snippet 1: only facenormals are computed as I said in my previous comment ([0.7071 0 0.7071]).
  • Snippet 2: for aligned vertices nothing is drawn, no warning, both normals are empty
  • Snippet 4: the normalmode property can still be get/set but a warning is displayed "Warning: The NormalMode property will be removed in a future release. "
  • The results of your tests are not very informative since for triangular patched and 'flat' lighting, ML doesn't seam to return any value for facenormals (I can confirm that lighting is enabled on-screen though):
Pantxo Diribarne <pantxo>
Project Member
Mon 25 Apr 2016 07:05:34 PM UTC, comment #10:

For the orientation (probably a long shot), did you see this comment in

I don't know what this is about but I though it could be related.

I had another test I didn't paste that demonstrates that when a patch is first created with "flat" lighting, only the facenormals are computed. Then when switching to "gouraud", the vertexnormals are computed and the facenormals are leftover:

I'll run your snippets by the end of the week.

Pantxo Diribarne <pantxo>
Project Member
Mon 25 Apr 2016 06:00:59 PM UTC, comment #9:

Thank you, Pantxo, for your tests in Matlab R2016a.
The underlying logic seems to have changed significantly since R2012a. Not only the lengths of the normals have changed but also their orientation.

Attached, please find a wip patch that gets the length and orientation of the normals right for your examples. However, they point into the opposite direction.

The conditions for when to calculate the normals when the "*Mode" is set to "auto" should now also be implemented like you explained.
Just wondering: Is the behavior different when you create the patch with "FaceLighting" set to "flat". If it is related to lighting, it would make sense to only calculate "FaceNormals" and not calculate "VertexNormals" in that case. And it would actually save some calculations...

With the current patch, the FaceNormals are re-used to calculate the VertexNormals only if both get calculated automatically. If Matlab does something different, we can try to adapt.

I used the rank of the matrix with the corners of each face to determine whether the face is planar (only for faces with more than three corners). Only in that case Newill's method is used. Otherwise, the normal is calculated at only one corner. Maybe, I should add a test to see whether that corner is straight and move to the next one in that case...
What does Matlab do for:

"NormalMode" shadows "VertexNormalMode". One set and one get method with only one line of code each should be enough. No big maintenance burden. "NormalMode" is a hidden property so that it is not listed for e.g.

Could you please test whether it is still working in ML2016a?

As long as it is working with current Matlab versions, it does not harm to keep it around imho.

The "always" option for the "*Mode" properties is just there to see which results I get for the normals while "light" is not available in Octave. For the final patch that hopefully can eventually be pushed after a lot more polish, it should probably be removed.

Can you please run the following examples and post the results to deduce in which direction the normals should point?

This for what is done on shared edges:

And this for 2d-patches:

Somewhat off-topic: It looks like "NormalMode" was recently (some time after R2012a) replaced by "VertexNormalsMode" for surfaces, too. Similarly to "patch", "FaceNormals" and "FaceNormalsMode" got added [1]. I will report a separate bug for those changes.


(file #36992)

Markus Mützel <mmuetzel>
Project Member
Sun 24 Apr 2016 08:23:33 PM UTC, comment #8:

I didn't have time to look into your patch yet but I ran the following example at work in ML2016a on a Unix server (with software opengl rendering) and Windows laptop.

  • For non planar patches vertexnormals are the same for all vertices, so vertexnormals only seam to depend on the "average" face normal (and eventually that of neighbor faces), not on the actual triangulated on-screen representation.
  • facenormals are only computed when in "flat" lighting. Not necessary to follow this behavior but it is probably a hint on how ML uses face vs vertex normals when rendering in opengl.
  • Both facenormals and vertexnormals are normalized.
  • ML only computes normals if a light is present and any of facelighting and edgelighting is not "none".

I will run more examples, including changing one normal (e.g. facenormals) manually and seeing if the other normal is affected.

As for keeping normalmode around, as a clone of vertexnormalmode it just depends on how much maintenance burden it brings. In any event it has to be planed for deprecation from the start (a clear FIXME at least).

I'd prefer to avoid the addition of the "always" value: it is misleading and those *mode properties become special cases.

Pantxo Diribarne <pantxo>
Project Member
Sat 16 Apr 2016 10:55:54 AM UTC, comment #7:

Thank you for the link to Newill's method and your other suggestions.

The attached patch uses it to calculate the facenormals for non triangular faces.

I would personally prefer to leave "normalmode" in as a hidden property because I have some Matlab code using it and I would like to see it running in Octave without too many "if"s.

I am waiting with changes to the calculation of vertexnormals for non-triangular faces for bug #47677.
Are you aware of a fast way to check whether a set of points lies in one plain (same normal for all vertices) or, in a following step, how to group them in "sub-plains" if necessary (same normals for all normals in each group)?
I was thinking about checking whether the triple product was zero for all series of four subsequent face corners. But maybe there is something more efficient.

If "*NormalsMode" is set to "auto", normals are only calculated if lights are present (and visible?) in the same axes and if lighting is active for the patch. I don't know what Matlab is doing exactly. But that seemed reasonable to me.
I am going to change patch #8943 to account for this if you think this is the right way to go.

To always get the normals, set the "*NormalsMode" to the "non-standard" value "always".

Probably, it is possible to use some of the facenormals to speed up the calculation of vertexnormals if both are in mode "auto" or "always". But the facenormals for non-planar faces are probably not the right ones to use for the vertexnormals.

I tried with patches with several 1000 vertices and faces and did not notice any obvious speed impact. However, I am not sure how to time this exactly.

I will adapt the doc string and tests when we are sure enough that nothing will change dramatically.

(file #36949)

Markus Mützel <mmuetzel>
Project Member
Tue 12 Apr 2016 08:25:59 PM UTC, comment #6:

A link for Newill method:

Pantxo Diribarne <pantxo>
Project Member
Tue 12 Apr 2016 08:25:15 PM UTC, comment #5:

I don't have Matlab at hand, I just read the most recent doc. As Octave has never supported lighting, there is no backward compatibility issue here and we should implement the current ML behavior in my opinion: users who want the old ML behavior just have to change normalmode to vertexnormalmode.

For facenormal of non-planar (even if Octave doesn't currently support those) or eventually concave polygons the Newill method (see e.g. [1]), that is used by ML (see link in comment #3), seams to be more appropriate than the cross product. Did you have a look at it?

Pantxo Diribarne <pantxo>
Project Member
Tue 12 Apr 2016 06:52:49 AM UTC, comment #4:


Thank you for reviewing this patch.

The reason, I only implemented "VertexNormals" is that I only had Matlab R2012a to compare to. In that version, there was no "FaceNormals". Also "VertexNormalsMode" was called "NormalMode" then. Maybe it would be a good idea to keep the deprecated property "NormalMode" around for legacy to mirror "VertexNormalsMode". Otherwise, some older code might not run as expected.

As you already pointed out, "FaceNormals" are calculated on the way. So it should not be too hard to implement that property with the same patch.

With that older Matlab version, the "VertexNormals" are calculated whenever "NormalMode" is set to "auto" (default), even if no light object is present in the same axes. However, they are only really needed when lights are present. Thus, especially if Matlab changed that behavior in more recent versions, I am happy to adapt the code for this. Which circumstances must be met exactly to trigger normals calculation?
That would however mean that the code would be harder to test since light objects are (not yet) available in Octave. So maybe wait with this change for a later version of the patch or implement a temporary trigger for testing?

I did not really time it for large patches but did not notice significant performance hits with "normal" ones due to this patch. I will try to do this.

Good you mentioned it because I did not check before: Matlab R2012a supports non planar patches:

However, the VertexNormals calculated in this case are different from what would be calculated with my patch. I will try to figure out how to do better...
Do you have access to a newer version of Matlab and give the results for the above example (maybe after calling "light()" or what else is necessary to activate normal calculation)? What value does "FaceNormals" have for that non-planar patch?

This code example does not work in Octave 4.0.1 erroring out with a syntax error. With changed syntax, Octave produces a patch that looks different from Matlab:

I think these are however two unrelated bugs. Fields with uppercase letters are not correctly recognized in Octave.

I will file two separate bug reports for these issues if not yet reported.

The weights for the average in step 2 are the lengths of the normal vectors. These are not normalized in step 1. Thus, normal vectors from larger triangles have a higher weight.

Markus Mützel <mmuetzel>
Project Member
Mon 11 Apr 2016 08:05:53 PM UTC, comment #3:

A few other comments:

  • now I see you have facenormals for free as a by product of your algorithm for computing vertexnormals. This is at least another good reason to switch to ML compatible properties.
  • I don't think Octave supports non planar patches, does Matlab do? If not then your algorithm can be simplified for polygons: any three vertices are enough to compute the normal for the entire face.
  • In step 2, I see a comment about weighted average, based on adjacent faces area, but can't find it afterwards. Shouldn't weighting be done in Step 1?
Pantxo Diribarne <pantxo>
Project Member
Mon 11 Apr 2016 05:23:27 PM UTC, comment #2:

Hi Markus,

I didn't take enough time to review this patch but I hope to do so, step by step with the few spare hours I have. Here is my first comment: Matlab has two distinct properties for patch normals, vertexnormals and facenormals (and their corresponding *mode). Can you clarify the role of those two and why you chose to implement vertexnormals only. We can add the support for one after the other but at least we should change normalmode to vertexnormalmode.

Also ML only computes those properties when a light object is present, did you notice a significant performance penalty over large number of patches?


Pantxo Diribarne <pantxo>
Project Member
Thu 24 Mar 2016 03:28:35 PM UTC, comment #1:

The attached patch fixes the failing test described in comment #0.

(file #36747)

Markus Mützel <mmuetzel>
Project Member
Thu 17 Mar 2016 06:48:44 PM UTC, original submission:

This patch adds the functionality to calculate vertexnormals for patch objects.

While it seems to work quite good in most cases, I do not know how to immitate Matlab's behavior in the following case (a test is included):
In Matlab:

The vertexnormals should be calculated while the patch is created. I managed to get that behavior after calling "reset (hpa)". But I do not know where to call update_normals to trigger it when the patch is created with default values.

I would appreciate any help, tips, info and opinions.

Markus Mützel <mmuetzel>
Project Member


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

Attach Files:

Attached Files
file #44543:  patch8951_patch_normals_doc.patch added by mmuetzel (2KiB - application/octet-stream)
file #44264:  patch8951_patch_normals_v7.patch added by mmuetzel (20KiB - application/octet-stream)
file #37433:  patch_normals_v6.patch added by mmuetzel (21KiB - text/x-diff)
file #37325:  patch_normals_v5.patch added by mmuetzel (19KiB - text/x-diff)
file #37312:  numlights.patch added by pantxo (2KiB - text/x-diff)
file #37065:  patch_normals_v4.patch added by mmuetzel (18KiB - text/x-diff)
file #37052:  patch_normals_v3.patch added by mmuetzel (17KiB - text/x-diff)
file #36992:  patch_normals_v2.patch added by mmuetzel (17KiB - text/x-diff)
file #36949:  patch_normals.patch added by mmuetzel (19KiB - text/x-diff)
file #36747:  patch_vertexnormals_v2.patch added by mmuetzel (12KiB - application/octet-stream)
file #36674:  patch_vertexnormals.patch added by mmuetzel (12KiB - application/octet-stream)


Depends on the following items: None found

   bug dependencies.


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

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

    Only logged-in users can vote.




    Follow 18 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-08-21 mmuetzel StatusReady For Test => Done
        Open/ClosedOpen => Closed
    2018-07-12 mmuetzel Attached File- => Added patch8951_patch_normals_doc.patch, #44543
    2018-06-04 mmuetzel Attached File- => Added patch8951_remove_listeners.txt, #44293
    2018-06-04 mmuetzel StatusIn Progress => Ready For Test
    2018-05-31 mmuetzel Attached File- => Added patch8951_patch_normals_v7.patch, #44264
        SummaryCalculate vertexnormals for patch objects => Automatically calculate "vertexnormals" and "facenormals" for patch objects
    2016-07-07 lachlan CategoryNone => Core : other
    2016-07-06 lachlan StatusNone => In Progress
    2016-06-08 mmuetzel Attached File- => Added patch_normals_v6.patch, #37433
    2016-05-31 mmuetzel Attached File- => Added patch_normals_v5.patch, #37325
    2016-05-30 pantxo Attached File- => Added numlights.patch, #37312
    2016-05-04 mmuetzel Attached File- => Added patch_normals_v4.patch, #37065
    2016-05-02 mmuetzel Attached File- => Added patch_normals_v3.patch, #37052
    2016-04-25 mmuetzel Attached File- => Added patch_normals_v2.patch, #36992
    2016-04-16 mmuetzel Attached File- => Added patch_normals.patch, #36949
    2016-03-24 mmuetzel Attached File- => Added patch_vertexnormals_v2.patch, #36747
    2016-03-17 mmuetzel Attached File- => Added patch_vertexnormals.patch, #36674

    Back to the top

    Powered by Savane 3.3