Re: gdiplus: implement GdipTransformPoints

[Nikolay Sivov]

Vincent Povirk wrote:
> From 1de9b2291bf9a5617ca1e3b0cba5f1260889c259 Mon Sep 17 00:00:00 2001

From: Vincent Povirk 
Date: Mon, 23 Mar 2009 16:34:12 -0500
Subject: [PATCH] gdiplus: implement GdipTransformPoints

It's a strange way to check enumeration with integer comparison, isn't it?
And not clear in this particular case.
> - if (stat == Ok)
> - {
> -
>    unitscale = convert_unit(graphics->hdc, graphics->unit);
> -
> -
>    if(graphics->unit != UnitDisplay)
> -
>        unitscale *= graphics->scale;
> -
> -
>    if (src_space <= CoordinateSpaceWorld && dst_space >= CoordinateSpacePage)
> -
>        GdipMultiplyMatrix(matrix, graphics->worldtrans, MatrixOrderAppend);
> -
>    if (src_space <= CoordinateSpacePage && dst_space >= CoordinateSpaceDevice)
> -
>        GdipScaleMatrix(matrix, unitscale, unitscale, MatrixOrderAppend);
> -
> -
>    if (dst_space <= CoordinateSpacePage && src_space >= CoordinateSpaceDevice)
> -
>        GdipScaleMatrix(matrix, 1.0/unitscale, 1.0/unitscale, MatrixOrderAppend);
> -
>    if (dst_space <= CoordinateSpaceWorld && src_space >= CoordinateSpacePage)
> -
>    {
> -
>        GpMatrix *inverted_transform;
> -
>        stat = GdipCloneMatrix(graphics->worldtrans, &inverted_transform);
> -
>        if (stat == Ok)
> -
>        {
> -
>            GdipInvertMatrix(inverted_transform);
> -
>            GdipMultiplyMatrix(matrix, inverted_transform, MatrixOrderAppend);
> -
>            GdipDeleteMatrix(inverted_transform);
> -
>        }
> -

Maybe it's better to use 'switch' for source coords with falling and ifs 
inside cases for destination.

<= CoordinateSpaceWorld is just CoordinateSpaceWorld
> = CoordinateSpaceDevice - is CoordinateSpaceDevice.

gdiplus: implement GdipTransformPoints

[Vincent Povirk]

It's odd, but it seems like the simplest way to write this.

I've tried what I think was your suggestion, and that's attached. I
don't think it's an improvement.

Vincent Povirk

On Mon, May 18, 2009 at 5:17 PM, Nikolay Sivov  wrote:
> Vincent Povirk wrote:
>
> From 1de9b2291bf9a5617ca1e3b0cba5f1260889c259 Mon Sep 17 00:00:00 2001
> From: Vincent Povirk 
> Date: Mon, 23 Mar 2009 16:34:12 -0500
> Subject: [PATCH] gdiplus: implement GdipTransformPoints
>
> It's a strange way to check enumeration with integer comparison, isn't it?
> And not clear in this particular case.
> > - if (stat == Ok)
> > - {
> > - unitscale = convert_unit(graphics->hdc, graphics->unit);
> > -
> > - if(graphics->unit != UnitDisplay)
> > - unitscale *= graphics->scale;
> > -
> > - if (src_space <= CoordinateSpaceWorld && dst_space >=
> >
> > CoordinateSpacePage)
> > - GdipMultiplyMatrix(matrix, graphics->worldtrans,
> >
> > MatrixOrderAppend);
> > - if (src_space <= CoordinateSpacePage && dst_space >=
> >
> > CoordinateSpaceDevice)
> > - GdipScaleMatrix(matrix, unitscale, unitscale,
> >
> > MatrixOrderAppend);
> > -
> > - if (dst_space <= CoordinateSpacePage && src_space >=
> >
> > CoordinateSpaceDevice)
> > - GdipScaleMatrix(matrix, 1.0/unitscale, 1.0/unitscale,
> >
> > MatrixOrderAppend);
> > - if (dst_space <= CoordinateSpaceWorld && src_space >=
> >
> > CoordinateSpacePage)
> > - {
> > - GpMatrix *inverted_transform;
> > - stat = GdipCloneMatrix(graphics->worldtrans,
> >
> > &inverted_transform);
> > - if (stat == Ok)
> > - {
> > - GdipInvertMatrix(inverted_transform);
> > - GdipMultiplyMatrix(matrix, inverted_transform,
> >
> > MatrixOrderAppend);
> > - GdipDeleteMatrix(inverted_transform);
> > - }
> > -
>
> Maybe it's better to use 'switch' for source coords with falling and ifs
> inside cases for destination.
>
> <= CoordinateSpaceWorld is just CoordinateSpaceWorld
> > = CoordinateSpaceDevice - is CoordinateSpaceDevice.

gdiplus: implement GdipTransformPoints

[Nikolay Sivov]

Vincent Povirk wrote:
> It's odd, but it seems like the simplest way to write this.
>
> I've tried what I think was your suggestion, and that's attached. I
> don't think it's an improvement.
>
> Vincent Povirk

Could be a single switch maybe. Don't know, looks better for me.
Comparisons were really ugly here - a strict comparison should be used 
instead if you don't like
switches.

Anyway it's just my thoughts.

gdiplus: implement GdipTransformPoints

[Alexandre Julliard]

Vincent Povirk  writes:
> It's odd, but it seems like the simplest way to write this.
>
> I've tried what I think was your suggestion, and that's attached. I
> don't think it's an improvement.

You should short-circuit the src_space == dst_space case, that would
simplify things a lot. Then you can probably have two switch statements,
one to do src_space -> Page and then Page -> dst_space.