On Wed, 29 Jan 2020 20:47:11 GMT, Kevin Rushforth <k...@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
