loodakrawa / SpriterDotNet

A pure C# Spriter implementation
zlib License
220 stars 75 forks source link

MathHelper.GetFactor division by zero for duplicate key #98

Closed rfadeev closed 6 years ago

rfadeev commented 6 years ago

According to Spriter staff, it's valid to have duplicate key in the timeline. Imagine having following timeline:

            <timeline id="2" name="my_sprite_name">
                <key id="0" spin="0">
                    <object folder="0" file="2" x="-13.830971" y="-4.577502" pivot_x="0.640495" pivot_y="0.51401" angle="0"/>
                </key>

                <!-- Other keys -->

                <key id="6" time="1200" spin="0">
                    <object folder="0" file="2" x="79.290733" y="-19.161719" pivot_x="0.640495" pivot_y="0.51401" angle="54.488976"/>
                </key>
                <key id="7" time="1200" spin="-1">
                    <object folder="0" file="2" x="64.972944" y="-19.838683" pivot_x="0.640495" pivot_y="0.51401" angle="54.488976"/>
                </key>
            </timeline>

This scml results in NaN being returned by MathHelper's GetFactor method when animation time reaches animation length (i.e. Time property in Animator is 1200 and deltaTime passed to Animate is nonzero) due to division by zero. This happens due to combination of duplicate key being at the end of animation and first key being at the start of it.

Step by step:

  1. FrameDataCalculator.GetFrameData calls SpriterHelper.AdjustTime. Passed keyA is key with id 6, keyB is key with id 0
  2. SpriterHelper.AdjustTime calls SpriterHelper.GetFactor, passed keys are the same
  3. In SpriterHelper.GetFactor calculated timeA is equal to timeB after this if statement.

I've implemented the fix though would like to confirm first that this is a bug since the fix goes beyond fixing the problematic if statement in SpriterHelper as there are other places I had to change to remove this division by zero:

Found the case by using SpriterDotNetUnity - Unity reported errors like due to this.

transform.localPosition assign attempt for *** is not valid. Input localPosition is { NaN, NaN, 0.000000 }.
loodakrawa commented 6 years ago

Thanks for the very detailed report.

I do believe this is indeed a bug. Looks like an edge case and I the solution you mentioned looks good so please submit a PR if you have a fix.

Thanks!