Hi devs,
I'm looking at Boolean operations (particularly Difference ops) on real-world SVG files again, and I've come across another bug, which I've reported here for posterity: https://bugs.launchpad.net/inkscape/+bug/1209281. This comes with a sample SVG file which might reproduce the bug. Note that I say "might" because it is reproduced on the current trunk on my x86 Ubuntu 12.04, but not on the latest Windows build I'm running (0.48.4).
I aim to have a look into what's going on here and hopefully come up with a patch - I've looked through the livarot code in a bit of detail now, and I think I understand at a basic level how the various path-intersection algorithms are implemented (particularly wrt quantization of coordinates and the implications for the sweep-line method).
Meanwhile, a couple of questions:
1) While I expect I'll be able to fix the current unstable behaviour (occasional copying of topmost path's coords into bottom-most path during difference operations, for too-small paths that "fall through" the quantization grid), the whole requirement for quantization in livarot is a bit unsatisfactory. Longer-term, it might be nice to switch to a path-intersection algorithm without the quantization restriction, and that deals better with degenerate cases - perhaps something like the CGAL 2D intersection methods ( http://www.cgal.org/Manual/beta/doc_html/cgal_manual/packages.html#part_VIII... I believe this part of CGAL is GPL licensed, is this an issue? Thoughts?
2) I keep coming across code in Inkscape that could be improved with the RAII pattern (particularly, things like:
{ Thing *t = new Thing(); ... do stuff with t ... delete t; }
When writing new code, I'd much prefer to use smart pointers, e.g.
{ std::unique_ptr<Thing> t(new Thing()); ... do stuff with t ... }
Obviously things like std::unique_ptr and std::shared_ptr require (part of) C++11. I note that parts of inkscape use boost::shared_ptr - is use of boost the way to go in this case, to keep support for older (non-C++11) compilers?
Thanks
Eric
Regarding CGal use, you may want to ask on the 2geom devel list about it (if no one answers here). http://lists.sourceforge.net/lists/listinfo/lib2geom-devel
Cheers, Josh
On Wed, Aug 7, 2013 at 8:43 AM, Eric Greveson <eric@...3003...> wrote:
Hi devs,
I'm looking at Boolean operations (particularly Difference ops) on real-world SVG files again, and I've come across another bug, which I've reported here for posterity: https://bugs.launchpad.net/inkscape/+bug/1209281. This comes with a sample SVG file which might reproduce the bug. Note that I say "might" because it is reproduced on the current trunk on my x86 Ubuntu 12.04, but not on the latest Windows build I'm running (0.48.4).
I aim to have a look into what's going on here and hopefully come up with a patch - I've looked through the livarot code in a bit of detail now, and I think I understand at a basic level how the various path-intersection algorithms are implemented (particularly wrt quantization of coordinates and the implications for the sweep-line method).
Meanwhile, a couple of questions:
- While I expect I'll be able to fix the current unstable behaviour
(occasional copying of topmost path's coords into bottom-most path during difference operations, for too-small paths that "fall through" the quantization grid), the whole requirement for quantization in livarot is a bit unsatisfactory. Longer-term, it might be nice to switch to a path-intersection algorithm without the quantization restriction, and that deals better with degenerate cases - perhaps something like the CGAL 2D intersection methods (http://www.cgal.org/Manual/beta/doc_html/cgal_manual/packages.html#part_VIII... I believe this part of CGAL is GPL licensed, is this an issue? Thoughts?
- I keep coming across code in Inkscape that could be improved with the
RAII pattern (particularly, things like:
{ Thing *t = new Thing(); ... do stuff with t ... delete t; }
When writing new code, I'd much prefer to use smart pointers, e.g.
{ std::unique_ptr<Thing> t(new Thing()); ... do stuff with t ... }
Obviously things like std::unique_ptr and std::shared_ptr require (part of) C++11. I note that parts of inkscape use boost::shared_ptr - is use of boost the way to go in this case, to keep support for older (non-C++11) compilers?
Thanks
Eric
Get 100% visibility into Java/.NET code with AppDynamics Lite! It's a free troubleshooting tool designed for production. Get down to code-level detail for bottlenecks, with <2% overhead. Download for free and get started troubleshooting in minutes. http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clk... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
2013/8/7 Eric Greveson <eric@...3003...>:
- While I expect I'll be able to fix the current unstable behaviour
(occasional copying of topmost path's coords into bottom-most path during difference operations, for too-small paths that "fall through" the quantization grid), the whole requirement for quantization in livarot is a bit unsatisfactory. Longer-term, it might be nice to switch to a path-intersection algorithm without the quantization restriction, and that deals better with degenerate cases - perhaps something like the CGAL 2D intersection methods (http://www.cgal.org/Manual/beta/doc_html/cgal_manual/packages.html#part_VIII... I believe this part of CGAL is GPL licensed, is this an issue? Thoughts?
The main issues with CGAL are: 1. In many distributions, it has a hard dependency on Boost.Thread and Qt - we do not want these as dependencies. 2. It is written in a super-generic style and rather hard to use.
The best solution would be to fork the parts of CGAL that implement the boolops algorithms and integrate them within 2Geom.
When writing new code, I'd much prefer to use smart pointers, e.g.
{ std::unique_ptr<Thing> t(new Thing()); ... do stuff with t ... }
Obviously things like std::unique_ptr and std::shared_ptr require (part of) C++11. I note that parts of inkscape use boost::shared_ptr - is use of boost the way to go in this case, to keep support for older (non-C++11) compilers?
I think the use of C++11 was not discussed yet. It would definitely simplify a lot of things, though compiler support is not yet universal.
In 95% of cases you can use std::auto_ptr or boost::scoped_ptr instead. All header-only parts of Boost are already an accepted dependency. The only differences are the behavior when copying: 1) boost::scoped_ptr is noncopyable, 2) std::auto_ptr always transfers pointer ownership to the copy, 3) std::unique_ptr is noncopyable, but has an rvalue reference constructor that transfers ownership (so e.g. you can put it in a container).
Regards, Krzysztof
On 7-8-2013 17:43, Eric Greveson wrote:
- I keep coming across code in Inkscape that could be improved with
the RAII pattern (particularly, things like:
{ Thing *t = new Thing(); ... do stuff with t ... delete t; }
When writing new code, I'd much prefer to use smart pointers, e.g.
{ std::unique_ptr<Thing> t(new Thing()); ... do stuff with t ... }
Can't comment too smartly on this but... How about { Thing t; ... do stuff with t... }
;-)
Cheers, Johan
2013/8/8 Johan Engelen <jbc.engelen@...2592...>:
Can't comment too smartly on this but... How about { Thing t; ... do stuff with t... }
;-)
+1
If the constructor can be called directly, and the object is deleted at the end of the function, using a stack variable is easier and faster than using a smart pointer.
Regards, Krzysztof
On Aug 8, 2013, at 12:37 PM, Johan Engelen wrote:
On 7-8-2013 17:43, Eric Greveson wrote:
- I keep coming across code in Inkscape that could be improved with
the RAII pattern (particularly, things like:
{ Thing *t = new Thing(); ... do stuff with t ... delete t; }
When writing new code, I'd much prefer to use smart pointers, e.g.
{ std::unique_ptr<Thing> t(new Thing()); ... do stuff with t ... }
Can't comment too smartly on this but... How about { Thing t; ... do stuff with t... }
;-)
Ah, exactly.
That's the preferred, simpler solution. Avoid adding pointers whenever possible. With C++ it's possible far more often.
participants (5)
-
Eric Greveson -
Johan Engelen -
Jon Cruz -
Josh Andler -
Krzysztof Kosiński