On Tue, 28 Jan 2020 19:19:57 GMT, Nir Lisker <nlis...@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. I'll look at it in more detail, but had two questions come up while looking at the fix. One is related to the fix, and one is preexisting. 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. ------------- PR: https://git.openjdk.java.net/jfx/pull/98
