Skip to content

Conversation

@mbredif
Copy link
Contributor

@mbredif mbredif commented Jun 22, 2018

Previous implementation relied on average quaternions (t=0.5) for small angles.
Now it uses the same approach as Quaternion.slerpFlat (same threshold and lerp interpolation).

This PR tries to address #14258 and iTowns/itowns#774

Previous implementation relied on average quaternions (t=0.5) for small angles.
Now uses the same approach as Quaternion.slerpFlat (threshold and lerp)
@WestLangley
Copy link
Collaborator

slerp() and slerpFlat() should not be using different algorithms. We will have to fix that.

Be that as it may, perhaps you should add this.normalize() after lerping; lerp() will not maintain unit length.

In fact, can you please check the consequence of failing to normalize in your app?

var sqrSinHalfTheta = 1.0 - cosHalfTheta * cosHalfTheta;

if ( Math.abs( sinHalfTheta ) < 0.001 ) {
if ( sqrSinHalfTheta <= Number.EPSILON ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Math.abs() is no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no : a square is always non negative !
do you prefer a update of this PR with a call to normalize() or a simply implementing slerp with a subcall to slerpFlat ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If cosHalfTheta = - 1.00001 due to roundoff then sqrSinHalfTheta could be negative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but that is no longer an issue : negative sqrSinHalfTheta corresponds to cases where the cos is round off errors away from 1 or -1, which corresponds to tiny values of the sin, for which lerp makes sense. (it WAS however an issue as the sqrt was not guarded against negative values and would output NaN values...)

@WestLangley
Copy link
Collaborator

Thank you @mbredif.

@mrdoob This can be merged.

@mrdoob mrdoob modified the milestones: r95, r94 Jun 26, 2018
@mrdoob mrdoob merged commit d106dde into mrdoob:dev Jun 26, 2018
@mrdoob
Copy link
Owner

mrdoob commented Jun 26, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants