lib2geom-devel — development discussion

Re: [Lib2geom-devel] C-arrays

[Nathan H. <nj...@nj...>]

I 100% support your position, but having been down this path in the
past I strongly advise you to take care:

On Wed, Nov 06, 2013 at 05:11:33PM +0100, jbc...@sw... wrote:
> ---- Nathan Hurst <nj...@nj...> wrote: 
> > I'm around, not much time though.  Is there a real benefit to porting
> > bezier-utils to vector?  I did it once before and it was a lot of work
> > and lead to some subtle bugs.  What benefit do you see?
> > 
> > I would rather we focused on careful profiling and optimisation.
> 
> The main gains I see are: exception safety / memleaks, crash safety, and maintainability/readability. Note: at zero performance cost.

But that code has been used for a decade now with no evidence of
problems, I would be loath to change it purely for stylistic reasons.
When I tried to do this, I remember that it uncovered some
mathematically unusual operations which were hard to express in
2geom.  As they were heuristics and not useful in general adding them
to say D2<> would have been ugly.  Without them the code was harder to
understand and slower.

> Yes, we can work on performance optimization, but I think that's
> pointless for code that is in a "bad" state. Note that vectors do
> not make code slow at all

That is not true in my experience.  Sure, there is negligible overhead
for the access of the vector vs array, but there is definitely
measurable overhead in the more complicated allocator.

as far as bad is concerned, I would rather we worked on improving the
api and numerical guarantees for other parts of the code.  What are
the main reasons that we haven't completely moved to 2geom in
inkscape?  Does someone want to take on boolops again?  Even porting
the livarot boolops to 2geom notation would be beneficial.

Another direction we could go is switching to cgal for the topology
operators.  Now that cgal has a compatible licence we need to
reconsider it.  The problems we had in the past (IIRC) where the
licence, performance and accuracy.  It is likely that these are all no
longer issues.

>, and when we better integrate this
> particular piece of code using the 'standard' 2geom types, it might
> have the side effect of making things a little bit faster because of
> less conversion between data structures. So my plan was: C++ify
> code, fix the bugs that show up, see what can be improved
> performance-wise and interface-wise.

That seems backwards - first profile and find the problems, then fix
the code.

> By the way, a big reason for me to switch to C++11 is to see if it
> would improve performance (e.g. move semantics) while maintaining
> our high-level of abstraction. I guess we can start writing
> performance tests/toys, to see if the changes really bring
> anything. The new C++11 <chrono> library seems to provide portable
> high-precision clock... I have not tested it yet though.

We can use C++11 in 2geom for tests and leave the library itself in C++99

njh

Re: [Lib2geom-devel] C-arrays

[<jbc...@sw...>]

---- Nathan Hurst <nj...@nj...> wrote: 
> I'm around, not much time though.  Is there a real benefit to porting
> bezier-utils to vector?  I did it once before and it was a lot of work
> and lead to some subtle bugs.  What benefit do you see?
> 
> I would rather we focused on careful profiling and optimisation.

The main gains I see are: exception safety / memleaks, crash safety, and maintainability/readability. Note: at zero performance cost.

Yes, we can work on performance optimization, but I think that's pointless for code that is in a "bad" state. Note that vectors do not make code slow at all, and when we better integrate this particular piece of code using the 'standard' 2geom types, it might have the side effect of making things a little bit faster because of less conversion between data structures. So my plan was: C++ify code, fix the bugs that show up, see what can be improved performance-wise and interface-wise.

By the way, a big reason for me to switch to C++11 is to see if it would improve performance (e.g. move semantics) while maintaining our high-level of abstraction. I guess we can start writing performance tests/toys, to see if the changes really bring anything. The new C++11 <chrono> library seems to provide portable high-precision clock... I have not tested it yet though.

regards,
  Johan

Re: [Lib2geom-devel] C-arrays

[Nathan H. <nj...@nj...>]

I'm around, not much time though.  Is there a real benefit to porting
bezier-utils to vector?  I did it once before and it was a lot of work
and lead to some subtle bugs.  What benefit do you see?

I would rather we focused on careful profiling and optimisation.

njh

On Tue, Nov 05, 2013 at 11:48:40PM +0100, Johan Engelen wrote:
> Hi all,
>    I just had a look at bezier-utils.cpp and was shocked to see such an 
> amount of C-array usage. (even newing/deleting them, etc). Converting 
> this to std::vector is going to be quite some task. :(
> But it will be worth it :-)
> 
> How many people are still somewhat active here? Could we schedule a 
> happy online get-together where we are C++ing the code together? More 
> fun that way.
> I think 2geom deserves to be better ;)
> 
> Cheers,
>    Johan
> 
> 
> 
> 
> ------------------------------------------------------------------------------
> November Webinars for C, C++, Fortran Developers
> Accelerate application performance with scalable programming models. Explore
> techniques for threading, error checking, porting, and tuning. Get the most 
> from the latest Intel processors and coprocessors. See abstracts and register
> http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk
> _______________________________________________
> Lib2geom-devel mailing list
> Lib...@li...
> https://lists.sourceforge.net/lists/listinfo/lib2geom-devel

Re: [Lib2geom-devel] C-arrays

[J. E. <jbc...@sw...>]

Yep, I only meant std::vector as an example. Indeed, that code should use much more of the types already present in 2geom.

- Johan

----- Reply message -----
From: "Krzysztof Kosiński" <twe...@gm...>
To: "Johan Engelen" <jbc...@sw...>
Cc: "Lib2geom-Devel" <lib...@li...>
Subject: [Lib2geom-devel] C-arrays
Date: Wed, Nov 6, 2013 00:46


2013/11/5 Johan Engelen <jbc...@sw...>:
> Hi all,
>    I just had a look at bezier-utils.cpp and was shocked to see such an
> amount of C-array usage. (even newing/deleting them, etc). Converting
> this to std::vector is going to be quite some task. :(
> But it will be worth it :-)

What about using Geom::BezierCurve instead of std::vector where applicable?

Regards, Krzysztof

Re: [Lib2geom-devel] C-arrays

[Krzysztof K. <twe...@gm...>]

2013/11/5 Johan Engelen <jbc...@sw...>:
> Hi all,
>    I just had a look at bezier-utils.cpp and was shocked to see such an
> amount of C-array usage. (even newing/deleting them, etc). Converting
> this to std::vector is going to be quite some task. :(
> But it will be worth it :-)

What about using Geom::BezierCurve instead of std::vector where applicable?

Regards, Krzysztof

[Lib2geom-devel] C-arrays

[Johan E. <jbc...@sw...>]

Hi all,
   I just had a look at bezier-utils.cpp and was shocked to see such an 
amount of C-array usage. (even newing/deleting them, etc). Converting 
this to std::vector is going to be quite some task. :(
But it will be worth it :-)

How many people are still somewhat active here? Could we schedule a 
happy online get-together where we are C++ing the code together? More 
fun that way.
I think 2geom deserves to be better ;)

Cheers,
   Johan

[Lib2geom-devel] Warning

[Johan E. <jbc...@sw...>]

Hi all,
   Inkscape hits on this warning:

src/2geom/generic-interval.h:65:9: warning: assuming signed overflow 
does not occur when assuming that (X + c) >= X is always true 
[-Wstrict-overflow]

It is caused by this line of code:
    Geom::IntRect area = 
Geom::IntRect::from_xywh(floor(event->button.x), floor(event->button.y), 
1, 1);

Relevant code snippets from our trunk:
generic-rect.h:
     static CRect from_xywh(C x, C y, C w, C h) {
         CPoint xy(x, y);
         CPoint wh(w, h);
         CRect result(xy, xy + wh);
         return result;
     }

generic-interval.h:
     GenericInterval(C u, C v) {
         if (u <= v) {           <------ this is line 65 that triggers 
the warning
             _b[0] = u; _b[1] = v;
         } else {
             _b[0] = v; _b[1] = u;
         }
     }

I believe the warning is valid for the optimization that GCC will 
perform. (actually without optimization, I read that the result is 
undefined when overflow would have occured)
How do we kill that warning?

Thanks,
   Johan

Re: [Lib2geom-devel] C++11

[Josh A. <sc...@gm...>]

On Tue, Oct 22, 2013 at 10:58 AM, Johan Engelen
<jbc...@sw...> wrote:
> On 22-10-2013 2:07, Nathan Hurst wrote:
>> Most of us do, but given our main customer is inkscape, we need to
>> wait for them (us) I think?

Isn't Scribus also a user? I'm not sure to what extent, but I seem to
recall that they are.

> I was hoping we are bold enough to make the switch earlier, have the fun
> earlier, and hopefully drag them (us) with us.
> The "syncs" between Inkscape and 2geom are not as common as they once
> were, making it less of a problem I feel.

Most changes are brought over by either yourself or Krzysztof these
days. To me it seems like if there are tangible benefits such as
better performance, simplified code, etc it might whet the appetite
more in the Inkscape community. Likewise, if there aren't any
substantive changes being actively worked on in 2geom that Inkscape
would really want (boolops would be an example), there really is no
cause for concern imho.

Note: I'm a REALLY bad person to chime in on this as I tend to always
run current distros with up-to-date compilers and wouldn't be
negatively affected.

Cheers,
Josh

Re: [Lib2geom-devel] C++11

[Johan E. <jbc...@sw...>]

On 22-10-2013 2:07, Nathan Hurst wrote:
> On Tue, Oct 22, 2013 at 12:05:40AM +0200, Johan Engelen wrote:
>> Hi all,
>>     With 2geom's heavy use of C++ features, it's pretty obvious that
>> C++11 brings huge improvements for lib2geom. When will we make the switch?
>>
>> I'd like to make the switch ASAP.
> Most of us do, but given our main customer is inkscape, we need to
> wait for them (us) I think?

I was hoping we are bold enough to make the switch earlier, have the fun 
earlier, and hopefully drag them (us) with us.
The "syncs" between Inkscape and 2geom are not as common as they once 
were, making it less of a problem I feel.

Cheers,
   Johan

Re: [Lib2geom-devel] C++11

[Nathan H. <nj...@nj...>]

On Tue, Oct 22, 2013 at 12:05:40AM +0200, Johan Engelen wrote:
> Hi all,
>    With 2geom's heavy use of C++ features, it's pretty obvious that 
> C++11 brings huge improvements for lib2geom. When will we make the switch?
> 
> I'd like to make the switch ASAP.

Most of us do, but given our main customer is inkscape, we need to
wait for them (us) I think?

njh

[Lib2geom-devel] C++11

[Johan E. <jbc...@sw...>]

Hi all,
   With 2geom's heavy use of C++ features, it's pretty obvious that 
C++11 brings huge improvements for lib2geom. When will we make the switch?

I'd like to make the switch ASAP.

Cheers,
   Johan

Re: [Lib2geom-devel] Point::isZero

[Krzysztof K. <twe...@gm...>]

2013/9/28 Johan Engelen <jbc...@sw...>:
> Hi all,
>    Point::isZero is using float point comparison instead of are_near.
> Any objections against changing this to are_near?
> The new code:
>      bool isZero() const {
>          return ( are_near(_pt[X],0) && are_near(_pt[Y],0) );
>      }

1. The method is called isZero() rather than isNearZero() for a
reason. There are cases when checking for exact equality is the right
thing to do.
2. Every method that tests for nearness must take an epsilon
parameter. You must never call are_near with the default epsilon
parameter within lib2geom.

Regards, Krzysztof

Re: [Lib2geom-devel] Point::isZero

[Nathan H. <nj...@nj...>]

On Sat, Sep 28, 2013 at 10:28:08PM +0200, Johan Engelen wrote:
> Hi all,
>    Point::isZero is using float point comparison instead of are_near.
> Any objections against changing this to are_near?
> The new code:
>      bool isZero() const {
>          return ( are_near(_pt[X],0) && are_near(_pt[Y],0) );
>      }

I think isZero should test for zero, but I'm happy for you to change
code to use pt.areNear(Point()) or add nearZero() or something

njh

[Lib2geom-devel] Point::isZero

[Johan E. <jbc...@sw...>]

Hi all,
   Point::isZero is using float point comparison instead of are_near.
Any objections against changing this to are_near?
The new code:
     bool isZero() const {
         return ( are_near(_pt[X],0) && are_near(_pt[Y],0) );
     }

Cheers,
   Johan

Re: [Lib2geom-devel] Generate SVG paths from Geom::PathVector

[Vinícius d. S. O. <vin...@gm...>]

Em Qui, 2013-08-15 às 19:20 +0200, Krzysztof Kosiński escreveu:

> > class SVGPathSink {
> > public:
> >     // ...
> >
> >     // This function will read \p svgpath and make some calls to the
> virtual methods
> >     // moveTo, hlineTo, vlineTo...
> >     void read(const Geom::PathVector &svgpath);
> >
> > }
> 
> Yes, this is what I had in mind, though a better name would be
> "process" or "sink". This method should be virtual, but unlike the
> other virtual methods in this lass it should have a default
> implementation defined.
> 
> > 2. Derive from SVGPathSink and implement SVGPathDataBuilder, a class
> > based on the code in src/svg/path-string.cpp.
> >
> >
> > I'll start from this.
> >
> > The name DataBuilder is a bit generic. What do you think about
> SVGPathTextBuilder? The name clearly exposes the intent.
> 
> "Path data" is the name used for the command string in the SVG
> specification, so I would recommend sticking to that. "Path text" does
> not appear in the specification. You could also use
> "SVGPathCommandStringBuilder", but that's a little too long. One
> possible way to disambiguate is to remove the "SVG" prefix on
> SVGPathSink, because it doesn't have any SVG-specific functionality.
> (I can handle that since it involves regenerating svg-path-builder.cpp
> using Ragel.)


I agree with choices of names now, but this design is more complicated
(for the implementer, not the user) than I initially expected. I'll use
a private copy of the implementation with "poor" and not-reasoned-enough
interface in libdepixelize and continue to do my GSoC tasks. After I
finish my GSoC proposal, I'll come back to contribute to lib2geom and to
change libdepixelize svg generation "backend".


-- 
Vinícius dos Santos Oliveira
https://about.me/vinipsmaker

Re: [Lib2geom-devel] Generate SVG paths from Geom::PathVector

[Krzysztof K. <twe...@gm...>]

2013/8/15 Vinícius dos Santos Oliveira <vin...@gm...>
> Should Inkscape::SVG::PathString become a wrapper around the Geom::PathSink... like SPCurve is a wrapper around Geom::PathVector?

More or less. Ideally the only specific functionality of
Inkscape::SVG::PathString would be to read Inkscape preferences and
set the path data output parameters based on them.

> class SVGPathSink {
> public:
>     // ...
>
>     // This function will read \p svgpath and make some calls to the virtual methods
>     // moveTo, hlineTo, vlineTo...
>     void read(const Geom::PathVector &svgpath);
>
> }

Yes, this is what I had in mind, though a better name would be
"process" or "sink". This method should be virtual, but unlike the
other virtual methods in this lass it should have a default
implementation defined.

> 2. Derive from SVGPathSink and implement SVGPathDataBuilder, a class
> based on the code in src/svg/path-string.cpp.
>
>
> I'll start from this.
>
> The name DataBuilder is a bit generic. What do you think about SVGPathTextBuilder? The name clearly exposes the intent.

"Path data" is the name used for the command string in the SVG
specification, so I would recommend sticking to that. "Path text" does
not appear in the specification. You could also use
"SVGPathCommandStringBuilder", but that's a little too long. One
possible way to disambiguate is to remove the "SVG" prefix on
SVGPathSink, because it doesn't have any SVG-specific functionality.
(I can handle that since it involves regenerating svg-path-builder.cpp
using Ragel.)

Regards, Krzysztof

Re: [Lib2geom-devel] Generate SVG paths from Geom::PathVector

[Vinícius d. S. O. <vin...@gm...>]

Em Qui, 2013-08-15 às 02:48 +0200, Krzysztof Kosiński escreveu:

> 2013/8/13 Vinícius dos Santos Oliveira <vin...@gm...>
> > Anyway, what I need is to do the other way back (generate a SVG file
> from some Geom::PathVector). The only code I found related to this
> functionality lies in the toys (not part of the lib) and uses a
> complicated scheme involving cairo and a lot of virtual func calls. Is
> there another way? If there is not, I want to implement it and I want
> to implement it fast. Would be possible to release another lib2geom
> version fast if code goes okay?
> 
> This functionality is already present in Inkscape, see
> src/svg/path-string.cpp. However, it would be a good idea to move it
> into 2Geom. Here is the optimal plan:


Should Inkscape::SVG::PathString become a wrapper around the
Geom::PathSink... like SPCurve is a wrapper around Geom::PathVector?


> 1. Make a method on SVGPathSink to output a PathVector into it. You
> can copy the relevant code with some modification from Inkscape, see
> src/svg/svg-path.cpp, or generalize the output_svg_path function.


I'm not sure if I understood what you mean. What I understood:

class SVGPathSink {
public:
    // ...

    // This function will read \p svgpath and make some calls to the
virtual methods
    // moveTo, hlineTo, vlineTo...
    void read(const Geom::PathVector &svgpath);
}


> 2. Derive from SVGPathSink and implement SVGPathDataBuilder, a class
> based on the code in src/svg/path-string.cpp.


I'll start from this.

The name DataBuilder is a bit generic. What do you think about
SVGPathTextBuilder? The name clearly exposes the intent.


-- 
Vinícius dos Santos Oliveira
https://about.me/vinipsmaker

Re: [Lib2geom-devel] Generate SVG paths from Geom::PathVector

[Vinícius d. S. O. <vin...@gm...>]

Em Ter, 2013-08-13 às 21:23 -0300, Vinícius dos Santos Oliveira
escreveu:

> My tests ran fine and I don't think I'll have much trouble, but
> lib2geom needs some love as a "standalone" installed library. When I
> tried to compile-and-link my tests to a system-installed-dynamic-lib
> 2geom, there where some include files errors due to missing installed
> files (numeric folder). I fixed this error locally, but I'll share the
> results soon.


This issue was already fixed in latest VCS code.

Then all I need to code/share is the PathVector::toSVG() code.


-- 
Vinícius dos Santos Oliveira
https://about.me/vinipsmaker

Re: [Lib2geom-devel] Build issues

[Vinícius d. S. O. <vin...@gm...>]

Em Ter, 2013-08-13 às 03:11 -0300, Vinícius dos Santos Oliveira
escreveu:

> I've had some build issues when I tried to compile latest stable
> version of lib2geom on my ArchLinux system (ArchLinux means latest
> version of every lib and compiler and no patched packages, if
> possible).


I've checked latest VCS code and it was already fixed. No changes
required (just a new version release).


-- 
Vinícius dos Santos Oliveira
https://about.me/vinipsmaker

Re: [Lib2geom-devel] rendering arcs - Bug 653315

[Alvin P. <pe...@va...>]

thanks, much appreciated!

Alvin Penner

Re: [Lib2geom-devel] rendering arcs - Bug 653315

[Krzysztof K. <twe...@gm...>]

2013/8/14 Alvin Penner <pe...@va...>:
> The issue of converting from sbasis to bezier has been raised again in the
> thread:
> http://inkscape.13.x6.nabble.com/quot-Edit-paths-by-nodes-quot-uses-different-and-wrong-approximation-of-arc-as-bezier-curve-td4967659.html
> In response to that, I have posted an updated version of
> sbasis-to-bezier.cpp and .h. These are compatible with current Inkscape
> trunk and appear to resolve the rendering problem mentioned in that thread.

I committed this and also gave you commit access to lib2geom.

Regards, Krzysztof

Re: [Lib2geom-devel] Generate SVG paths from Geom::PathVector

[Krzysztof K. <twe...@gm...>]

2013/8/13 Vinícius dos Santos Oliveira <vin...@gm...>
> Anyway, what I need is to do the other way back (generate a SVG file from some Geom::PathVector). The only code I found related to this functionality lies in the toys (not part of the lib) and uses a complicated scheme involving cairo and a lot of virtual func calls. Is there another way? If there is not, I want to implement it and I want to implement it fast. Would be possible to release another lib2geom version fast if code goes okay?

This functionality is already present in Inkscape, see
src/svg/path-string.cpp. However, it would be a good idea to move it
into 2Geom. Here is the optimal plan:

1. Make a method on SVGPathSink to output a PathVector into it. You
can copy the relevant code with some modification from Inkscape, see
src/svg/svg-path.cpp, or generalize the output_svg_path function.

2. Derive from SVGPathSink and implement SVGPathDataBuilder, a class
based on the code in src/svg/path-string.cpp.

Regards, Krzysztof

Re: [Lib2geom-devel] Generate SVG paths from Geom::PathVector

[Vinícius d. S. O. <vin...@gm...>]

Em Qui, 2013-08-15 às 02:11 +0200, Krzysztof Kosiński escreveu:

> I can add Vinicius to the hackers team. Vinicius, please give me your
> Launchpad login or propose yourself as a member on this page:
> https://launchpad.net/~lib2geom-hackers


https://launchpad.net/~vinipsmaker


-- 
Vinícius dos Santos Oliveira
https://about.me/vinipsmaker

Re: [Lib2geom-devel] Generate SVG paths from Geom::PathVector

[Krzysztof K. <twe...@gm...>]

2013/8/13 Nathan Hurst <nj...@nj...>:
>> Would be possible to release another lib2geom version
>> fast if code goes okay?
>
> Absolutely,  indeed I think we should just give your write access to
> the repo (given you're already an inkscape dev).  Who remembers how to
> do this?

I can add Vinicius to the hackers team. Vinicius, please give me your
Launchpad login or propose yourself as a member on this page:
https://launchpad.net/~lib2geom-hackers

Regards, Krzysztof

Re: [Lib2geom-devel] rendering arcs - Bug 653315

[Alvin P. <pe...@va...>]

The issue of converting from sbasis to bezier has been raised again 
in the thread:
http://inkscape.13.x6.nabble.com/quot-Edit-paths-by-nodes-quot-uses-different-and-wrong-approximation-of-arc-as-bezier-curve-td4967659.html
In response to that, I have posted an updated version of 
sbasis-to-bezier.cpp and .h. These are compatible with current 
Inkscape trunk and appear to resolve the rendering problem mentioned 
in that thread.

any comments would be welcome,
Alvin Penner