Use of Floating Point equality comparison

[Bpfowler]

Around line 956 of display/curve.cpp
http://www.inkscape.org/doc/doxygen/html/display_2curve_8cpp.php#a4
we use != to check that two points are not the same.

This should almost certainly certainly be replaced by a
comparison in the form:
    fabs(bpath->x3 - bpath[i-1].x3) > tolerance

Obviously, I can't recommend such a change during a hard
freeze. but if there is someone here who feels that this
could be important, would it possible for this to be
picked up,
for later.

Less obviously:
    1. x3 and the other members of NArtBpath should be
private
    2. NArtBpath itself should perhaps look more like a
first
class C++ class with constructors and a C++ source file.
    3. Functions such as this one ( static NArtBpath
const * sp_bpath_check_subpath (NArtBpath const
bpath[]) ) which
look like NArtBpath member functions (take a pointer to
a bpath, et cetera) ought perhaps to be in the class
and file
above.
    4. If I am on the right lines then documentation for
NArtBpath should refer to primary reference material such
as http://www.w3.org/TR/SVG/paths.html
     5. Possibly, rather than dummying up an array of
floats using an ad hoc syntax: x1,y1 ..., to hold the
path's element's geometry ['element' = member of an
array, not a part of an XML doxument - it is another word
that like 'path' gets easily overloaded), we could use
three NR:Points (this might be an RFE for milestone 0.44),
which I assume hold two control points and an end point
(I have not seen this documented), the start point's being
assumed.
    6a. Similalrly, NRArtBpath is a candidate for becoming
a pure abstract class, with a subclass for each type of
path segment (commands M,C,z et cetera); though this
may be wrong for some reason that I have not yet
discovered.
    6b The use of the NRPathcode enum to hold the
SVG path command letter of each command (array
element as per point 5. This appears to be the cause
of using numerous switch statements rather than
polymorphism and a roccocco method for determining
whether a command is of type MOVETO ...

 ... I'd better stop nw.

I am assumimg that I have missed something, in the sense
that neither Inkscape nor Sodipodi would be lilely to reach
maturity without good data structures,

    "Show me your flow charts and conceal your tables
and I
    shall continue to be mystified, show me your tables
and I
    won't usually need your flow charts; they'll be
obvious. "

exempli gratia
http://newton.ex.ac.uk/teaching/resources/jmr/structures.html

[Buliabyak-users (buliabyak-users)]

This class is very old, I think even from libart, not
sodipodi. I don't think it's worthwhile to C++fy it just for
the hell of it. Instead the livarot/ contains the Path class
which it uses for everything. It's much richer and more
advanced. If you want to do something interesting with
paths, just use it.

As for float comparison, I think you are right. Feel free to
patch it (after the release).

[Bpfowler (bpfowler)]

Yes, it is from libart.

Yes, I would not be proposing to convert something to C++ just
for fun.

I would guess that most of the source to Inkscape is somewhere
on a spectrum between 'well-written' and 'in need of a
re-write';
whilst working code can be excused scrutiny, surely if a module
is so little needed that it is not worthwhile to re-write it for
consistency or to bump it along my spectrum then we should
be thinking about removing it altogether.

Is it intended to deprecate class NArtBpath in favour of
class Path?

There is another floating point equality at
svg/gnome-canvas-bpath-util.cpp at line 151

I have this method for the NR::Point class,
which tests whether two NR::Points are near each other:
    bool fNear( NR::Point pt, gdouble t) const {
        return ( fabs( _pt[X] - pt[X] ) <= t && fabs( _pt[Y]
- pt[Y] ) <= t );
    }

[Bryce Harrington (bryce)]

Originator: NO

Bumping this bug a bit. It would be nice to eliminate any remaining
libart bits. We'll be converting to Cairo eventually, and having libart
exised from the codebase could help simplify that work.

[Jaspervdg (jaspervdg)]

Wish I found this earlier :) I encountered it while trying to fix another problem (see bug #216152).

To "solve" this I decided to simply remove sp_bpath_good, sp_bpath_clean and sp_bpath_check_subpath (in SVN). The latter is only used by the other two and is the one performing the actual checks. The other two are only used by sp_curve_new_from_foreign_bpath. This function is only used with pre-defined paths, from sp_curve_copy and from sp_curve_new_from_bpath. The first two uses shouldn't need the checks, the latter is used in the following cases:
 - Directly following sp_svg_read_path. As far as I know this (now) produces valid paths, and if it doesn't it should be fixed.
 - In path-chemistry.cpp it is used on a path that is a transformed version of an existing curve. As far as sp_bpath_check_subpath is concerned this curve/path should already be okay.
 - In sp-ellipse.cpp it is used on path data that was generated. This should also not need these kinds of sanity checks.
 - In libnrtype/Layout-TNG-Output.cpp it is used on path data that was generated based on a glyph from a font (which is ultimately generated by Path::MakeArtBPath in livarot/PathCutting.cpp as far as I can see). I expect that this should not need any sanity checks (if it does generate invalid paths something should probably be fixed).

Furthermore, all the sp_curve_moveto/lineto/etc. functions also seem pretty water-tight, so as far as I can see there is no point in performing these checks (anymore?). I also tried loading a few files, tested path combine and tried drawing some text. As far as I can see everything is indeed still functioning just fine.

I did find however that sp_curve_closepath uses the same dubious equality comparison (and perhaps other functions make the same mistake). It would be good to find more of these cases and make sure they use more suitable comparisons.

[Rygle (rygle)]

Jaspervdg, sounds like you've got a long way with this. Can you do a patch for SVN for others to test? Thanks.

[Jaspervdg (jaspervdg)]

It's already in SVN. So far it doesn't seem to have caused any problems, except that it exposed a bug in the SVG path data parser.

[Beluga (buovjaga)]

Is there anything further that has to be done or can this be closed as fixed?

[Jaspervdg (jaspervdg)]

I would be inclined to say that it can be closed as fixed. It might be possible to find more instances of the same problem, but if no one found a need in the last eight years...