Re: [Inkscape-devel] [cairo] Fwd: cairo path rendering bug with inkscape
On Sat, 05 Apr 2008 06:26:30 -0700, Carl Worth wrote:
So Bulia, I'm throwing the ball back into your court. It appears there's a bug in inkscape that is converting some C path elements into cairo_move_to instead of cairo_curve_to.
I haven't tried chasing this down into inkscape yet.
It was easy enough to see where inkscape is using cairo_move_to instead of cairo_curve_to:
In inkscape-cairo.cpp:112:
if (!optimize_stroke || swept.intersects(view)) cairo_curve_to (ct, tm1[NR::X], tm1[NR::Y], tm2[NR::X], tm2[NR::Y], tm3[NR::X], tm3[NR::Y]); else cairo_move_to(ct, tm3[NR::X], tm3[NR::Y]);
And sure enough, eliminating that optimization, (as with the below patch), does make the bug go away. But that's just a band-aid really, and doesn't tell us what's wrong with the optimization, (since it should be a reasonable thing to discard curves that are not visible).
I'll look a little closer now.
-Carl
PS. I guess since we're in inkscape's code now, I'll move the discussion to the inkscape-devel list. Followups need not include the cairo list anymore.
From ea87c392609cf6ad931bf64b0be35ac6a7233977 Mon Sep 17 00:00:00 2001
From: Carl Worth <cworth@...573...> Date: Sat, 5 Apr 2008 06:48:45 -0700 Subject: [PATCH] Bandaid to avoid incorrect curve optimization
I'm not sure what's wrong with the optimization here, but removing it definitely clears up bugs. Look at this file with the cairo-based outline mode both with and without the optimization to see:
http://cairographics.org/~cworth/images/cairobug.svg
(You may need to zoom in a bit to see the bug appear.) --- src/display/inkscape-cairo.cpp | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/src/display/inkscape-cairo.cpp b/src/display/inkscape-cairo.cpp index 5d87d39..a32cd13 100644 --- a/src/display/inkscape-cairo.cpp +++ b/src/display/inkscape-cairo.cpp @@ -109,10 +109,7 @@ feed_curve_to_cairo (cairo_t *ct, NArtBpath *bpath, NR::Matrix trans, NR::Maybe< tm1 -= shift; tm2 -= shift; tm3 -= shift; - if (!optimize_stroke || swept.intersects(view)) - cairo_curve_to (ct, tm1[NR::X], tm1[NR::Y], tm2[NR::X], tm2[NR::Y], tm3[NR::X], tm3[NR::Y]); - else - cairo_move_to(ct, tm3[NR::X], tm3[NR::Y]); + cairo_curve_to (ct, tm1[NR::X], tm1[NR::Y], tm2[NR::X], tm2[NR::Y], tm3[NR::X], tm3[NR::Y]); break; }
On Sat, Apr 5, 2008 at 9:54 AM, Carl Worth <cworth@...573...> wrote:
It was easy enough to see where inkscape is using cairo_move_to instead of cairo_curve_to:
Thanks for investigating this Carl, I looked into that optimization and tested it and I think it works as designed, at least for that file. Note that Inkscape draws screen in (usually horizontal) strips, so the entire feed_curve_to_cairo is called once for each strip. And in this file, it so happens that the logo sits on the boundary between the strips. So for one strip, some of the curves are replaced by movetos, and for the next strip, some others. Some curves overlap both strips and will be drawn twice, some only once. I don't see what might go wrong here.
I then wondered how replacing a curveto with a moveto in any path may result in drawing a _line_ which was _not in the path_. But then I remembered about the close_path command, z. If some curves preceding it were replaced by movetos, it will indeed draw a line from the last stroked point which will be wrong. But then the question is, is it correct to draw a closepath command from the last stroked point, not from the current point? If closepath would work from the current point, as set by movetos, there would be no bug here.
OK, while I'm still not sure about the correct behavior of close_path, I just eliminated it by replacing it with a lineto to the remembered start-of-path point, and that got rid of the bug. Thanks for helping to figure this out, and sorry for bothering you with this :)
On Sat, Apr 5, 2008 at 2:40 PM, bulia byak <buliabyak@...400...> wrote:
On Sat, Apr 5, 2008 at 9:54 AM, Carl Worth <cworth@...573...> wrote:
It was easy enough to see where inkscape is using cairo_move_to instead of cairo_curve_to:
Thanks for investigating this Carl, I looked into that optimization and tested it and I think it works as designed, at least for that file. Note that Inkscape draws screen in (usually horizontal) strips, so the entire feed_curve_to_cairo is called once for each strip. And in this file, it so happens that the logo sits on the boundary between the strips. So for one strip, some of the curves are replaced by movetos, and for the next strip, some others. Some curves overlap both strips and will be drawn twice, some only once. I don't see what might go wrong here.
I then wondered how replacing a curveto with a moveto in any path may result in drawing a _line_ which was _not in the path_. But then I remembered about the close_path command, z. If some curves preceding it were replaced by movetos, it will indeed draw a line from the last stroked point which will be wrong. But then the question is, is it correct to draw a closepath command from the last stroked point, not from the current point? If closepath would work from the current point, as set by movetos, there would be no bug here.
-- bulia byak Inkscape. Draw Freely. http://www.inkscape.org
On Sat, 5 Apr 2008 15:48:20 -0400, "bulia byak" wrote:
OK, while I'm still not sure about the correct behavior of close_path, I just eliminated it by replacing it with a lineto to the remembered start-of-path point, and that got rid of the bug. Thanks for helping to figure this out, and sorry for bothering you with this :)
That will make the bug go away, sure. But it will introduce another. If you don't do a cairo_close_path when the SVG has the close-path ('z') operator, then you'll get caps at the beginning and ending of the path rather than a join between them.
On Sat, Apr 5, 2008 at 2:40 PM, bulia byak <buliabyak@...400...> wrote:
On Sat, Apr 5, 2008 at 9:54 AM, Carl Worth <cworth@...573...> wrote: I then wondered how replacing a curveto with a moveto in any path may result in drawing a _line_ which was _not in the path_. But then I remembered about the close_path command, z.
Yes, the close_path is critical to the bug.
If some curves precedingit were replaced by movetos, it will indeed draw a line from the last stroked point which will be wrong. But then the question is, is it correct to draw a closepath command from the last stroked point, not from the current point? If closepath would work from the current point, as set by movetos, there would be no bug here.
That's not quite what's happening.
A close_path operation always do draw a line from the current point. But it draws a line (and joins) back to the beginning of the current sub-path, (that is, it joins back to the most recent move_to).
What's funny is that in cairo 1.2 we added a path manipulation function, (cairo_new_sub_path), that doesn't have any analogue in PostScript, PDF, or SVG. What it does is begin a new sub path but without setting the current point, (so it's exactly half of a move_to).
For your optimization you would actually want the other half of move_to. That is, what you want to do is to change the current point, but without starting a new sub-path. I suppose we could add another function for that, but I wonder if it would be more confusing than helpful in general.
In the meantime, you can actually do the right thing, but it takes a little more effort. What you would need to do is every time you are optimizing away a path element, save the final point, (but don't issue a move_to), replacing a point saved in any immediately previously optimized-away element. Then, at the next non-optimized-away element, if there's a saved point you would issue a cairo_line_to to that point. That way, you optimize away almost as much drawing, (all but one line), but you avoid introducing the extra, undesired sub-path.
I don't know if any of that is making sense, (I hope it is, but describing the state-management in prose can be tricky). I wanted to try writing code for that today, but I didn't get a chance.
Anyway, more than anything, I'm glad that you're playing with cairo inside of inkscape, and I want to let you know that we're always available if you have any questions or problems.
-Carl
On Sat, 05 Apr 2008 23:27:52 -0700, Carl Worth wrote:
For your optimization you would actually want the other half of move_to. That is, what you want to do is to change the current point, but without starting a new sub-path. I suppose we could add another function for that, but I wonder if it would be more confusing than helpful in general.
In the meantime, you can actually do the right thing, but it takes a little more effort.
By the way, the extra effort for the optimization is only necessary if the current sub-path will end in a close_path, (that is if in the path data there is a "z" before the next "m" or "M"). From the looks of it, the path representation that inkscape's working with knows this in advance, (the last moveto was either MOVE_TO or MOVE_TO_CLOSED and there's a 'closed' flag set in advance).
So, given that, the current optimization works just fine if the current subpath will not be closed. It's what's already coded and it's even more optimal[*] than my approach. So whether my idea gets coded up or not, it might be nice to do the current thing whenever the current path is not closed.
-Carl
[*] The current approach is more optimal since it replaces each line_to or curve_to with a move_to, hence no drawing at all. Instead, the best mine can do is to replace a series of line_to or curve_to operations with a single line_to. So if only a single operation gets optimized, then there's not much savings, (and in the case of a single line_to, no savings at all).
On Sun, Apr 6, 2008 at 3:27 AM, Carl Worth <cworth@...573...> wrote:
That will make the bug go away, sure. But it will introduce another. If you don't do a cairo_close_path when the SVG has the close-path ('z') operator, then you'll get caps at the beginning and ending of the path rather than a join between them.
Good catch! Although right now it does not matter, as it is only used for outline mode. But it will matter when we switch the main rendering to cairo.
For your optimization you would actually want the other half of move_to. That is, what you want to do is to change the current point, but without starting a new sub-path. I suppose we could add another function for that, but I wonder if it would be more confusing than helpful in general.
I think it would be helpful, and I think it makes perfect sense to add it, since you already have that other half-function :)
In the meantime, you can actually do the right thing, but it takes a little more effort. What you would need to do is every time you are optimizing away a path element, save the final point, (but don't issue a move_to), replacing a point saved in any immediately previously optimized-away element. Then, at the next non-optimized-away element, if there's a saved point you would issue a cairo_line_to to that point. That way, you optimize away almost as much drawing, (all but one line), but you avoid introducing the extra, undesired sub-path.
This would work, but I like it less for the obvious reason - it's less of an optimization, because it would still spend resources on drawing that invisible line. So I would prefer to use a move-without-new-subpath function.
Anyway, more than anything, I'm glad that you're playing with cairo inside of inkscape, and I want to let you know that we're always available if you have any questions or problems.
Thanks, that is very much appreciated!
On Sun, 6 Apr 2008 22:36:47 -0300, "bulia byak" wrote:
For your optimization you would actually want the other half of move_to. That is, what you want to do is to change the current point, but without starting a new sub-path. I suppose we could add another function for that, but I wonder if it would be more confusing than helpful in general.
I think it would be helpful, and I think it makes perfect sense to add it, since you already have that other half-function :)
It's possible, certainly. Do you have a good name suggestion for it?
I've added this to cairo's TODO list so we won't forget it:
And if there's more action on this front, (say a concrete proposal with a documentation update that explains how cairo_move_to, cairo_new_sub_path, and the new function all relate), then we could get this onto the 1.8 roadmap:
http://cairographics.org/roadmap
-Carl
Bulia and Carl,
Threads like this would seem to indicate that there is work being done on the Cairoification of Inkscape. Is any of the work worthy of sharing a status report? How are things falling in to place? What timeframe are you projecting for getting some code into SVN? Is there anything that others can do to get their hands dirty and help the process along?
Aaron Spike
On Mon, 14 Apr 2008 13:11:38 -0700, Carl Worth <cworth@...573...> wrote:
On Sun, 6 Apr 2008 22:36:47 -0300, "bulia byak" wrote:
For your optimization you would actually want the other half of move_to. That is, what you want to do is to change the current point, but without starting a new sub-path. I suppose we could add another function for that, but I wonder if it would be more confusing than helpful in general.
I think it would be helpful, and I think it makes perfect sense to add it, since you already have that other half-function :)
It's possible, certainly. Do you have a good name suggestion for it?
cairo_set_current_point
?
-mental
participants (4)
-
Aaron Spike -
bulia byak -
Carl Worth -
MenTaLguY