[PATCH v2 0/1] MR3752: d2d1: Respect stroke style D2D1_STROKE_TRANSFORM_TYPE_FIXED in DrawGeometry::ID2D1DeviceContext1().

[Dmitry Timoshkov (＠dmitry)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54368

--
  v2: d2d1: Respect stroke style D2D1_STROKE_TRANSFORM_TYPE_FIXED in DrawGeometry::ID2D1DeviceContext1().

https://gitlab.winehq.org/wine/wine/-/merge_requests/3752

[PATCH v2 1/1] d2d1: Respect stroke style D2D1_STROKE_TRANSFORM_TYPE_FIXED in DrawGeometry::ID2D1DeviceContext1().

[Dmitry Timoshkov]

From: Dmitry Timoshkov

Signed-off-by: Dmitry Timoshkov 
---
 dlls/d2d1/d2d1_private.h |  1 +
 dlls/d2d1/device.c       | 10 ++++++++--
 dlls/d2d1/stroke.c       | 10 +++++++++-
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h
index f3029bab0f2..1dd5cdb0a39 100644
--- a/dlls/d2d1/d2d1_private.h
+++ b/dlls/d2d1/d2d1_private.h
@@ -383,6 +383,7 @@ struct d2d_stroke_style

HRESULT d2d_stroke_style_init(struct d2d_stroke_style *style, ID2D1Factory *factory,
         const D2D1_STROKE_STYLE_PROPERTIES1 *desc, const float *dashes, UINT32 dash_count);
+struct d2d_stroke_style *unsafe_impl_from_ID2D1StrokeStyle(ID2D1StrokeStyle *iface);

struct d2d_layer
 {
diff --git a/dlls/d2d1/device.c b/dlls/d2d1/device.c
index 33cf943010c..fbcd51fe6c1 100644
--- a/dlls/d2d1/device.c
+++ b/dlls/d2d1/device.c
@@ -942,6 +942,7 @@ static void STDMETHODCALLTYPE d2d_device_context_DrawGeometry(ID2D1DeviceContext
     const struct d2d_geometry *geometry_impl = unsafe_impl_from_ID2D1Geometry(geometry);
     struct d2d_device_context *context = impl_from_ID2D1DeviceContext(iface);
     struct d2d_brush *brush_impl = unsafe_impl_from_ID2D1Brush(brush);
+    struct d2d_stroke_style *stroke_style_impl = unsafe_impl_from_ID2D1StrokeStyle(stroke_style);

TRACE("iface %p, geometry %p, brush %p, stroke_width %.8e, stroke_style %p.\n",
             iface, geometry, brush, stroke_width, stroke_style);
@@ -953,8 +954,13 @@ static void STDMETHODCALLTYPE d2d_device_context_DrawGeometry(ID2D1DeviceContext
         return;
     }

-    if (stroke_style)
-        FIXME("Ignoring stroke style %p.\n", stroke_style);
+    if (stroke_style_impl)
+    {
+        if (stroke_style_impl->desc.transformType == D2D1_STROKE_TRANSFORM_TYPE_FIXED)
+            stroke_width /= context->drawing_state.transform.m11;
+        else
+            FIXME("Ignoring stroke style %p (%d).\n", stroke_style, stroke_style_impl->desc.transformType);
+    }

d2d_device_context_draw_geometry(context, geometry_impl, brush_impl, stroke_width);
 }
diff --git a/dlls/d2d1/stroke.c b/dlls/d2d1/stroke.c
index 16fa1f60771..652b6d0b2a6 100644
--- a/dlls/d2d1/stroke.c
+++ b/dlls/d2d1/stroke.c
@@ -191,6 +191,14 @@ static const struct ID2D1StrokeStyle1Vtbl d2d_stroke_style_vtbl =
     d2d_stroke_style_GetStrokeTransformType
 };

+struct d2d_stroke_style *unsafe_impl_from_ID2D1StrokeStyle(ID2D1StrokeStyle *iface)
+{
+    if (!iface)
+        return NULL;
+    assert((const struct ID2D1StrokeStyle1Vtbl *)iface->lpVtbl == &d2d_stroke_style_vtbl);
+    return CONTAINING_RECORD(iface, struct d2d_stroke_style, ID2D1StrokeStyle1_iface);
+}
+
 HRESULT d2d_stroke_style_init(struct d2d_stroke_style *style, ID2D1Factory *factory,
         const D2D1_STROKE_STYLE_PROPERTIES1 *desc, const float *dashes, UINT32 dash_count)
 {
@@ -211,7 +219,7 @@ HRESULT d2d_stroke_style_init(struct d2d_stroke_style *style, ID2D1Factory *fact
     if (desc->dashStyle > D2D1_DASH_STYLE_CUSTOM)
         return E_INVALIDARG;

-    if (desc->transformType != D2D1_STROKE_TRANSFORM_TYPE_NORMAL)
+    if (desc->transformType != D2D1_STROKE_TRANSFORM_TYPE_NORMAL && desc->transformType != D2D1_STROKE_TRANSFORM_TYPE_FIXED)
         FIXME("transformType %d is not supported\n", desc->transformType);

style->ID2D1StrokeStyle1_iface.lpVtbl = &d2d_stroke_style_vtbl;

[Nikolay Sivov (＠nsivov)]

On Thu Sep  7 08:50:18 2023 +0000, Dmitry Timoshkov wrote:
> > > - if (stroke_style)
> > > -
> > >    FIXME("Ignoring stroke style %p.\n", stroke_style);
> > > - if (stroke_style_impl)
> > > - {
> > > -
> > >    if (stroke_style_impl->desc.transformType == D2D1_STROKE_TRANSFORM_TYPE_FIXED)
> > > -
> > >        stroke_width /= context->drawing_state.transform.m11;
> > > - }
> >
> > This looks incomplete, and definitely not enough to remove the fixme.
>
> Why is it only using m11 component?
> The idea of using m11 was suggested by geometry.c,::StrokeContainsPoint().
> FIXME() can be left as is if desired.

Where is it used like that? I don't see drawing_state.transform.m11 used anywhere by itself. FIXME should stay unconditionally, because we don't use any aspect of it, like dashes or join type.

Correct way I imagine is to generate a stroked geometry that would be taking into account everything style has configured, together with current state. Obviously I don't expect this to happen in your patch.

[Dmitry Timoshkov (＠dmitry)]

> Where is it used like that? I don't see drawing_state.transform.m11 used
> anywhere by itself.

You asked about using only m11 component, and I pointed out where an idea
is coming from. Screenshots from the referenced bug report seem to confirm
that it works well for at least an application it was tested with.
> FIXME should stay unconditionally, because we don't use any aspect of it,
> like dashes or join type.
>
> Correct way I imagine is to generate a stroked geometry that would be
> taking into account everything style has configured, together with
> current state. Obviously I don't expect this to happen in your patch.

Yes, the patch has limited functionality, and apparently doesn't pretend
to implement support for all kinds of stroke styles. An obvious thing that
the patch aims to fix is the stroke width, and according to my testing it
works pretty well. Should I just drop it, or the patch could be improved
and accepted?

[Henri Verbeet (＠hverbeet)]

> Where is it used like that? I don't see drawing_state.transform.m11 used anywhere by itself.

I don't think the reference was terribly clear, but d2d_transformed_geometry_StrokeContainsPoint() does this as well, introduced by commit b88f06fd3425e04ed6148ba88d0a9dc40b654fcc. It's clearly not ideal, but it does help the common trivial cases.

[Nikolay Sivov (＠nsivov)]

On Tue Sep 12 10:23:54 2023 +0000, Henri Verbeet wrote:
> > Where is it used like that? I don't see drawing_state.transform.m11
>
> used anywhere by itself.
> I don't think the reference was terribly clear, but
> d2d_transformed_geometry_StrokeContainsPoint() does this as well,
> introduced by commit b88f06fd3425e04ed6148ba88d0a9dc40b654fcc. It's
> clearly not ideal, but it does help the common trivial cases.

I see, thanks.

[Nikolay Sivov (＠nsivov)]

On Mon Sep 11 17:20:37 2023 +0000, Dmitry Timoshkov wrote:
> > Where is it used like that? I don't see drawing_state.transform.m11 used
> > anywhere by itself.
>
> You asked about using only m11 component, and I pointed out where an idea
> is coming from. Screenshots from the referenced bug report seem to confirm
> that it works well for at least an application it was tested with.
> > FIXME should stay unconditionally, because we don't use any aspect of it,
> > like dashes or join type.
> >
> > Correct way I imagine is to generate a stroked geometry that would be
> > taking into account everything style has configured, together with
> > current state. Obviously I don't expect this to happen in your patch.
>
> Yes, the patch has limited functionality, and apparently doesn't pretend
> to implement support for all kinds of stroke styles. An obvious thing that
> the patch aims to fix is the stroke width, and according to my testing it
> works pretty well. Should I just drop it, or the patch could be improved
> and accepted?

Please drop unrelated changes, like the one in d2d_stroke_style_init() or moved fixme in d2d_device_context_DrawGeometry(), leave just what you need - this single scaling factor.