RFR: 8237975: Non-embedded Animations do not play backwards after being paused

[Nir Lisker]

On Wed, 29 Jan 2020 20:47:11 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> [8236858](https://bugs.openjdk.java.net/browse/JDK-8236858) (Animations do not play backwards after being paused) has been split to deal with [embedded](https://bugs.openjdk.java.net/browse/JDK-8237974) and [not embedded](https://bugs.openjdk.java.net/browse/JDK-8237975) animations. This is a fix for the latter.
>> The reason for the split is that embedded animations have a much more complex behavior. The current state of the relation between an animation and its clip envelope is already messy and should be corrected, even more so for embedded animations whose parent controls their behavior as well (sometimes in conflict with the child's clip envelope). This will require a redesign which can be discussed for 15. See the parent issue [8210238](https://bugs.openjdk.java.net/browse/JDK-8210238) for the list of bugs that arise from it.
>> 
>> This simple fix allows to change the current rate of a `ClipEnvelope` also when the animations is `PAUSED`. A possible issue with this approach is that it changes the buggy behavior of embedded animations to a different buggy behavior.
>> 
>> A concept test has been added, but it does not work yet since the mock clip envelope does not have sufficient behavior (`doTimePulse` does not actually do a time pulse). Open for ideas on how to make it simple, otherwise I will add a method to set a clip envelope and create a new one ad-hoc.
> 
> modules/javafx.graphics/src/main/java/com/sun/scenario/animation/shared/FiniteClipEnvelope.java line 85:
> 
>> 84:         if (status != Status.STOPPED) {
>> 85:             setInternalCurrentRate((Math.abs(currentRate - this.rate) < EPSILON) ? rate : -rate);
>> 86:             deltaTicks = newTicks - Math.round((ticks - deltaTicks) * Math.abs(rate / this.rate));
> 
> What are the implications of not calling the existing `setCurrentRate` method for animations that are in the `RUNNING` state? It means that `Animation::setCurrentRate` will no longer be called in that case. Should it be?
> 
> Independent of your change, I can't figure out why the existing logic works today, even for animations that are running. The expression `(Math.abs(currentRate - this.rate) < EPSILON) ? rate : -rate` will evaluate either to `rate` or `-rate` depending on whether `currentRate` and `this.rate` are closer to each other than epsilon, irrespective of whether either (or both) of `this.rate` or `currentRate` are positive or negative. I feel I must be missing something.

> What are the implications of not calling the existing `setCurrentRate` method for animations that are in the `RUNNING` state? It means that `Animation::setCurrentRate` will no longer be called in that case. Should it be?

The situation is complicated (to put it politely), as you've realized. `currentRate` is actually being set from `Animation#setRate` first, then `ClipEnvelope.setRate` sets it again. I initially removed `currentRate` from `ClipEnvelope` altogether, but that broke embedded animations whose rate is being set by their parent through their `ClipEnvelope` (doing it through `Animation` will throw an exception). So, instead, I added a "backdoor" for the parent to change its child's `ClipEnvelope.currentRate` without changing its `Animation.currentRate`.

The result is that non-embedded animations now work correctly and embedded animations are still broken, but in a slightly different way. Their `Animation.currentRate` is now always 1 instead of either 1 or -1, but this is incorrect anyway if the rate is not 1 or -1.

> Independent of your change, I can't figure out why the existing logic works today, even for animations that are running. The expression `(Math.abs(currentRate - this.rate) < EPSILON) ? rate : -rate` will evaluate either to `rate` or `-rate` depending on whether `currentRate` and `this.rate` are closer to each other than epsilon, irrespective of whether either (or both) of `this.rate` or `currentRate` are positive or negative. I feel I must be missing something.

There's a lot I couldn't figure out in time for 14, especially with regard to embedded animations (have a look at `ParallelTransition.doPlayTo`).

For non-embedded animations, If the animation is `RUNNING`, then:
* If the animation is during a forward cycle then `currentRate == this.rate`, so the current rate is set to the new rate.
* If the animation is during a reverse cycle, then `currentRate == -this.rate`, so the current rate is set to the negative of the new rate (to accommodate the reverse cycle's rule of `currentRate == -rate`).

However, this check is already done in `Animation.rate`'s `invalidate` method, where the cryptic `Math.abs(currentRate - this.rate) < EPSILON` is at least given the name `playingForward`:
if (getStatus() == Status.RUNNING) {                                             
    final double currentRate = getCurrentRate();                                 
    if (Math.abs(currentRate) < EPSILON) {                                       
        doSetCurrentRate(lastPlayedForward ? newRate : -newRate);                
        resumeReceiver();                                                        
    } else {                                                                     
        final boolean playingForward = Math.abs(currentRate - oldRate) < EPSILON; // HERE
        doSetCurrentRate(playingForward ? newRate : -newRate);  // AND HERE
    }                                                                            
}                                                                                
oldRate = newRate;                                                               

For embedded animations this is even worse, since there is an additional rate property kept in the parent animations (`rates[i]`), so the rate is kept in 3 places...

Overall, the animation area will require a soft redesign. That might weed out the bugs in bulk. The more I was working to find a fix the more bugs I found. I turned [JDK-8210238](https://bugs.openjdk.java.net/browse/JDK-8210238) to an umbrella issue to keep track.

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

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