Use of Floating Point equality comparison
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Inkscape |
Fix Released
|
Medium
|
Jaspervdg |
Bug Description
Around line 956 of display/curve.cpp
http://
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_
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://
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://
Changed in inkscape: | |
status: | New → Confirmed |
tags: | added: code-design |
Changed in inkscape: | |
assignee: | nobody → Jaspervdg (jaspervdg) |
milestone: | none → 0.47 |
status: | Confirmed → Fix Released |
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).