Matrix3::inverse Matrix4::Inverse

[madmarx]

hello,

I don't know if it was already corrected, but there is :

Matrix4 Ogre::Matrix4::inverse ( ) const
and
Matrix3 Ogre::Matrix3::Inverse ( Real fTolerance = 1e-06 ) const
and
bool Ogre::Matrix3::Inverse (Matrix3 &rkInverse, Real fTolerance=1e-06) const
and
Quaternion Ogre::Quaternion::Inverse ( )

So a few lowercase/uppercase things.

BTW, not all matrix are inversible, so I was wondering if there is a need for a bool somewhere (as an outputed reference) ?

There is something I find quite nasty too, but it's rather different :
Real Ogre::Vector3::normalise( ) .

This does not return the normalised vector, but a real.
Ogre::Vector3 lVal = somevector3.normalise() ;
will create a vector that is only unit_scale * lenght of somevector3.

Obviously things would be clearer if :
when this is a calculation on the object => prefix make (like makeFloor)
when this is a calculation returning something => prefix get (like getRotationTo)
But this would destroy completely tons of code if we do that. So we could make it so that the compiler goes crazy if we do something wrong (so get rid of Vector3 (const Real scaler), and use a static func instead, this constructor is dangerous anyway as it is ).

What do you think?

[jacmoe]

Keep old versions, add new ones while flagging the old ones a deprecated?
Then get rid of them when people have gotten used to the new way..

[Kojack]

The matrix/quaternion inverse functions (mixed case) were in my post yesterday: http://www.ogre3d.org/forums/viewtopic.php?f=22&t=62638

Having a normalise which returns the length of the vector isn't that uncommon. Ogre has normalise() (modify the vector) and normaliseCopy() (return a new vector).
A few other libraries which have vector normalise return a float length:
- PhysX
- Havok
- Wild Magic

[madmarx]

@kojack

The matrix/quaternion inverse functions (mixed case) were in my post yesterday: viewtopic.php?f=22&t=62638

Sorry I did not check the recent previous papercuts .

Having a normalise which returns the length of the vector isn't that uncommon.

I had not realized that, thanks for the examples.
Anyway, I am wondering if the returned lenght of such normalise() function is really used ? I don't see when it would be interesting (even for ray collision).

[CABAListic]

It comes at no cost since the normalise function has to calculate the vector length, anyway. In the off chance that you do need the length, you can save a few instructions by not having to calculate the length separately before the normalise.

[madmarx]

It comes at no cost since the normalise function has to calculate the vector length

I know that. But if it never happens that you need it, that is not a valid argument . I wonder if this need ever happened before.
What is more, there is a cost in the API because, as explained above, it can be mistaken with something else.

Ex of the error:

Code: Select all

Quaternion q1;
Quaternion q2;
q2 = q1.Inverse(); // so I suppose a function in the form of Inverse() always return a quaternion... (not applying to it)
Vector3 v1;
Vector3 v2;
v2 = v1.perpendicular(); // ok, same layout for the function call as above.
v2 = v1.normalise(); // This compiles, unfortunately, this is absolutely not the same behavior as above, because this time, v1 is changed..., and v2 != of the normalized v1.

If someone really needs to normalise and get the lenght, this could be done with a different interface, which would garantee the compiler detects it (like void normalise(float*outputLenght=0)...).

[CABAListic]

Well, the fault here is that Vectors can be initialised from a single float which, imho, is very wrong. Such a constructor should not exist or, at the very least, be made explicit. Then the above code would not compile.

[madmarx]

Yes completely!

opening post :

get rid of Vector3 (const Real scaler), and use a static func instead, this constructor is dangerous anyway as it is

So this is a valid papercut?

[CABAListic]

Absolutely.