The implementation uses rel_line_to to create the rectangle path which introduces additional rounding errors as demonstrated in the snippet below. (The gap goes away if cairo_line_to is used instead.) Furthermore, the cairo_rectangle API introduces possible rounding errors by having to specify the width and height of the rectangle instead of the other corner. Snippet that demonstrates the problem: /* don't normalize, the snippet demonstrates a rounding error * snippet_normalize (cr, width, height); */ cairo_set_antialias(cr, CAIRO_ANTIALIAS_NONE); cairo_translate(cr, -300, -300); cairo_scale(cr, 677.0/26, 677.0/26); cairo_translate(cr, 1, 1); /* this should draw a seamless 2x2 rectangle */ cairo_rectangle(cr, 11, 11, 1, 1); cairo_rectangle(cr, 11, 12, 1, 1); cairo_rectangle(cr, 12, 11, 1, 1); cairo_rectangle(cr, 12, 12, 1, 1); cairo_set_source_rgb(cr, 0, 0, 0); cairo_fill(cr);
Created attachment 2926 [details] [review] change cairo_rectangle to use cairo_line_to instead of cairo_rel_line_to This fixes the first problem; I guess it's too late for an API change now.
The bug here is in cairo_rel_line_to which loses precision when converting the relative value to fixed-point. It should instead only convert to fixed- point after the aboslute position is computed. When that bug is fixed, the two versions of cairo_rectangle as seen in the patch above should be equivalent.
(patch) the patch has a typo, y+width should be y+height (my test program only draws squares, apparently). (cworth) If cairo_rel_line_to is fixed that way, that will definitely improve the situation a lot. There will still be (much smaller) rounding errors due to the limited precision of double though.
If all rounding is done correctly, there is no need to worry about epsilon errors ... as long as they are less than 1 fixed point device unit, they won't make any difference to the output, things will still be pixel aligned in the end. There were some problems earlier where we were truncating converting to ints rather than rounding - see bug 2379. Don't know if we still have these.
(otaylor) #2739 is a different issue, it is concerned with drawing primitives with slightly varying coordinates, and expecting the result to be the same. The issue in this report is that when an application specifies two polygons with an identical lines, the respective border should be exactly the same for the two polygons. To achieve this, you have to round everything in exactly the same way; it actually does not matter much how. NB. this is primarily a non-antialiasing issue, because without antialiasing, the smallest error can eventually lead to missing whole pixels, if it happens at a rounding boundary. With antialiasing, this will hardly ever produce visible differences.
If you do rounding correctly, then the cases that you care about (pixel aligned primitives are robust) - x,y,width,height = 10,10,40,40 Gives the same as x,y,width,height = 9.9999,10.0001,39.99999,40.0001 With antialiasing off, you *can* construct cases that trigger on epsilon changes, but you still get robustness in the normal cases. Making the assumption that it is ever possible to get the *same* double values in two different code paths is not a good idea.
To be a little more explicit, if everything rounds correctly, then to introduce seems as you do, then you need to end up with numbers that are (N+0.5/66536) +/- epsilon Ending up with something that is at (N+0.5) +/- epsilon As in your test case is much more likely. x' = ((x + 1) * 677 / 26) - 300 gives x' = 38.5 for x = 12. I think the problem is likely cairo_fixed_from_double() which doesn't round correctly.
Changing cairo_fixed_from_double does indeed fix this test case. Now committed to CVS: cairo_fixed_t _cairo_fixed_from_double (double d) { - return (cairo_fixed_t) (d * 65536); + return (cairo_fixed_t) floor (d * 65536 + 0.5); } Though I would still like to clean up the precision problem with cairo_rel_line_to and friends, (eg. make cairo store the current point with double-precision instead of just in fixed-point). PS. Bertram, if you could send the test case with appropriate licensing information I would definitely like to include it in the test suite. Thanks!
Created attachment 2937 [details] [review] testcase (cworth) is this what you had in mind?
Created attachment 2938 [details] [review] testcase - with corrected license (cworth) I forgot to replace a few occurrences of 'Billy Biggs'; I also had not read your followup email. Sorry for this. (And I think using 'the author' is a very good idea)
i feel so unloved...
Thanks for the test case! All committed now.
Move bugs against "cvs" version to "0.9.3" so we can remove the "cvs" version.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.