Then I will take approach 1 for now. Remind me again, which JBS bug you are planning to use to fix this issue?
https://bugs.openjdk.java.net/browse/JDK-8236858 On Wed, Jan 22, 2020 at 1:31 AM Kevin Rushforth <kevin.rushfo...@oracle.com> wrote: > This got buried in my inbox, and I missed seeing it. > > I'll respond inline. > > > On 1/10/2020 6:45 PM, Nir Lisker wrote: > > I forgot to bring up https://bugs.openjdk.java.net/browse/JDK-8200206, > which is also relevant to this discussion. The solution to that one is > probably allowing only 1 parent, similar to the scenegraph policy. That > will avoid parent animations fighting over the state of common children > animations. > > > Good point. Regardless of which approach (1 or 2) we go with, I agree that > allowing only a single parent transition is best. I note that this would be > a semantic change, though, so even though we ought to decide what we want > the behavior to be, actually enforcing a single parent is probably best > left to JavaFX 15 (I note that > https://bugs.openjdk.java.net/browse/JDK-8200206 is targeted to 15). > > On Sat, Jan 11, 2020 at 3:08 AM Nir Lisker <nlis...@gmail.com> wrote: > >> What happens if you set the rate of the parent transition and then set >>> the rate of a child animation? >>> >> What if you bind to the rate of a child animation? >> >> >> There is already some code that touches this in the Animation's rate >> property: >> >> public void invalidated() { >> final double newRate = getRate(); >> if (isRunningEmbedded()) { >> if (isBound()) { >> unbind(); >> } >> set(oldRate); >> throw new IllegalStateException("Cannot set rate of embedded >> animation while running."); >> } >> >> isRunningEmbedded checks for a parent animation that is not STOPPED. That >> means that the rate of a child animation can only change while the parent >> is stopped, but it is ignored (the child's ClipEnvelope's rate is updated >> to the parent's rate instead). >> Binding to the rate property is never an issue, but it does give >> meaningless results since the rate is ignored. Binding the rate property >> itself will cause it to be unbinded. >> >> If we go with option 1, which is almost the current situation, there is >> only some cleanup needed when it comes to the relation between the >> Animation's rate and its ClipEnvelope's rate. >> > > This (option 1) seems easiest, but I can see am argument for either > approach. > > Remind me again, which JBS bug you are planning to use to fix this issue? > > If we go with option 2, we can eliminate the duplication of rate in the >> Animation and the ClipEnvelope (though ClipEvenope to Animation is somewhat >> like a peer to a Node, which duplicates the values as well, so maybe we >> want that?). The children's rate will always be the parent's rate. In this >> case the above code should probably be modified to account for STOPPED >> parents as well. A bound rate property will be unbinded and setting it will >> be either ignored or throw an exception. >> > > Yes, I would agree with this. > > >> I'll note that currentRate is eliminated from the ClipEnvelope in the fix >> I'm working on, but as it's read-only, it's a different story. >> >> As long as the implementation >>> is consistent in ignoring the rate property from the child Animation, >>> this seems like a fine solution and is consistent with other properties >>> where one property overrides another without modifying it. >> >> >> Which other properties? >> > > I meant outside animation. Node.opacity for example. You can set it to any > value and it is clamped on use. I realize that this is a different > situation. > > > delay, autoReverse, and cycleCount all work separately on the parent and >> child. For example, if both parent and child have autoReverse = true and >> cycleCount = 2, the animation will play forwards, backwards, forwards, >> backwards, so both are taken into account. >> > > OK. > > -- Kevin > > > On Wed, Jan 8, 2020 at 1:14 AM Kevin Rushforth <kevin.rushfo...@oracle.com> >> wrote: >> >>> A rate in a "parent" transition should probably override the rate in a >>> child Animation. There are a couple ways to do it, and it sounds like it >>> doesn't really implement it right. >>> >>> 1. The public rate property values of child Animations within a parent >>> Transition are ignored, but not updated. As long as the implementation >>> is consistent in ignoring the rate property from the child Animation, >>> this seems like a fine solution and is consistent with other properties >>> where one property overrides another without modifying it. >>> >>> 2. Setting a rate on the parent Transition actually modifies the child >>> node. This can work, but has more issues. What happens if you set the >>> rate of the parent tansition and then set the rate of a child animation? >>> What if you bind to the rate of a child animation? >>> >>> -- Kevin >>> >>> On 1/1/2020 1:04 PM, Nir Lisker wrote: >>> > Hi, >>> > >>> > As part of my work in the animations package I've come across an >>> undefined >>> > spec. Is the rate of an animation that is embedded in a >>> Parallel/Sequential >>> > transition inherited and overridden by its parent? >>> > >>> > If I have an animations with a rate of 1 and a parent with a rate of >>> 2, and >>> > I add the animation to the parent, does the rate property change from >>> 1 to >>> > 2 (generating a change/invalidation event etc.)? Then, should any >>> > subsequent changes to the parent's rate trigger a change in the child's >>> > rate? >>> > >>> > The implementation is a bit messy in this regard. The rate and >>> currentRate >>> > values exist in both Animation and its ClipEnvelope, which is the >>> source >>> > for a couple of bugs. For example, changing the rate of a >>> > ParallelTransition changes the child's ClipEnvelope's rate and >>> currentRate, >>> > but only the animation's currentRate (and not rate). Is it supposed to >>> be >>> > this way? >>> > >>> > What is clear is that it's not possible to change the rate of the >>> embedded >>> > animation directly. >>> > >>> > The docs do not talk about the rate of an embedded animation (they do >>> talk >>> > about the node property though). >>> > >>> > - Nir >>> >>> >
