[PATCH] d2d: Suppress last empty segment also for D2D1_FIGURE_END_OPEN

[Stefan Brüns]

When the last vertex is coincident with the first vertex, the last
segment should be suppressed for both END_OPEN and END_CLOSED.

When the last, zero length segment is not omitted
d2d_geometry_intersect_self will add invalid segments.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51139
Signed-off-by: Stefan Brüns 
---
 dlls/d2d1/geometry.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/dlls/d2d1/geometry.c b/dlls/d2d1/geometry.c
index a7074899fda..95aaf199ad2 100644
--- a/dlls/d2d1/geometry.c
+++ b/dlls/d2d1/geometry.c
@@ -2917,8 +2917,9 @@ static void STDMETHODCALLTYPE d2d_geometry_sink_EndFigure(ID2D1GeometrySink *ifa
     {
         ++geometry->u.path.segment_count;
         figure->flags |= D2D_FIGURE_FLAG_CLOSED;
-        if (!memcmp(&figure->vertices[0], &figure->vertices[figure->vertex_count - 1], sizeof(*figure->vertices)))
-            --figure->vertex_count;
+    }
+    if (!memcmp(&figure->vertices[0], &figure->vertices[figure->vertex_count - 1], sizeof(*figure->vertices))) {
+        --figure->vertex_count;
     }

if (!d2d_geometry_add_figure_outline(geometry, figure, figure_end))

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=104686

Your paranoid android.

=== debian11 (32 bit report) ===

d2d1:
d2d1.c:6688: Test failed: Figure does not match.
d2d1.c:6729: Test failed: Figure does not match.
d2d1.c:10390: Test failed: Test 33: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 34: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 37: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 38: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 41: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 42: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 44: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 45: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 47: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 54: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 57: Got unexpected result 0.

=== debian11 (32 bit Chinese:China report) ===

d2d1:
d2d1.c:10390: Test failed: Test 33: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 34: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 37: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 38: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 41: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 42: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 44: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 45: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 47: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 54: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 57: Got unexpected result 0.

=== debian11 (32 bit WoW report) ===

d2d1:
d2d1.c:10390: Test failed: Test 33: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 34: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 37: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 38: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 41: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 42: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 44: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 45: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 47: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 54: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 57: Got unexpected result 0.
d2d1.c:6664: Test failed: Figure does not match.

=== debian11 (64 bit WoW report) ===

d2d1:
d2d1.c:10390: Test failed: Test 33: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 34: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 37: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 38: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 41: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 42: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 44: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 45: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 47: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 54: Got unexpected result 0.
d2d1.c:10390: Test failed: Test 57: Got unexpected result 0.

[Henri Verbeet]

On Fri, 31 Dec 2021 at 08:23, Stefan Brüns  wrote:
> When the last vertex is coincident with the first vertex, the last
> segment should be suppressed for both END_OPEN and END_CLOSED.
>
> When the last, zero length segment is not omitted
> d2d_geometry_intersect_self will add invalid segments.

Unfortunately, I don't think that's correct. For an END_OPEN figure,
the first and last vertex coinciding doesn't imply that the last
segment is a zero-length segment.

Note that if the issue is with some part of
d2d_geometry_intersect_self() not handling coinciding vertices well,
that issue should be fixed there. Begin and end vertices coinciding
isn't the only way that situation can occur; consider for example a
trifolium curve (or rose curves more generally) with a shared vertex
at the centre.

[Stefan Brüns]

On Montag, 3. Januar 2022 12:58:50 CET you wrote:
> On Fri, 31 Dec 2021 at 08:23, Stefan Brüns

wrote:
> > When the last vertex is coincident with the first vertex, the last
> > segment should be suppressed for both END_OPEN and END_CLOSED.
> >
> > When the last, zero length segment is not omitted
> > d2d_geometry_intersect_self will add invalid segments.
>
> Unfortunately, I don't think that's correct. For an END_OPEN figure,
> the first and last vertex coinciding doesn't imply that the last
> segment is a zero-length segment.

Please supply an example where coinciding does not mean zero length.

Last vertex refers to the second vertex of the last segment, not its start 
vertex.
> Note that if the issue is with some part of
> d2d_geometry_intersect_self() not handling coinciding vertices well,
> that issue should be fixed there. Begin and end vertices coinciding
> isn't the only way that situation can occur; consider for example a
> trifolium curve (or rose curves more generally) with a shared vertex
> at the centre.

Coinciding vertices in general are not a problem, only when the start and end 
vertex of a single line/curve segment are identical (and for any curve, also 
all control points are coincident).

AddVertex even checks for this and suppresses the vertex (though, only for 
lines).

Anyway, just suppressing the vertex causes some issues elsewhere, thus I have 
been reworking the patch.

Regards,

Stefan

[Henri Verbeet]

On Mon, 3 Jan 2022 at 15:10, Stefan Brüns  wrote:
> On Montag, 3. Januar 2022 12:58:50 CET you wrote:
> > On Fri, 31 Dec 2021 at 08:23, Stefan Brüns
>
> wrote:
> > > When the last vertex is coincident with the first vertex, the last
> > > segment should be suppressed for both END_OPEN and END_CLOSED.
> > >
> > > When the last, zero length segment is not omitted
> > > d2d_geometry_intersect_self will add invalid segments.
> >
> > Unfortunately, I don't think that's correct. For an END_OPEN figure,
> > the first and last vertex coinciding doesn't imply that the last
> > segment is a zero-length segment.
>
> Please supply an example where coinciding does not mean zero length.
>
> Last vertex refers to the second vertex of the last segment, not its start
> vertex.

Consider for example a triangle like this:

p0, p3
       /\
      /  \
     /____\
   p1      p2

and the corresponding Direct2D calls:

BeginFigure(p0, ...);
    AddLine(p1);
    AddLine(p2);
    AddLine(p3);
    EndFigure(END_OPEN/END_CLOSED);

In the END_CLOSED case, we'd first create three segments:

p0->p1
    p1->p2
    p2->p3

and then we'd have a fourth implicit zero-length segment p3->p0. By
removing the p3 vertex, we replace the p2->p3 and p3->p0 segments with
an implicit p2->p0 segment.

In the END_OPEN case however, we just have the three explicit segments
above, without the implicit p3->p0 segment. In that case, removing the
p3 vertex simply removes the p2->p3 segment, and we'd be left with the
following figure:

p0
       /
      /
     /____
   p1      p2

Looking at d2d_geometry_intersect_self() though, it looks like it
doesn't take D2D_FIGURE_FLAG_CLOSED into account, probably because it
predates its introduction. Perhaps that explains the behaviour you're
seeing. In any case, thanks for looking into this!

Henri

[Stefan Brüns]

On Montag, 3. Januar 2022 16:40:50 CET you wrote:
> On Mon, 3 Jan 2022 at 15:10, Stefan Brüns

wrote:
> > On Montag, 3. Januar 2022 12:58:50 CET you wrote:
> > > On Fri, 31 Dec 2021 at 08:23, Stefan Brüns
> >
> > wrote:
> > > > When the last vertex is coincident with the first vertex, the last
> > > > segment should be suppressed for both END_OPEN and END_CLOSED.
> > > >
> > > > When the last, zero length segment is not omitted
> > > > d2d_geometry_intersect_self will add invalid segments.
> > >
> > > Unfortunately, I don't think that's correct. For an END_OPEN figure,
> > > the first and last vertex coinciding doesn't imply that the last
> > > segment is a zero-length segment.
> >
> > Please supply an example where coinciding does not mean zero length.
> >
> > Last vertex refers to the second vertex of the last segment, not its start
> > vertex.
>
> Consider for example a triangle like this:
>  p0, p3
>    /\
>   /  \
>  /____\
>
> p1      p2
>
> and the corresponding Direct2D calls:
> BeginFigure(p0, ...);
> AddLine(p1);
> AddLine(p2);
> AddLine(p3);
> EndFigure(END_OPEN/END_CLOSED);
>
> In the END_CLOSED case, we'd first create three segments:
> p0->p1
> p1->p2
> p2->p3
>
> and then we'd have a fourth implicit zero-length segment p3->p0. By
> removing the p3 vertex, we replace the p2->p3 and p3->p0 segments with
> an implicit p2->p0 segment.
>
> In the END_OPEN case however, we just have the three explicit segments
> above, without the implicit p3->p0 segment. In that case, removing the
> p3 vertex simply removes the p2->p3 segment, and we'd be left with the
> following figure:

Seems my explanation was somewhat unclear ...

I do not want to remove the last vertex (p3), but the last segment. In the 
current code, vertex_type[3] (using your example) for p3->p0 is always marked 
as TYPE_LINE (see EndFigure code). In case of p0 == p3, this causes troubles 
in the intersection code. In case p0 != p3, the p3->p0 segment must be a line 
(for intersection tests and filling), regardless if END_OPEN or END_CLOSED. 
The only difference between OPEN or CLOSED is the stroking of the p3->p0 
segment.

What seems to work quite fine is setting the TYPE for the last vertex 
(vertex_type[3]) to either TYPE_LINE for non-coincident p3/p0, or TYPE_END 
(newly added type) otherwise. (TYPE_NONE is already overloaded and can't be 
used.)

For my current approach, see
https://build.opensuse.org/package/view_file/
home:StefanBruens:branches:Emulators/wine/0003-d2d1-Skip-last-empty-segment-
in-intersection-tests.patch?expand=1&rev=19

Regards, Stefan

[Henri Verbeet]

On Mon, 3 Jan 2022 at 17:59, Stefan Brüns  wrote:
> Seems my explanation was somewhat unclear ...
>
> I do not want to remove the last vertex (p3), but the last segment. In the
> current code, vertex_type[3] (using your example) for p3->p0 is always marked
> as TYPE_LINE (see EndFigure code). In case of p0 == p3, this causes troubles
> in the intersection code. In case p0 != p3, the p3->p0 segment must be a line
> (for intersection tests and filling), regardless if END_OPEN or END_CLOSED.
> The only difference between OPEN or CLOSED is the stroking of the p3->p0
> segment.

I think I see what you mean. You're right, we need the implicit
segment for e.g. filling, so the current d2d_geometry_intersect_self()
behaviour is correct in that regard.
> What seems to work quite fine is setting the TYPE for the last vertex
> (vertex_type[3]) to either TYPE_LINE for non-coincident p3/p0, or TYPE_END
> (newly added type) otherwise. (TYPE_NONE is already overloaded and can't be
> used.)
>
> For my current approach, see
> https://build.opensuse.org/package/view_file/
> home:StefanBruens:branches:Emulators/wine/0003-d2d1-Skip-last-empty-segment-
> in-intersection-tests.patch?expand=1&rev=19

I think that approach should work, yes. Alternatives would be
introducing a D2D_FIGURE_FLAG_ flag for this or just explicitly
checking for coinciding start and end vertices in
d2d_geometry_intersect_self(). Those alternatives have their
advantages and disadvantages, but ultimately are largely equivalent. I
don't have a clear favourite, so in that regard your (new) patch is
fine. I would however request to please include a small test for the
issue this fixes; both to demonstrate the issue and to prevent future
regressions.

Henri