Re: [PATCH] d2d1: Partially implement StrokeContainsPoint() for rectangle geometry

[Henri Verbeet]

On 31 August 2017 at 14:00, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> -    if (transform)
> -    {
> -        if (!d2d_matrix_invert(&g_i, transform))
> -            return D2DERR_UNSUPPORTED_OPERATION;
> -        d2d_point_transform(&point, &g_i, point.x, point.y);
> -    }
> +    if (FAILED(hr = geometry_inv_transform_point(transform, &point)))
> +        return hr;
> That's fine in principle, but please do it in a separate commit, and
> please follow the existing conventions. E.g.:

if (transform && FAILED(hr = d2d_point_transform_inverse(&point,
transform, point.x, point.y)))
        return hr;
> +    stroke_width /= 2.0f;
> +
> +    rect = geometry->u.rectangle.rect;
> +    rect.left -= stroke_width;
> +    rect.right += stroke_width;
> +    rect.top -= stroke_width;
> +    rect.bottom += stroke_width;
> +
> +    dx = max(fabsf((rect.right  + rect.left) / 2.0f - point.x) - (rect.right  - rect.left) / 2.0f, 0.0f);
> +    dy = max(fabsf((rect.bottom + rect.top)  / 2.0f - point.y) - (rect.bottom - rect.top)  / 2.0f, 0.0f);
> +
> +    if ((*contains = tolerance * tolerance > (dx * dx + dy * dy)))
> +    {
> +        rect = geometry->u.rectangle.rect;
> +        stroke_width += tolerance;
> +        rect.left += stroke_width;
> +        rect.right -= stroke_width;
> +        rect.top += stroke_width;
> +        rect.bottom -= stroke_width;
> +
> +        *contains = rect.left >= point.x || rect.right <= point.x || rect.top >= point.y || rect.bottom <= point.y;
> +    }
> I think that looks a little more complicated than it needs to be. How about:

D2D1_POINT_2F d, s;
...
    s.x = rect.right - rect.left;
    s.y = rect.bottom - rect.top;
    d.x = fabsf((rect.right + rect.left) * 0.5f - point.x);
    d.y = fabsf((rect.bottom + rect.top) * 0.5f - point.y);

    if (d.x < (s.x - stroke_width) * 0.5f - tolerance && d.y < (s.y -
stroke_width) * 0.5f - tolerance)
    {
        *contains = FALSE;
        return S_OK;
    }

    d.x = max(d.x - (s.x + stroke_width) * 0.5f, 0.0f);
    d.y = max(d.y - (s.y + stroke_width) * 0.5f, 0.0f);
    *contains = (d.x * d.x + d.y * d.y) < tolerance * tolerance;

I notice you don't have any tests with transformations, is that
intentional? I think you'd need to transform the stroke width with the
transformation matrix.

[PATCH] d2d1: Partially implement StrokeContainsPoint() for rectangle geometry

[Nikolay Sivov]

On 01.09.2017 1:25, Henri Verbeet wrote:
> On 31 August 2017 at 14:00, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> > -    if (transform)
> > -    {
> > -        if (!d2d_matrix_invert(&g_i, transform))
> > -            return D2DERR_UNSUPPORTED_OPERATION;
> > -        d2d_point_transform(&point, &g_i, point.x, point.y);
> > -    }
> > +    if (FAILED(hr = geometry_inv_transform_point(transform, &point)))
> > +        return hr;
> > That's fine in principle, but please do it in a separate commit, and
> > please follow the existing conventions. E.g.:
>
> if (transform && FAILED(hr = d2d_point_transform_inverse(&point,
> transform, point.x, point.y)))
>         return hr;

Sure.
> > +    stroke_width /= 2.0f;
> > +
> > +    rect = geometry->u.rectangle.rect;
> > +    rect.left -= stroke_width;
> > +    rect.right += stroke_width;
> > +    rect.top -= stroke_width;
> > +    rect.bottom += stroke_width;
> > +
> > +    dx = max(fabsf((rect.right  + rect.left) / 2.0f - point.x) - (rect.right  - rect.left) / 2.0f, 0.0f);
> > +    dy = max(fabsf((rect.bottom + rect.top)  / 2.0f - point.y) - (rect.bottom - rect.top)  / 2.0f, 0.0f);
> > +
> > +    if ((*contains = tolerance * tolerance > (dx * dx + dy * dy)))
> > +    {
> > +        rect = geometry->u.rectangle.rect;
> > +        stroke_width += tolerance;
> > +        rect.left += stroke_width;
> > +        rect.right -= stroke_width;
> > +        rect.top += stroke_width;
> > +        rect.bottom -= stroke_width;
> > +
> > +        *contains = rect.left >= point.x || rect.right <= point.x || rect.top >= point.y || rect.bottom <= point.y;
> > +    }
> > I think that looks a little more complicated than it needs to be. How about:
>
> D2D1_POINT_2F d, s;
> ...
>     s.x = rect.right - rect.left;
>     s.y = rect.bottom - rect.top;
>     d.x = fabsf((rect.right + rect.left) * 0.5f - point.x);
>     d.y = fabsf((rect.bottom + rect.top) * 0.5f - point.y);
>
> if (d.x < (s.x - stroke_width) * 0.5f - tolerance && d.y < (s.y -
> stroke_width) * 0.5f - tolerance)
>     {
>         *contains = FALSE;
>         return S_OK;
>     }
>
> d.x = max(d.x - (s.x + stroke_width) * 0.5f, 0.0f);
>     d.y = max(d.y - (s.y + stroke_width) * 0.5f, 0.0f);
>     *contains = (d.x * d.x + d.y * d.y) < tolerance * tolerance;

Ok, I'll take a look.
> I notice you don't have any tests with transformations, is that
> intentional? I think you'd need to transform the stroke width with the
> transformation matrix.

No particular reason, but it's a good point. Also we don't have any
tests for non-invertible case apparently, I'll add some.