Affine transforms - matrix algebra

[Pavel Safrata]

Hi Richard,
thanks for your comments!

On 16.8.2012 16:29, Richard Bair wrote:
> Hi Pavel,
>
> Here are my thoughts:
>
>> New NoninvertibleTransformException
> I believe it should be NonInvertibleTransformException with a capital "I".

Both in J2D and in our internal geom packages the "i" is lower-case. But 
I have no problem changing it.

>
>> New TransformChangedEvent with single event type TRANSFORM_CHANGED
> This event goes on the Transform object itself, and would replace our usage of impl_add, impl_remove and such. Is that right?

It goes on the Transform object itself. However, it doesn't replace 
impl_add or impl_remove. The event has no data and it is meant for users 
just to say "hey, this transform has changed some of its parameters". 
Right now if you need a method "when the matrix changes, do some 
computation with it", you need to register listeners to all 12 elements 
(for Affine). Even worse, if you have a Transform, you would have to do 
instanceofs and register listeners to all the possible properties of all 
the different Transform subclasses). This event is meant to remove this 
crazy necessity and notify users when "something changes".

>
>> On Point2D:
>>     // similar to Point3D mapped to 2D (except of crossProduct still returning Point3D)
> Do Point2D and Point3D have a common base class?

Currently not.

>
>>     public Transform clone()
>>         // Of course Transform implementing Cloneable
> Why does it have to implement clone?

To allow copying the transform (without sharing properties with the old 
one).

>
>>     public Point3D deltaTransform(Point3D point)
>>     public Point2D deltaTransform(Point2D point)
>>         // transformation without translation (transformation of a vector)
>>     public Point3D inverseDeltaTransform(Point3D point)
>>     public Point2D inverseDeltaTransform(Point2D point)
>>         // inverse transformation without translation
> I haven't followed the entire thread, but this was the first part of the API who's meaning wasn't immediately obvious. What is a delta transform, and what is it used for?

It is transformation without translation. In another words, it is 
transformation of a vector. Unfortunately we have point and vector both 
represented by a single class (PointND), but point is transformed 
differently than vector. So deltaTransform transforms PointND as if it 
was a vector. This approach is copied from J2D.

If we had a Vector3D, we could replace deltaTransform(Point3D) with 
transform(Vector3D). But there is the backward compatibility issue with 
Rotate and its axis, which is Point3D although it is a vector.

>
>>     public double determinant()
> Why isn't this a getter?

Because currently it is not a property.

>
>>     public boolean is2D()
>>     public boolean is3D()
>>     public boolean isIdentity()
> All four of these, determinant, is2D, is3D, and isIdentity can all (in theory) change their value as the Transform is changed, is that right? In that case, they should be read only properties.

Ah, they probably should. So:

isIdentity() - I think I would abandon this one. Each property that 
needs to be invalidated with each change slows everything down and I 
don't think this one is that useful, I think it was added rather for 
completeness.

determinant() - let's have determinantProperty() and getDeterminant().

is2D()/is3D() always return the opposite value, so it should probably be 
only one property. It cannot start with a number so the naming will not 
be that simple. What about this:
twoDimentionalProperty(), isTwoDimensional()

>>     public double getElement(MatrixType type, int row, int column)
>>
>> On Affine class:
>>
>>     public Affine(double[] array, MatrixType type, int beginIndex)
> In some methods in Transform, MatrixType is first, but in Affine, it is second with the array being the first parameter. I think I would put the array as the last parameter and have MatrixType first. The reason is that this code:
>
> new Affine(new double[] {
>      1, 1, .5, 1,
>      3, 2, 1, 0,
>      1, 1, .5, 1,
>      3, 2, 1, 0}, MatrixType.MT_3D_4x4, 0);
>
> is uglier than:
>
> new Affine(MatrixType.MT_3D_4x4, 0, new double[] {
>      1, 1, .5, 1,
>      3, 2, 1, 0,
>      1, 1, .5, 1,
>      3, 2, 1, 0});
>
> It is a minor thing but it brings peace to the heart :-). Of course with the array as the last parameter, it also means it could be a double... if you wanted to, but I'm not sure it matters.

First, this is not only issue of the constructor, but of all the methods 
that accept the array of doubles. I was thinking about the order too. I 
chose this one because the beginIndex is insignificant and in a vast 
majority of cases it will be 0, so it seemed strange to have it before 
the actual important data. Also all the methods where the type is first 
are kind of getters, you just pass in what you want to get, whereas to 
the kind of setters you pass the data and than tell how to interpret 
them, so it is not really inconsistent. So your order takes a bit of 
peace from my heart, but I don't insist on the proposed order. Any other 
opinions?

>
>>     public void appendTranslation(double tx, double ty)
>>     public void appendTranslation(double tx, double ty, double tz)
>>     public void appendScale(double sx, double sy)
>>     public void appendScale(double sx, double sy, double pivotX, double pivotY)
>>     public void appendScale(double sx, double sy, Point2D pivot)
>>     public void appendScale(double sx, double sy, double sz)
>>     public void appendScale(double sx, double sy, double sz, Point3D pivot)
>>     public void appendRotation(double angle)
>>     public void appendRotation(double angle, double pivotX, double pivotY)
>>     public void appendRotation(double angle, Point2D pivot)
>>     public void appendRotation(double angle,
>>                                double pivotX, double pivotY, double pivotZ,
>>                                double axisX, double axisY, double axisZ)
>>     public void appendRotation(double angle,
>>                                double pivotX, double pivotY, double pivotZ,
>>                                Point3D axis)
>>     public void appendRotation(double angle, Point3D pivot, Point3D axis)
>>     public void appendShear(double shx, double shy)
>>     public void appendShear(double shx, double shy, double pivotX, double pivotY)
>>     public void appendShear(double shx, double shy, Point2D pivot)
>>     public void prepend* // 20 methods
> I don't know that it is worth it to add these 32 methods. Somebody trying to eek performance and skilled in the art can just use the long form append method, and somebody who is just hacking away where performance isn't a concern can do:
>
> t.append(Transform.translate(5, 10));
>
> It create a short-lived object, yes, but if that bothered me I could instead use the append(one billion parameters) method call instead. Do you think having these convenience methods is compelling enough to justify their addition?

I think it is. The methods are highly optimized. Some of them have 
hundreds of lines of code. If you wanted to use the 
one-billion-parameter method, you would have to do all the math yourself 
(which defeats the purpose of whole this API) and still it wouldn't be 
nearly that fast because we do take huge advantage of stored internal 
state of the matrix.

Thanks,
Pavel

>
> Thanks
> Richard