bugGNU Octave - Bugs: bug #55895, __go_patch__ fails when NaN...

 
 

bug #55895: __go_patch__ fails when NaN vertices exist

Submitter:  Rik <rik5>
Submitted:  Tue 12 Mar 2019 06:59:30 PM UTC
   
 
Category:  Plotting 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

Tue 10 Dec 2019 11:21:09 PM UTC, comment #15: 

This seems right.  The example produces from comment #3 produces the same results in Octave and Matlab.

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Tue 10 Dec 2019 10:20:22 PM UTC, comment #14: 

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.

Markus Mützel <mmuetzel>
Group administrator
Mon 01 Apr 2019 10:22:25 PM UTC, comment #13: 

Is there more that needs to be done for this bug report?  Should std::vector replace std::list in graphics.in.h for coplanar_last_idx?  Is there extra code to add for calc_face_normals?

Rik <rik5>
Group administrator
Thu 21 Mar 2019 06:24:15 PM UTC, comment #12: 

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

Rik <rik5>
Group administrator
Thu 21 Mar 2019 03:20:27 PM UTC, comment #11: 

If I understood correctly, coplanar_last_idx only ever had less elements than faces in the patch if there were unclosed contours. I'd guess that the default case is that all faces are closed.
So I agree with you that coplanar_last_idx should better be dense (and a std::vector).
Also looking at [1] makes me believe that std::vector would be the better choice.
Using a "dense" vector would also simplify the conditions on when to increment the iterator...

[1]: https://baptiste-wicht.com/posts/2012/12/cpp-benchmark-vector-list-deque.html

Markus Mützel <mmuetzel>
Group administrator
Sun 17 Mar 2019 08:07:43 PM UTC, comment #10: 

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.

Eddy <count>
Sun 17 Mar 2019 02:29:17 PM UTC, comment #9: 

Aaah. Now I see. There is no such thing as a face with unclosed contour. But the list element for unclosed contours was missing.
What about the attached patch?

(file #46563)

Markus Mützel <mmuetzel>
Group administrator
Sun 17 Mar 2019 01:12:05 PM UTC, comment #8: 

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.

Eddy <count>
Sun 17 Mar 2019 12:53:42 PM UTC, comment #7: 

I pushed your changes to graphics.cc after adding a FIXME here:
http://hg.savannah.gnu.org/hgweb/octave/rev/ef922c0631e7

It might be better to first fix non-coplanar faces with unclosed contour before changing draw_patch in gl-render.cc. Maybe that change won't be necessary anymore? But maybe I am misunderstanding...

Markus Mützel <mmuetzel>
Group administrator
Sun 17 Mar 2019 11:18:21 AM UTC, comment #6: 

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)

Eddy <count>
Sat 16 Mar 2019 07:12:12 PM UTC, comment #5: 

Unclosed non-planar patches should work as well (not sure if they ever did).
As for the other issue: If it is really the same lines of code that needs to be changed, I'd say include it in the same patch. If it is a different bug, maybe two separate patches would be better. (You can still use the same bug report here if the issues are similar.)

Markus Mützel <mmuetzel>
Group administrator
Sat 16 Mar 2019 04:06:31 PM UTC, comment #4: 

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)

Eddy <count>
Thu 14 Mar 2019 10:29:15 PM UTC, comment #3: 

Regarding comment #1, attached is a plot from this code


v = [0 0 0; 1 0 0; 0 1 0; 0 0 1; nan nan nan];
f = 1:rows(v)
patch("vertices", v, "faces", f);
view (3)


made with octave-5.1.  The results are the same with Matlab.

Note that the polygon is not closed, because of the NaN.



Rik <rik5>
Group administrator
Thu 14 Mar 2019 08:42:47 PM UTC, comment #2: 

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.

Eddy <count>
Thu 14 Mar 2019 07:26:49 PM UTC, comment #1: 

I'm looking into the problem.

One thing I found is, the SVD based co-planar test somehow accept NaN as input (do not complain about it, in contrast to eig). So it is potentially a bug. e.g. this create a infinite loop.


%v = [0 0 0; 1 0 0; 0 1 0; 0 0 1; nan nan nan];
f = 1:rows(v)
patch("vertices", v, "faces", f);


To fix it, I need to make sure some detail of _go_patch_.

1. When the last vertex contains NaN, what is the expected result? Still a polygons (a closed loop) but with less edges? or a open loop?

2. If a open loop is expected (as what ver 5.1.0 shown), what should be the normal vector of that face? Especially what if the whole face is not in a plane. It is not a typical face actually.

Eddy <count>
Tue 12 Mar 2019 06:59:30 PM UTC, original submission:  

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




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 #46563:  bug55895_non_coplanar_mix_in.patch added by mmuetzel (3KiB - application/octet-stream)
file #46562:  bug55895_allow_nan_v2.patch added by count (9KiB - text/x-patch - patch fix both nan problem and mixed coplanar-noncoplanar faces problem)
file #46557:  bug55895_allow_nan.patch added by count (8KiB - text/x-patch - patch for fixing reported bug)
file #46540:  nanpatch.png added by rik5 (16KiB - image/png)
file #46506:  patch.var added by rik5 (4KiB - application/octet-stream)
file #46507:  tst_patch.m added by rik5 (545B - text/x-matlab)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by count (Posted a comment)
  • -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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-02-12 mtmiller Dependencies- bugs #57792 is dependent
    2019-12-10 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2019-03-17 mmuetzel Attached File- Added bug55895_non_coplanar_mix_in.patch, #46563
    2019-03-17 mmuetzel StatusConfirmed Ready For Test
    2019-03-17 count Attached File- Added bug55895_allow_nan_v2.patch, #46562
    2019-03-16 count Attached File- Added bug55895_allow_nan.patch, #46557
    2019-03-14 rik5 Attached File- Added nanpatch.png, #46540
    2019-03-12 rik5 Summarypatch() fails when NaN vertices exist _go_patch_ fails when NaN vertices exist
    2019-03-12 rik5 Attached File- Added patch.var, #46506
        Attached File- Added tst_patch.m, #46507

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code