Re: VCL drawPolygon() off-by-one without line color

Hi,

On Fri, Nov 1, 2019 at 9:48 PM Caolán McNamara <caolanm@redhat.com> wrote:

> On Fri, 2019-11-01 at 13:40 +0100, Luboš Luňák wrote:
> > , the second and third rectangles miss their right and bottom edges.
> > The first one is correct, and it is because that one uses drawRect()
>
> There are so many rectangles there that I'm not sure I see the
> difference, but I know that the various ::drawRect implementations are
> all hacked to pull in the bottom right by a pixel for the line draw so
> drawRect is the "odd" case and drawPolyPolygon is the "normal" one.
>
> vcl/headless/svpgdi.cxx's drawRect just calls drawPolyPolygon with a
> -1,-1 for the line case. While X11SalGraphicsImpl::drawRect calls
> XDrawRectangle with a -1,-1, AquaSalGraphics::drawRect likewise and so
> on. So if they were left to their own "natural" behaviour the drawRect
> would presumably give the same results as drawPolyPolygon.

The case in visualbackendtest corresponds to:
- BackendTest::testDrawFilledRectWithRectangle
- BackendTest::testDrawFilledRectWithPolygon
- BackendTest::testDrawFilledRectWithPolyPolygon
- BackendTest::testDrawFilledRectWithPolyPolygon2D

Looking into test case for testDrawFilledRectWithPolygon it draws a polygon
for 2,2 -> 10,2 -> 10,10 -> 2,10 and from this setup I would say it should
draw a filled polygon with pixel 10 inclusive, but that's not the case. I
guess drawRect just compensates for this fact. In any way - either drawRect
or drawPolygon (PolyPolygon, Polyline) is needs fixing and we need to make
our expectations clear and document them (with the test and clarifying in
the test).

I think the reason polygon draws it like this is because interpretation of
floating-point coordinates is usually exclusive because that's what makes
sense when you have sub-pixel positioning, but for integer coordinates
(like in this case) it is inclusive and we are mixing one with the other on
various levels. OTOH why doesn't this happen

Generally, I don't think this is a too much of a problem, because we
usually draw with line + fill color, even when they are both set to the
same color, but we need to set our expectations straight. The bad think
because of this issue is that backends usually either "fill" or "stroke",
so because of this problem here we need to actually do 2 drawing operations
when we would only need 1. Generally not a problem, but with a more
complicated polygons we are giving up performance.

Presumably the goal is to get the same results in the skia backend as
> the existing ones so if you get the same results I'd call it a job well
> done and leave it at that ;-) I imagine any deviation from how it works
> currently will just triggers regressions.
Well, the BackendTests were created just for off-by-one errors and to make
sure there are no differences in rendering between backends, but the tests
also go through OutputDevice interface so the expectations of what should
be drawn is clear. I don't agree we should just do-it-like-everyone-else in
this case (I think this is a very bad practice), we need to do it properly
and test driven.

The BackendTests are disabled currently for every backend because most of
the backends show various rendering issues in various tests. I didn't have
time to evaluate the results yet and investigate what the problems are and
how to fix them or the test sets up wrong expectations.

Regards, Tomaž