Re: [PATCH] Fixes to AffineTransform

[graydon hoare]

Olga Rodimina wrote:

> Here is the patch that fixes few errors in the AffineTransform class. 
> It corrects a small error in shear transformation and fixes
> createInverse() to return correct inverse matrix.

hi olga,

this stuff looks good, thanks a lot! it's surprising this just sat there 
broken for so long. I've checked it against a few algebra texts I have 
kicking around and some geometry libraries (including cairo) and they 
seem to mostly agree with your calculation; though just to be difficult 
they all use different notation and half of them do it upside down :)

one comment: I this this "shear" function is written in a confusing way. 
I think it is slightly clearer -- to my lazy eyes -- if written as so:

    double n00 = m00 + (shy * m01);
    double n01 = m01 + (shx * m00);
    double n10 = m10 + (shy * m11);
    double n11 = m11 + (shx * m10);

though I suppose it's a matter of aesthetics. I find it easier to check 
that against what I see written elsewhere. assuming I didn't get the 
terms mixed up just now. ugh.

it might be nice to write up a mauve testsuite which generates a bunch 
of pseudorandom vectors and runs them through a bunch of pseudorandom 
scaled, sheared and translated transformations (and their inverses) and 
checks that the vectors pop out the same as they went in, within some 
epsilon to account for floating point error. anything to make this stuff 
less fidgety and prone to eyes-glossing-over would probably be good. 
it's too easy to make a minor arithmetic mistake which completely breaks it.

oh.. also just browsing the class, I notice that the comments on the 
field definitions in AffineTransform for m11 and m10 are backwards; it 
refers to m11 as the shearing element and m10 as the scaling. you might 
want to fix that when you commit.. assuming you can. did you get write 
access yet?

-graydon