TRotationMatrix<T>::MakeFromY function produces incorrect tranform matrix

Development Platform & Builds
, ,
1

Hi there,

We found a bug in the implementation of the TRotationMatrix<T>::MakeFromY function. It is assigning the inverted X axis as the NewZ axis, making the NewX axis point in the direction of the UpVector.

template<typename T>
TMatrix<T> TRotationMatrix<T>::MakeFromY(TVector<T> const& YAxis)
{
	TVector<T> const NewY = YAxis.GetSafeNormal();

	// try to use up if possible
	TVector<T> const UpVector = (FMath::Abs(NewY.Z) < (1.f - UE_KINDA_SMALL_NUMBER)) ? TVector<T>(0, 0, 1.f) : TVector<T>(1.f, 0, 0);

	const TVector<T> NewZ = (UpVector ^ NewY).GetSafeNormal();
	const TVector<T> NewX = NewY ^ NewZ;

	return TMatrix<T>(NewX, NewY, NewZ, TVector<T>::ZeroVector);
}

The amended implementation would look like this, which would match the MakeFromYZ function:

template<typename T>
TMatrix<T> TRotationMatrix<T>::MakeFromY(TVector<T> const& YAxis)
{
	TVector<T> const NewY = YAxis.GetSafeNormal();

	// try to use up if possible
	TVector<T> const UpVector = (FMath::Abs(NewY.Z) < (1.f - UE_KINDA_SMALL_NUMBER)) ? TVector<T>(0, 0, 1.f) : TVector<T>(1.f, 0, 0);
	
	const TVector<T> NewX = (NewY ^ UpVector).GetSafeNormal();
	const TVector<T> NewZ = NewX ^ NewY;

	return TMatrix<T>(NewX, NewY, NewZ, TVector<T>::ZeroVector);
}

Kind regards

2

Hi,

I’ve forwarded this to the folks responsible for our Math library and they’ve had a look and determined that while not immediately obvious, this behavior is not a bug.

Here’s their full message:

While I agree that this is an unfortunate implementation choice, it is a not a bug. The doc comment for MakeFromY() is very clear on this:

/** Builds a rotation matrix given only a YAxis. X and Z are unspecified but will be orthonormal. YAxis need not be normalized. */
[[nodiscard]] static TMatrix<T> MakeFromY(TVector<T> const& YAxis);

There’s absolutely no guarantees on what the resulting X and Z axes will be; this function makes an arbitrary choice. The only guarantee is that the matrix’s Y axis matches YAxis, which it does.If you care about what goes into the X or Z rows of the resulting matrix, this is not the function to use, and there’s a high bar to clear to change the behavior of core math library functions, which have a high likelihood of breaking other code that implicitly relies on the existing results for the X and Z rows of the matrix. (Which it also shouldn’t, but these things just happen.)

You already have a function that implements the particular behavior you want. My recommendation would be to just move that to a shared utility header file and use it in your code that wants the X and Z axes to align in this specific way.

We’re very cautious about changing existing behavior of functions after the fact, so we currently prefer to keep the implementation as is. Feel free to change it in your engine build if you rely on this behavior on the engine side.

Kind Regards,

Sebastian