I pushed the following change to make coplanar_last_idx a std::vector and thereby hopefully fix the inconsistency in calc_face_normals:
http://hg.savannah.gnu.org/hgweb/octave/rev/8c1cb06468db

I think this is the final change needed for this report.

Using std::vector seems right to me as well.

Oh sorry... I forgot to say that, pls discard the previous patch (file #46557) and use the one in Comment #6 (file #46562) ..... anyway, I find it is not perfect either. Let's start from what is in repository now.

In your patch (file #46563), this

-            for (int i = 0; i < nf; i++)
+            for (int i = 0; i < nf; i++, it1++)

assumes coplanar_last_idx has `nf` number of elements. But that's not true in general, e.g. when `octave::math::isnan (idx(3,jj))` is triggered, coplanar_last_idx will have fewer elements. That's why the fix in (file #46562) is like:

+                bool tested_coplanarity = props.coplanar_last_idx.size () > 0
+                                          && ! octave::math::isnan (f(i,3));
#+                                          && ! clip_f(i); // no more need test "&& ! clip_f(i)"
                 bool is_non_planar = false;
-                if (props.coplanar_last_idx.size () > 0 && (*it1).size () > 1)
+                if (tested_coplanarity && (*it1).size () > 1)
...
-                if (is_non_planar)
+                if (tested_coplanarity)
                   it1++;

I guess the use of linked list for `coplanar_last_idx` was intended for saving memory and for speed (a lots .push_back()), i.e. there might be not so many non-trivial case.

If we now use coplanar_last_idx in a "dense" way (have nf elements), the advantage of using linked list will all lose. Instead, using vector< vector<octave_idx_type> > will be better for this "dense" case.

There is an extra place may need to be fix which is not covered in my last patch (file #46562). That is `patch::properties::calc_face_normals` in file graphics.cc.

  for (octave_idx_type i = 0; i < num_f; i++)
    {
      bool is_coplanar = true;
      if (coplanar_last_idx.size () > 0)
        {
          if ((*cp_it).size () > 1)
          {
            is_coplanar = false;
          }
          cp_it++;
        }

Depending on coplanar_last_idx.size() == num_f is true or false, the `cp_it++;` may need some condition.

I was thinking that the non-coplanar faces with unclosed contour might be needed, that's why the question 1,2,3 in Comment #1 and Comment #2.

But after reading the corresponding part of `opengl_renderer::draw_patch`, I realize that when it is a unclosed contour, result of coplanar test is not used at all, also it is drawn as a line with no face. So, to my understanding, at least in current stage, no coplanar test is required in such case.

They are different bugs, but related because of the variable `coplanar_last_idx`. Coplanar tester is the writer, `opengl_renderer::draw_patch` is the reader. The decision to not include unclosed patch in coplanar_last_idx will affect both sides.

I put the fix to both bugs in one patch (see attached file), otherwise the patches will depends on each other to work correctly. I also moved the coplanar test&partition code into a new function, because I think it is complex enough and independent enough.

I tested the samples in comment #4, comment #3, comment #0, comment #0 in bug #55751 and a patch demo. No problem so far.

make check failed at bsxfun.cc-tst, so I can't run a full test.

(file #46562)

Thanks for the clarification.

In the attached patch, I skipped coplanar test when any of the face vertex contain NaN or Inf, matches the original behavior.

I checked the implementation `opengl_renderer::draw_patch` in gl-render.cc. The coplanar test results (in `coplanar_last_idx`) are used in this way:

  // nf = number of faces/polygon
  for (int i = 0; i < nf; i++)
    {
      if (clip_f(i))
        continue;

      bool is_non_planar = false;
      if (props.coplanar_last_idx.size () > 0 && (*it1).size () > 1)
        {
          is_non_planar = true;
          ...
        }
      ...
    }
  ...
  for (int i = 0; i < nf; i++)
    {
      bool is_non_planar = false;
      if (props.coplanar_last_idx.size () > 0 && (*it1).size () > 1)
        is_non_planar = true;
      if (clip_f(i) || is_non_planar)
        {
          // This is an unclosed contour or a non-planar face.
          // Draw it as a line.
         ...
      ...
    }

In both for-loop, whenever clip_f(i) == true (face `i` is unclosed contour, i.e. contain NaN or Inf vertex), `is_non_planar` is then useless. In other words, an "unclosed contour" is distinct from a face (planar or non-planar), only the face need coplanar test. This also justifies my patch.

A separate but related issue, in `opengl_renderer::draw_patch`, it seems that the coplanar test results are not properly used sometimes. Can be observed like this:

clf;
v = [0 0 0; 1 0 0; 1 1 0; 0 1 0; 0 1 1; 0 0 1;];
f = [1:4; 3:6];  % buggy patch
%f = [3:6; 1:4];  % correct patch
patch ('Vertices', v, 'Faces', f, 'FaceColor', 'g');

Should I fix that also?

(file #46557)

And a more subtle question is:

3. What if the NaN occurred in the middle (and probably multiple NaN)?

Should I view the NaN as a separator so that vertices are split to different groups (different coplanar test within each group), or treat vertices as a single face (i.e. viewing NaN as missing/ignored data), coplanar test for all non-NaN (and non-Inf?) vertices as a whole.

I feel the later behavior is expected.

BTW: It seems to me, in the bug demo, the coplanar test should not be called in the first place, there is no "face" there and no complex lighting (computed from normal vectors of each plane) is required.

Probably the new co-planar test for patches is the source of this trouble.

Download the attached variable file and m-file and then run tst_patch.  This works on 5.1.0, and fails on dev with this message:

tst_patch
error: __go_patch__: EIG: matrix contains Inf or NaN values