fu5ha / ultraviolet

A wide linear algebra crate for games and graphics.
https://crates.io/crates/ultraviolet
750 stars 81 forks source link

Slerp between Rotor3's doesn't always takes shortest path #152

Closed G0BL1N closed 1 year ago

G0BL1N commented 1 year ago

When animating a model exported from most graphical editors, they assume that interpolation between two quaternions in keyframes should always take shortest path, despite their dot products having different signs. The problem is explained here, better than I ever could. To accommodate for this, additional condition should be added to slerp. Here's a pseudocode from gltf tutorial, for example:

    Quat slerp(previousQuat, nextQuat, interpolationValue)
        var dotProduct = dot(previousQuat, nextQuat)

        //make sure we take the shortest path in case dot Product is negative
        if(dotProduct < 0.0)
            nextQuat = -nextQuat
            dotProduct = -dotProduct

        //if the two quaternions are too close to each other, just linear interpolate between the 4D vector
        if(dotProduct > 0.9995)
            return normalize(previousQuat + interpolationValue(nextQuat - previousQuat))

        //perform the spherical linear interpolation
        var theta_0 = acos(dotProduct)
        var theta = interpolationValue * theta_0
        var sin_theta = sin(theta)
        var sin_theta_0 = sin(theta_0)

        var scalePreviousQuat = cos(theta) - dotproduct * sin_theta / sin_theta_0
        var scaleNextQuat = sin_theta / sin_theta_0
        return scalePreviousQuat * previousQuat + scaleNextQuat * nextQuat

However, in your library when using slerp on rotors, path selection depends on the dot product sign, which differs from the code that should be used to interpolate between quaternions, in, for example, gltf. But as I understand it, it's not just gltf, it's all 3d formats, but I could be wrong about that. Either way, it sadly makes ultraviolet unusable for animating gltf models. My quick&dirty fix was adding this:

                let (end, dot) = if dot < 0.0 {
                    (end * -1.0, -dot)
                } else {
                    (end, dot)
                };

To the slerp method. But I honestly don't know if it's idiomatic or how it performs. Maybe there should be another interpolation method that includes this, if performance is concern? If you think my solution is good enough, I could submit a PR, but I really don't know.

fu5ha commented 1 year ago

Interesting. I'm fine with adding that bit of logic to the slerp method on Rotor3 -- though think it would probably be better to just make end and dot mut and negating them instead of shadowing them (in all likelihood shouldn't matter anyhow).

G0BL1N commented 1 year ago

Thanks for a quick reply, I created a pull request #153 with proposed changes.

fu5ha commented 1 year ago

Fixed by #153