RFR: 8241582: Infinite animation does not start from the end when started with a negative rate

[Nir Lisker]

On Wed, 22 Apr 2020 00:08:05 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> ### Cause
>> 
>> `Animation#jumpTo(Duration)` does not handle `Duration.INDEFINITE` properly. This causes
>> `InfiniteClipEnvelope#jumpTo(long)` to receive an erroneous value and start the animation not from the end, depending
>> on the modulo calculation.  ### Fix
>> 
>> For infinite cycles, use the `cycleDuration` as the destination since that is the effective end point.
>> 
>> ### Tests
>> 
>> * Added an `testJumpTo_IndefiniteCycles` test that fails without the patch and passes after it.
>> * Added a `testJumpTo_IndefiniteCycleDuration` that passes both before and after, just to make sure that this type of
>>   infinite duration is correct.
>> * Removed a test in `SequentialTransition` that fails with this patch, but it passed before it only because the modulo
>>   value happened to land in the right place. Changing the duration of one of the child animation can cause it to fail, so
>>   the test is faulty anyway at this stage.
>> 
>> ### Future work
>> 
>> Playing backwards will still not work correctly, but at least now it start from the correct value. This is the first of
>> a series of fixes under the umbrella issue [JDK-8210238](https://bugs.openjdk.java.net/browse/JDK-8210238).
>
> modules/javafx.graphics/src/test/java/test/javafx/animation/AnimationTest.java line 271:
> 
>> 270:         animation.jumpTo("end");
>> 271:         assertEquals(Duration.millis(Long.MAX_VALUE / 6), animation.getCurrentTime());
>> 272:     }
> 
> Why `/ 6` ?

This is a good question and I think I should have explained it in a comment because it also points to a mini-flaw. The
short answer is that the 6 comes from the `TicksCalculation` class, which defines `TICKS_PER_SECOND = 6000` and
`TICKS_PER_MILI = TICKS_PER_SECOND / 1000.0` which is 6.

The test works as follows:

This is the "ticks from duration" calculation for jumping to the end (`Duration.INDEFINITE`), demonstrated by
mathematical substitutions: double millis = Duration.INDEFINITE.toMillis() = Double.POSITIVE_INFINITY
long ticks = TickCalculation.fromMillis(millis) = Math.round(TICKS_PER_MILI * millis)
        = Math.round(6 * Double.POSITIVE_INFINITY) = Math.round(Double.POSITIVE_INFINITY)
        = Long.MAX_VALUE (as per the spec. of Math.round)
Notice that we lose precision when multiplying `POSITIVE_INFINITY`.

Then getting the current time calculates "duration from ticks":

animation.getCurrentTime() = TickCalculation.toDuration(currentTicks)
        = ticks / TICKS_PER_MILI = Long.MAX_VALUE / 6

So the division carries out normally (there's no `Long.POSITIVE_INFINITY`), but the multiplication does not.

Maybe we shouldn't convert between the `double`-based `Duration` and the `long` ticks so much, but I think that this is
somewhat negligible.

My next patch is refactoring in preparation of the next, heavier, changes, so I will add this explanation on the test.

-------------

PR: https://git.openjdk.java.net/jfx/pull/169