[Octave-bug-tracker] [bug #55895] __go_patch__ fails when NaN vertices exist

[Eddy]

Follow-up Comment #10, bug #55895 (project octave):

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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?55895>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/