[PATCH] d2d1: Ignore whether the rectangle of the text layout in DrawText() is inverted.

[Henri Verbeet]

On Fri, 22 Jan 2021 at 03:21, 陈Daxia <chy2009123(a)gmail.com> wrote:
> @@ -1161,7 +1161,6 @@ static void STDMETHODCALLTYPE d2d_device_context_DrawText(ID2D1DeviceContext *if
>     IDWriteTextLayout *text_layout;
>     IDWriteFactory *dwrite_factory;
>     D2D1_POINT_2F origin;
> -    float width, height;
>     HRESULT hr;
>
> TRACE("iface %p, string %s, string_len %u, text_format %p, layout_rect %s, "
> @@ -1176,14 +1175,13 @@ static void STDMETHODCALLTYPE d2d_device_context_DrawText(ID2D1DeviceContext *if
>         return;
>     }
>
> -    width = max(0.0f, layout_rect->right - layout_rect->left);
> -    height = max(0.0f, layout_rect->bottom - layout_rect->top);
>     if (measuring_mode == DWRITE_MEASURING_MODE_NATURAL)
>         hr = IDWriteFactory_CreateTextLayout(dwrite_factory, string, string_len, text_format,
> -                width, height, &text_layout);
> +                fabs(layout_rect->right - layout_rect->left), fabs(layout_rect->bottom - layout_rect->top), &text_layout);
>     else
>         hr = IDWriteFactory_CreateGdiCompatibleTextLayout(dwrite_factory, string, string_len, text_format,
> -                width, height, render_target->desc.dpiX / 96.0f, (DWRITE_MATRIX *)&render_target->drawing_state.transform,
> +                fabs(layout_rect->right - layout_rect->left), fabs(layout_rect->bottom - layout_rect->top),
> +                render_target->desc.dpiX / 96.0f, (DWRITE_MATRIX *)&render_target->drawing_state.transform,
>                 measuring_mode == DWRITE_MEASURING_MODE_GDI_NATURAL, &text_layout);
>     IDWriteFactory_Release(dwrite_factory);
>     if (FAILED(hr))

When Nikolay sent 9521c6a1eb781ff7cb75a83544bf9b78f889a9e2, he said
his (private) tests showed negative width/height behave like zero
width/height. Do you have tests that contradict that? Regardless of
whether this patch is functionally correct, getting rid of the "width"
and "height" variables doesn't seem like an improvement.

[Nikolay Sivov]

On 1/25/21 5:08 PM, Henri Verbeet wrote:
> On Fri, 22 Jan 2021 at 03:21, 陈Daxia <chy2009123(a)gmail.com> wrote:
> > @@ -1161,7 +1161,6 @@ static void STDMETHODCALLTYPE d2d_device_context_DrawText(ID2D1DeviceContext *if
> >     IDWriteTextLayout *text_layout;
> >     IDWriteFactory *dwrite_factory;
> >     D2D1_POINT_2F origin;
> > -    float width, height;
> >     HRESULT hr;
> >
> > TRACE("iface %p, string %s, string_len %u, text_format %p, layout_rect %s, "
> > @@ -1176,14 +1175,13 @@ static void STDMETHODCALLTYPE d2d_device_context_DrawText(ID2D1DeviceContext *if
> >         return;
> >     }
> >
> > -    width = max(0.0f, layout_rect->right - layout_rect->left);
> > -    height = max(0.0f, layout_rect->bottom - layout_rect->top);
> >     if (measuring_mode == DWRITE_MEASURING_MODE_NATURAL)
> >         hr = IDWriteFactory_CreateTextLayout(dwrite_factory, string, string_len, text_format,
> > -                width, height, &text_layout);
> > +                fabs(layout_rect->right - layout_rect->left), fabs(layout_rect->bottom - layout_rect->top), &text_layout);
> >     else
> >         hr = IDWriteFactory_CreateGdiCompatibleTextLayout(dwrite_factory, string, string_len, text_format,
> > -                width, height, render_target->desc.dpiX / 96.0f, (DWRITE_MATRIX *)&render_target->drawing_state.transform,
> > +                fabs(layout_rect->right - layout_rect->left), fabs(layout_rect->bottom - layout_rect->top),
> > +                render_target->desc.dpiX / 96.0f, (DWRITE_MATRIX *)&render_target->drawing_state.transform,
> >                 measuring_mode == DWRITE_MEASURING_MODE_GDI_NATURAL, &text_layout);
> >     IDWriteFactory_Release(dwrite_factory);
> >     if (FAILED(hr))
> > When Nikolay sent 9521c6a1eb781ff7cb75a83544bf9b78f889a9e2, he said
> > his (private) tests showed negative width/height behave like zero
> > width/height. Do you have tests that contradict that? Regardless of
> > whether this patch is functionally correct, getting rid of the "width"
> > and "height" variables doesn't seem like an improvement.

The evidence I used is that negative or zero width performs the same -
it triggers default wrapping behavior (DWRITE_WORD_WRAPPING_WRAP), for
example for text without normal breaks like "ABC" it gives three lines,
for A, B, and C. Fixing it up with fabs() will obviously be different.
Now, that might be difficult to see now, because of wrapping itself does
not work correctly (I filed a bug for that, and am still working on it).

[Haoyang Chen]

Yeh, but if wrapping works, then the negative value of width/height
can't be treated as zero. So, I think the width and height of this
rectangle should be scalar.According to Nikolay Sivov's suggestion, if
you set the WordWrapping of text_layout to
DWRITE_WORD_WRAPPING_CHARACTER, then the wrapping will work properly
for now. So the width/height of the rectangle should always be valid
(except for zero), regardless of whether it is inverted or not.

Henri Verbeet <hverbeet(a)gmail.com> 于2021年1月25日周一 下午10:08写道：
> On Fri, 22 Jan 2021 at 03:21, 陈Daxia <chy2009123(a)gmail.com> wrote:
> > @@ -1161,7 +1161,6 @@ static void STDMETHODCALLTYPE d2d_device_context_DrawText(ID2D1DeviceContext *if
> >     IDWriteTextLayout *text_layout;
> >     IDWriteFactory *dwrite_factory;
> >     D2D1_POINT_2F origin;
> > -    float width, height;
> >     HRESULT hr;
> >
> > TRACE("iface %p, string %s, string_len %u, text_format %p, layout_rect %s, "
> > @@ -1176,14 +1175,13 @@ static void STDMETHODCALLTYPE d2d_device_context_DrawText(ID2D1DeviceContext *if
> >         return;
> >     }
> >
> > -    width = max(0.0f, layout_rect->right - layout_rect->left);
> > -    height = max(0.0f, layout_rect->bottom - layout_rect->top);
> >     if (measuring_mode == DWRITE_MEASURING_MODE_NATURAL)
> >         hr = IDWriteFactory_CreateTextLayout(dwrite_factory, string, string_len, text_format,
> > -                width, height, &text_layout);
> > +                fabs(layout_rect->right - layout_rect->left), fabs(layout_rect->bottom - layout_rect->top), &text_layout);
> >     else
> >         hr = IDWriteFactory_CreateGdiCompatibleTextLayout(dwrite_factory, string, string_len, text_format,
> > -                width, height, render_target->desc.dpiX / 96.0f, (DWRITE_MATRIX *)&render_target->drawing_state.transform,
> > +                fabs(layout_rect->right - layout_rect->left), fabs(layout_rect->bottom - layout_rect->top),
> > +                render_target->desc.dpiX / 96.0f, (DWRITE_MATRIX *)&render_target->drawing_state.transform,
> >                 measuring_mode == DWRITE_MEASURING_MODE_GDI_NATURAL, &text_layout);
> >     IDWriteFactory_Release(dwrite_factory);
> >     if (FAILED(hr))
>
> When Nikolay sent 9521c6a1eb781ff7cb75a83544bf9b78f889a9e2, he said
> his (private) tests showed negative width/height behave like zero
> width/height. Do you have tests that contradict that? Regardless of
> whether this patch is functionally correct, getting rid of the "width"
> and "height" variables doesn't seem like an improvement.