In FMath::QInterpConstantTo, DeltaInterpSpeed is incorrectly clamped to 0-1 ?

Development Platform & Builds
, ,
1

Hi there,

We found out an issue in FMath::QInterpConstantTo.

in the following code:

	float DeltaInterpSpeed = FMath::Clamp<float>(DeltaTime * InterpSpeed, 0.f, 1.f);
	float AngularDistance = FMath::Max<float>(UE_SMALL_NUMBER, (float)(Target.AngularDistance(Current)));
	float Alpha = FMath::Clamp<float>(DeltaInterpSpeed / AngularDistance, 0.f, 1.f);

	return UE::Math::TQuat<T>::Slerp(Current, Target, Alpha);

DeltaInterpSpeed is clamped to 0 - 1, while AngularDistance is in radians, which means we can only move up to 1 Rad per call…?

Since Alpha is already clamped to 0 - 1, seems there’s no need to clamp DeltaInterpSpeed at all?

Looks like this function is used in motion matching code, so we want to confirm if it is intentional behavior for some reason, or if we misunderstand anything. Thank you!

2

Hello Xun Zhang,

My understanding of this function is similar to yours; I believe the extra clamps may be unneeded.

DeltaInterpSpeed shouldn’t need clamping unless someone is using “negative” time, and the check above should ensure the AngularDistance is never 0 or less.

I implemented my own function on an actor with and without the clamping to test the behavior, and achieved the same results with both methods over multiple tests.

I’ve escalated this case to Epic to see if we can gain further understanding on the intention of this behavior.

Thanks,

Kyle B.

3

Hi [mention removed]​, thanks for raising this issue. I also agree that the code looks incorrect. It should be:

float DeltaInterpSpeed = FMath::Max<float>(0.f, DeltaTime * InterpSpeed);
	float AngularDistance = FMath::Max<float>(UE_SMALL_NUMBER, (float)(Target.AngularDistance(Current)));
	float Alpha = FMath::Clamp<float>(DeltaInterpSpeed / AngularDistance, 0.f, 1.f);

I’ll talk with the dev team about this and look to get a fix committed. The main concern about making this change will be whether anyone was relying on the previously clamped behaviour. But the only engine code relying on it is the Pose Search code so you are good to make the change locally.

4

Thanks for taking a look!

We believe that Pose Search’s behavior with default settings may depend on the clamping.

​

In PoseSearchTrajectoryLibrary.cpp:

CurrentFacingWS = FMath::QInterpConstantTo(CurrentFacingWS, CurrentAccelerationCS.ToOrientationQuat(), SecondsPerPredictionSample, TrajectoryData.RotateTowardsMovementSpeed);SecondsPerPredictionSample = 0.4 by default

TrajectoryData.RotateTowardsMovementSpeed = 10 by default (perhaps intended to be in degrees?)

​

Thus if unclamped, Pose Search’s generate trajectory will go from 1 -> 4 RAD per sample with the default settings.

5

Can you point me to where you see the default for SecondsPerPredictionSample being set to 0.4? I could only see it being set to 0.2 (as a default argument to UPoseSearchTrajectoryLibrary::PoseSearchGenerateTrajectory and lower values elsewhere).

The point that you make is correct, though. We do need to compensate for removing the clamp from QInterpConstantTo by changing any dependent code. For runtime code, it’s unlikely there will be any problem since the sampling interval is usually based on frame time, which is small enough that DeltaInterpSpeed is unlikely to exceed 1. But with the pose search trajectory generation code, the sampling values are higher so DeltaInterpSpeed can exceed 1.

6

Thanks for following up. I’m talking with the dev team about committing the fix I mentioned previously. Assuming they are happy with it, it’ll go into the 5.8 release.

7

You’re right, that was my mistake, it is 0.2 by default.