Bug/single keyframe exception

kevzettler commented 5 years ago

Hey @chinedufn I think I have encountered a unhandled exception that occurs if currentAnimation.keyframes only has a single record

TypeError: Cannot read property '0' of undefined
    at /Users/kev/code/skeletal-animation-system/src/skeletal-animation-system.js:122:84
    at Array.reduce (<anonymous>)
    at Object.interpolateJoints (/Users/kev/code/skeletal-animation-system/src/skeletal-animation-system.js:85:43)
    at Test.<anonymous> (/Users/kev/code/skeletal-animation-system/test/blend-out-previous-anim.js:508:19)
    at Test.bound [as _cb] (/Users/kev/code/skeletal-animation-system/node_modules/tape/lib/test.js:66:32)
    at Test.run (/Users/kev/code/skeletal-animation-system/node_modules/tape/lib/test.js:85:10)
    at Test.bound [as run] (/Users/kev/code/skeletal-animation-system/node_modules/tape/lib/test.js:66:32)
    at Immediate.next [as _onImmediate] (/Users/kev/code/skeletal-animation-system/node_modules/tape/lib/results.js:71:15)
    at runCallback (timers.js:781:20)
    at tryOnImmediate (timers.js:743:5)

Stems from: https://github.com/chinedufn/skeletal-animation-system/blob/master/src/skeletal-animation-system.js#L122

when currentAnimLowerKeyframe is undefined

I have successfully patched by adding a guard above line: https://github.com/chinedufn/skeletal-animation-system/blob/master/src/skeletal-animation-system.js#L57

That checks if there is only 1 keyframe and sets currentAnimLowerKeyframe to that.

This PR includes a failing test that reproduces.

Let me know your thoughts and I can add a commit with the fix for review.

chinedufn commented 5 years ago

Thanks for the failing test!

Your fix idea sounds good! - mind also just pairing down the failing test to only have one joint? So basically jointNums: 0 and only one array in keyframes: {'0.0'" ...}

kevzettler commented 5 years ago

thanks, @chinedufn have added commits with the fix and your suggestions.

chinedufn commented 5 years ago

Awesome LGTM!

chinedufn commented 5 years ago

Mind releasing a new patch version to npm ?

kevzettler commented 5 years ago

@chinedufn sure thing. Think I need access to the npm repo though

chinedufn commented 5 years ago

added you!

kevzettler commented 5 years ago

@chinedufn ok I have published 0.8.1 but cannot push the git tag to this repo

chinedufn / skeletal-animation-system

Bug/single keyframe exception #14