Affine transforms - matrix algebra

[Pavel Safrata]

First, Richard, are you satisfied with my answers regarding the event, 
clone and append*, or did you read only Jim's response with those parts 
of my email trimmed?

On 17.8.2012 3:05, Richard Bair wrote:
> On Aug 16, 2012, at 3:52 PM, Jim Graham wrote:
>
>> On 8/16/2012 10:38 AM, Pavel Safrata wrote:
>>> On 16.8.2012 16:29, Richard Bair wrote:
>>>>> 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.
>> In particular, Non isn't a word so why should it be separated (via capitalization) from the word to which it is a prefix?
> Well, noninvertible isn't a word (at least, according to my dictionary), and I don't know that non-invertible would be represented as Noninvertible when converted to camel-case?

In javafx-beans we have NonNull and NonIterableChange, so the capital 
"I" would be more consistent with the existing FX API.

>
>>>>> public double determinant()
>>>> Why isn't this a getter?
>>> Because currently it is not a property.
>> I'm not sure one would want to track it or bind to it either.
>>
>>>>> 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:
>> Or should they?  What is the use case for tracking or binding to these?  You can already listen for events to know when things change.  These are all more for "I want to know how to most quickly handle this transform" and are only useful for optimization cases where you still need to write the general case.  Since you always need the general case and since these attributes are only needed when you actually get to the "I'm dealing with the transform values now" code, why do they need to be properties?
>>
>> I think properties should be things that are independently specifiable.  Values that you calculate from other properties shouldn't necessarily be properties themselves (though it might make sense in some cases - you should provide a use case to argue for that rather than just have a "wouldn't it be great if everything were a property" default approach).
>>
>> Why don't we make hashcode a property?  ;)
> In fact, it is! hashcode follows precisely the pattern for immutable properties -- it is just a getter. If something can change, somebody is going to want to know about it. Listening to changes via an event object is OK if you don't care what exactly has changed, but cumbersome when you want to know exactly what has changed. In general though, the issue I have is that a getFoo() should either be immutable, or should have a fooProperty() method. The reason I have wanted to maintain this pattern is that it allows tools / frameworks to deduce things about properties on objects by the patterns they follow.
>
> Alternatively, we could (I suppose) have taken to annotating everything to indicate whether they are mutable / immutable etc. The way we've worked around this in other places where we didn't want something to be observable was to just not prefix it with get / is, but like with List use a simple name, such as size(). So "determinant" is OK if we decide we don't want to make it observable for terms of weight. The lame part about such names though is that some names like "identity" are not clear as to whether they are reading the identity or making it an identity transform.

Here is my proposal updated based on your comments:

isIdentity() - I'm now not sure about it. It could be useful for example 
if you retrieve a local-to-scene transform and want to know if your node 
is transformed or not. It seems to me it doesn't have much use to 
observe it, but as Richard said, identity() is a bad name. I didn't 
succeed to find a better name (I considered something like nonTrivial() 
or modifiesCoordinates() but those are ugly with unclear meaning). So I 
would probably make the identityProperty() from it.

determinant() - I would leave it like that, because
- it's similar to e.g. point's magnitude()
- very few users will need that because now the class will do all of the 
math with it for them
- the property would be invalidated at the same time as the 
TransformChangedEvent will be sent if you really need to observe it..

is2D()/is3D() is probably useful to observe - a lot of things changes 
when a transform becomes 3D. So I'm repeating my proposal deleted by Jim:
twoDimentionalProperty(), isTwoDimensional()

>
>>>> 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.
>> With regard to using the varargs notation for the setters:
>>
>> I think that would be interesting and fun.  On the other hand, we might be better served by having methods that explicitly take the N parameters since we then avoid unnecessary object creation.  Also, these methods do not take an arbitrary number of doubles, they take a very explicit number of doubles based on the matrix type they are considering so the flexibility of the varargs notation is a little overkill when it is really just a lower performing way to allow inline argument list syntax.
> If you're going to provide an array, there is no difference in performance or semantics between varargs or []. Avoiding an array all together (and, ostensibly the MatrixType argument and offset argument) at least has the benefit of not requiring any bounds checking etc within the constructor, so I can see an argument for having only that.

The methods that take arrays of doubles were requested by users for 
interoperability with other libraries (e.g. physics) which often support 
the array conversions.

>
> I can see Pavel tearing his hair out though. This is the longest API review thread ever!

Anything for producing a good API :-)

>
>>> 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?
>> Unfortunately I don't have a strong opinion there either, but if potential varargs usage is one of the key reasons for moving the array to the end, then I think I've given some good counter-reasons above...
> Having parameters in a more-or-less consistent order was really what I was after. Varargs just fell out of it, but wasn't a really useful reason in and of itself.

There are methods that take directly the values, the array-accepting 
methods won't usually be called with an inlined array literal but with a 
variable. So I think that Richard's argument about the nicer code is not 
really important because mostly it will be

new Affine(arrayGotFromSomewhere, MatrixType.MT_3D_4x4, 0);

I tried to explain why I think my proposal is consistent, any comments 
to that?
Thanks,
Pavel

>
> Richard