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

 
 

bug #55895: __go_patch__ fails when NaN vertices exist

Submitted by:  Rik <rik5>
Submitted on:  Tue 12 Mar 2019 06:59:30 PM UTC  
 
Category:  Plotting Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Ready For Test Assigned to:  None
Originator Name:  Open/Closed:  Open
Release:  dev Operating 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)

Thu 21 Mar 2019 06:24:15 PM UTC, comment #12:

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

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

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:

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.

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

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:

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

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

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

(file #46540)

Rik <rik5>
Project 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.

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:

Rik <rik5>
Project 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

Items that depend on this one: None found

 

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)
  •  

    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 project members can vote.

     

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    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.4