sshahine / JFoenix

JavaFX Material Design Library
MIT License
6.29k stars 1.06k forks source link

Changeable in animation speed of JFXRippler #773

Open schlegel11 opened 6 years ago

schlegel11 commented 6 years ago

It would be great if there is a method where you can set a duration speed of an animation like: rippler.setInAnimationSpeed(Duration d);

or something like that :)

schlegel11 commented 6 years ago

The right spot for this is line 539 in class JFXRippler: ... ), new KeyFrame(Duration.millis(500), inKeyValues)); ... so if I change the duration of the key frame the interpolation of the previous frame is done faster or slower. I cloud implement this if it were something that could be accepted :)

jfoenixadmin commented 6 years ago

Hello, sure it's good to have such a feature. Moreover, to allow animation customization instead of just the speed.

Regards,

schlegel11 commented 6 years ago

I'm making progress with the implementation. Are there any settings for the code formatter? I have seen that it is formatted according to the google standard.

jfoenixadmin commented 6 years ago

yes we are using google standard. Feel free to share your progress in a PR, so we can start reviewing the implementation.

Regards,

schlegel11 commented 6 years ago

OK I changed and added some things but I have to cleanup and refactor.

So I thought about what we could use for a simple animation experience. Since CSS is quite widespread and animations are also possible, I thought we could orientate to it. So I created the JFXAnimation class which is in general a builder class for a Timeline but with the exception that you can handle this builder like a CSS animation. It's completely lazy.

I used some CSS animations from this site https://daneden.github.io/animate.css/ For comparison this is the CSS "tada" animation:

@keyframes tada {
  from {
    transform: scale3d(1, 1, 1);
  }

  10%,
  20% {
    transform: scale3d(0.9, 0.9, 0.9) rotate3d(0, 0, 1, -3deg);
  }

  30%,
  50%,
  70%,
  90% {
    transform: scale3d(1.1, 1.1, 1.1) rotate3d(0, 0, 1, 3deg);
  }

  40%,
  60%,
  80% {
    transform: scale3d(1.1, 1.1, 1.1) rotate3d(0, 0, 1, -3deg);
  }

  to {
    transform: scale3d(1, 1, 1);
  }
}

.tada {
  animation-name: tada;
}

This is the java JFXAnimation representation:

JFXAnimation.builder()
          .from()
          .action(b -> b.target(Node::scaleXProperty, Node::scaleYProperty).endValue(1))
          .percent(10, 20)
          .action(b -> b.target(Node::scaleXProperty, Node::scaleYProperty).endValue(0.9))
          .action(b -> b.target(Node::rotateProperty).endValue(-3))
          .percent(30, 50, 70, 90)
          .action(b -> b.target(Node::scaleXProperty, Node::scaleYProperty).endValue(1.1))
          .action(b -> b.target(Node::rotateProperty).endValue(3))
          .percent(40, 60, 80)
          .action(b -> b.target(Node::scaleXProperty, Node::scaleYProperty).endValue(1.1))
          .action(b -> b.target(Node::rotateProperty).endValue(-3))
          .to()
          .action(b -> b.target(Node::scaleXProperty, Node::scaleYProperty).endValue(1))
          .action(b -> b.target(Node::rotateProperty).endValue(0))
          .config(b ->b.duration(Duration.seconds(1.3)).interpolator(Interpolator.EASE_BOTH).infiniteCycle());

So you can use the "from()", "to()" or "percent(...)" method for defining the different keyframes. To create a delay between keyframes or set it to different time intervals its possible to write "... .percent(10).percent(20)..." or shorter "... .percent(10, 20) ...". The action method uses a builder for a "JFXAnimationValue". So the method "... action(b -> ....) ..." takes a "JFXAnimationValue.Builder" instance where "b" is already a fresh "JFXAnimationValue.Builder" instance for quick usage. For every method there exists also a non lambda version so you could also use "... action(JFXAnimation.JFXAnimationValue.builder()...) ...".

The "JFXAnimationValue" builder has also some specials :wink: Normally you have to define for every KeyValue target an new KeyValue instance (mostly for the same endValue) e.g:

new KeyValue(scaleXProperty(), 1, Interpolator.EASE_IN)
new KeyValue(scaleYProperty(), 1,Interpolator.EASE_IN)

or with the action methods:

...
.action(b -> b.target(Node::scaleXProperty).endValue(1).interpolator(Interpolator.EASE_IN))
.action(b -> b.target(Node::scaleYProperty).endValue(1).interpolator(Interpolator.EASE_IN))
...

So for an easier handling the "target(...)" method can also use varargs for such cases like:

.action(b -> b.target(Node::scaleXProperty, Node::scaleYProperty).endValue(1))

There is also the possibility to define a "onFinish(...)" handler or an "animateWhen(...)" condition even if you have no animation target like:

.action(b -> b.target(Node::scaleXProperty, Node::scaleYProperty).endValue(1).onFinish((node, event) -> System.out.println("Hello")))
.action(b -> b.noTargets().onFinish((node, event) -> System.out.println("Visible")).animateWhen(Node::isVisible))

If you are done with the animation actions the last part of the building process is the configuration of the timeline like:

...
.config(b -> b.duration(Duration.seconds(1.3)).interpolator(Interpolator.EASE_BOTH).infiniteCycle());

So the duration is the total duration of the animation. The interpolator is set for all "action(...)s" which have not defined their own Interpolator. You can configure all possible Timeline methods.

After the configuration part you just call the ".build(T node)" method with your Node as parameter. The generic T type is defined when you instance the JFXAnimationn builder so "JFXAnimation.builder()" defines just a general Node.class type. You can also define your own with "JFXAnimation.builder(MyNode.class)".

Another feature which I implemented is a SVG Rippler cutout so you can define a "SVGGlyph" which is subtracted or intersected with a Rippler circle. I experimented with the JFXRippler demo and created some examples which uses some logo and icomoon glyph cutouts. Furthermore the CSS animations "heartbeat", "flash" and "tada" are used.

jfx_buttons

I also made some minor changes e.g. for the RegexValidator because I had also defined one by myself and doesn't know that there already exist one on the master branch.

You can find the code changes on my fork:

jfoenixadmin commented 6 years ago

Hello @schlegel11 , very nice work 👍 well done.

I did a quick review for the code and it looks pretty good, I like how you restructured the animation creation process to be similar to web css 👍. I believe it's more straight forward than the standard way of creating animations.

However, the thing is we already have JFXAnimationTimer/JFXKeyValue/JFXKeyFrame in JFoenix ( this implementation is based on AnimationTimer which adds some flexibility to the animation, so you can have conditional KeyValues / changeable end value / injection of external properties to be animated ). So we need to make it clear somehow to differentiate between these 2 methods, otherwise it might be confusing for the user.

Regarding the cutout, It's a very cool idea ;) never thought of it before. I noticed that you changed the Ripple class so it extends Path instead of Circle, I need to do a full review to make sure it's well structured.

All in all, it looks very good, great job 👍 Regards,

schlegel11 commented 6 years ago

Hi, oh thank you :smiley:

But yes you are totally right. I saw JFXAnimationTimer/JFXKeyValue/JFXKeyFrame classes and thought about how can I integrate them with my approach. In the first version of JFXAnimation I tried to use the JFXKeyValue but due to some not so good cast's for the KeyVaule I implemented the new generic JFXAnimationValue. But yes I liked the JFXKeyValue approach with the Supplier as return values and also the animateCondition. That's why I also implemented the animateWhen method or for laziness the Suppliers :wink: I fully agree with you that it could be confusing for the user. Maybe the name JFXAnimation is not well chosen. I also like the JFXAnimationTimer and its features. I think that we could make the JFXAnimation build(T node) method more generic regarded to the return value. So currently there is only a default Timeline returned. Perhaps we could implement build methods which can also return some custom implementations of Animation like a CachedTimeline version or especially a JFXAnimationTimer. I think that shoudn't be a problem.

Ah and yes for the cutout I had to replace the Circle extension for the Ripple with the Path extension and use the Circle for the cutout which ends always in a Path object:wink: Take your time with the review because I have to refactor and revert some things in the JFXRippler. For the demo showcase I implemented very quick the cutout method so with a boolean for intersection. I think that's not the best approach and I would use an Enum or so but yeah it was only for a quick look for the demo and I have to cleanup and comment all that stuff :grinning:

jfoenixadmin commented 6 years ago

Hi, can you specify the not so good casts of JFXKeyValue?

Also I noticed in JFXAnimationKeyValue, all the suppliers/functions are dependent on the animation node, which I don't think is a good dependency.

schlegel11 commented 6 years ago

Hi, yes and I have to withdraw my statement sorry. So it's not about a cast but about unchecked types. For example with the JFXKeyValue I could write this:

    JFXKeyValue a =
        JFXKeyValue.builder()
            .setTarget(rippler.scaleXProperty())
            .setEndValue("works but makes no sense")
            .build();
    //return type of "getTarget" is unchecked.
    new KeyValue(a.getTarget(), a.getEndValue()); 

I also have to deal with the case that I could set some type to a specific WritableValue and it's only discovered at runtime.

    JFXAnimation.JFXAnimationValue<?, Number> b =
        JFXAnimation.JFXAnimationValue.builder()
            .target(rippler.scaleXProperty())
            //Method cant be resolved cause Number is required.
            .endValue("dont work cause need Number type")
            .build();
    new KeyValue(b.getFirstTarget().orElse(/*something if there are no targets*/), b.getEndValue());

In this case I can't set a wrong type to a specific WritableValue. The JFXAnimationValue depends on the animation node for better handling with a KeyValue target. So in the above example I used a wildcard as generic for the animation node

JFXAnimation.JFXAnimationValue<?, Number> b ...

because in this case I don't need a reference to my Node (in this case a Rippler). So the Rippler instance is in my actual method scope and I can directly specify my target

... .target(rippler.scaleXProperty()) ...

But if I want to predefine my animation lazily as a template for any possible target which is related to an unknown object instance I could do this:

  public static final JFXAnimation.Builder<Node> FLASH =
      JFXAnimation.builder(JFXRippler.class)
          .from()
          .percent(50)
          .action(b -> b.target(JFXRippler::opacityProperty).endValue(0))
          .percent(25, 75)
          .action(b -> b.target(JFXRippler::opacityProperty).endValue(1))
          .config(b -> b.duration(Duration.seconds(1.5)).interpolator(Interpolator.EASE_BOTH));

In this case all I know is that I want my animation used with a e.g JFXRippler type and I need to modify some JFXRippler/Node type properties. All the methods in JFXAnimationValue with a Functions<JFXRippler,...> parameter allow me to access a potential JFXRippler type. Later in my class where I would like to use this animation I can just call FLASH.build(rippler) and thats it. That's the reason for all the generic type information.

schlegel11 commented 6 years ago

I improved the JFXAnimation a little bit so I removed the <N extends Node> generics and changed it to <N> cause the type restriction makes no sense because a KeyValue target property can come from any type not only Node types.

I also added the possibility to add more instances of animation nodes which can be used for modify other KeyValue targets.

So I rely on it that if a Animation is defined at least one main animation node could be used like this normal case:

public static final JFXAnimation.Builder<Node> FLASH =
      JFXAnimation.builder()
          .from()
          .percent(50)
          .action(b -> b.target(Node::opacityProperty).endValue(0))
          .percent(25, 75)
          .action(b -> b.target(Node::opacityProperty).endValue(1))
          .config(b -> b.duration(Duration.seconds(1.5)).interpolator(Interpolator.EASE_BOTH));

If I call "FLASH.build(myNodeObject)" one node object is used. If I want to predefine my animation with more KeyValue targets which rely on different animation objects I can define N KeyValue pairs which can be used in the Animation like:

public static final JFXAnimation.Builder<Node> FLASH =
      JFXAnimation.builder()
          .from()
          .percent(50)
          .action(b -> b.target(Node::opacityProperty).endValue(0))
          //here I want also animate some Rectangle object so I define the type of it and key.
          .action(b ->  b.withClass(Rectangle.class, "rect").target(Rectangle::translateXProperty).endValue(70))
          .percent(25, 75)
          .action(b -> b.target(Node::opacityProperty).endValue(1))
          .config(b -> b.duration(Duration.seconds(1.5)).interpolator(Interpolator.EASE_BOTH));
Rectangle rectangle = new Rectangle();
//This rectangle object should be used in the animation on a "withClass(Rectangle.class, "rect")" call.
//In all other cases the "myMainNodeObject" could be simply used.
FLASH.build(myMainNodeObject, new Pair<>("rect", rectangle));
schlegel11 commented 6 years ago

I also added now a builder function for building a JFXAnimationTimer. Furthermore I changed the builder method signature to a more functional approach where the specific builder can be set like:

//default build a Timeline
Timeline timeline = FLASH.build(node);

//or by specify the Timeline builder
Timeline timeline = FLASH.build(JFXAnimation::buildTimeline, node);

//build a JFXAnimationTimer
JFXAnimationTimer timer = FLASH.build(JFXAnimation::buildAnimationTimer, node);

To my surprise I have also seen that JFXKeyValue<T> also has a generic for specifying the WriteableValue type but the Builder doesn't handle that information. Anyway it doesn't matter for the JFXAnimation building process where this covers the generic requirements.

schlegel11 commented 6 years ago

I made some refactor steps. I created an extra package "com.jfoenix.transitions.creator" and changed name of JFXAnimation to JFXAnimationCreator. Furthermore I moved all the inner classes to it's own class. The JFXAnimationBuilder class contains now the specific implementations for creating a Timeline or JFXAnimationTimer.

I think those were all the adjustments I had in mind so far.

jfoenixadmin commented 6 years ago

Regarding type safety for JFXKeyValue, I agree with you, it's better to make JFXKeyValue builder type safe. Moreover, how about combining these two classes JFXKeyValue/JFXAnimationValue? so it would be less confusing for the user. One JFXKeyValue class and its builder instead of having multiple classes for the same purpose.

jfoenixadmin commented 6 years ago

I updated JFXKeyValue builder to ensure type safety and pushed the fix

schlegel11 commented 6 years ago

Ah great that's a really good enhancement. Regarding to the JFXKeyValue/JFXAnimationCreatorValue classes when I started developing the JFXAnimationCreator it was also my intention to combine the classes or also to use the JFXKeyValue cause it was more or less similar. Now I would say it's good to have them separated although they look very similar (because i also oriented myself to the JFXKeyValue class :wink: ), they still differ in some functions. The JFXAnimationCreatorValue uses a lot of Functions not Suppliers in order to make it possible to use directly a helper object in the creating process like:

// Here the Node object is the helper object which properties are used.
.action(b -> b.target(Node::opacityProperty).endValue(0))

Also there are some other new methods like "ignoreAnimation()", "onFinish(...)", "withClass(...)" to make the creation process easier.

All in all I would say they doesn't share the same purpose because the JFXAnimationCreator and also the JFXAnimationCreatorValue and the other classes are part of a general builder for a Timeline or for a AnimationTimer or for some other Animation related construct.

On the other hand the JFXKeyValue class is a specific Builder or part of a specific construct in this case the JFXAnimationTimer. It's comparable with the KeyValue class which is specifically used for the Timeline class. The JFXAnimationCreatorValue class is just a unspecific Builder which wraps it up.

schlegel11 commented 6 years ago

Maybe we need another name for the JFXAnimationCreatorValue class. Something like JFXAnimationCreatorAction because it's related to the action method in the builder process.

jfoenixadmin commented 6 years ago

I guess, maybe we need a better naming. I'm usually bad at naming but how about JFXAnimationTemplate?

also I have some questions regarding the code:

  1. JFXAnimationCreatorValue

    • if noTarget method is allowed, then what's the point of forcing 1 argument in target method?
    • what's the purpose of GenericBuilderWrapper / animationHelper? I'm kinda confused here :s
  2. JFXAnimationBuilder

    • Not sure why decoupling the creator from the builder, however I think it it can be done in a better way. how about creating a build method in JFXAnimationCreator that takes a builder. Similar to stream collect api.
    • also what's the purpose of creatorConfigBuilderFunction ?
schlegel11 commented 6 years ago

Hi, yes ok Template sounds good why not :blush:

I refactored all the names that it makes hopefully a little more sense :wink:

Ah and I also changed the JFXKeyValue so that the WritableValue<T> target uses now WritableValue<? super T> target because if you define the Builder like that: .setEndValue(3).setTarget(...) in most cases the WriteableValue<?> can't be inferred because e.g. a DoubleProperty uses a WriteableValue<Number> but the EndValue is interpreted as Integer.

jfoenixadmin commented 6 years ago

Hi @schlegel11, I'm trying to keep up with all these builder, however I keep getting lost. I have a question, what's the point of JFXAnimationTemplate? I mean this class is only used to wrap the builder object! right?. if that's the case then why not making it the builder itself? for the sake of simplicity :p

Also I noticed you are using default methods, this will break the mobile build. We can't have default methods on mobile just yet.

jfoenixadmin commented 6 years ago

Also if you can provide some examples that cover all cases it would appreciated.

schlegel11 commented 6 years ago

Hi, no problem. In summarize:

The JFXAnimationTemplateAction describes a animation action like the KeyValue from a Timeline. You can define actions like:

//This action uses a Node type as animation object. For the layoutX and Y properties.
    JFXAnimationTemplateAction<Node, Number> action =
        JFXAnimationTemplateAction.builder()
            .target(Node::layoutXProperty, Node::layoutYProperty)
            .endValue(10)
            .interpolator(Interpolator.LINEAR)
//In the builder method we have to provide the specific object for the Node type in this case a JFXRippler rippler1 object. The key can be ignored.
            .build(key -> rippler1);

//This action uses a specific JFXSlider type.
    JFXAnimationTemplateAction<JFXSlider, Number> action2 =
        JFXAnimationTemplateAction.builder(JFXSlider.class)
            .target(JFXSlider::translateXProperty, JFXSlider::translateYProperty)
            .endValue(10)
            .interpolator(Interpolator.LINEAR)
//We must build with a JFXSlider object.
            .build(key -> slider);

//In this action we don't use the lazy build ability and can use the rippler1 object directly.
    JFXAnimationTemplateAction<?, Number> action3 =
        JFXAnimationTemplateAction.builder(JFXSlider.class)
            .target(rippler1.translateXProperty(), rippler1.translateYProperty())
            .endValue(10)
            .interpolator(Interpolator.LINEAR)
//We don't use a lazy object in the target methods so we don't set it here (internally it's set to null)
            .build();

//This action overrides the default Node type for the animation object with a JFXSlider type.
    JFXAnimationTemplateAction<JFXSlider, Number> action =
// The builder method gives us just the default Node type.
        JFXAnimationTemplateAction.builder()
// The withAnimationObject defines that the target methods should use the lazy object of JFXSlider type and not the Node type which is the default one. Furthermore a specific key is set.
            .withAnimationObject(JFXSlider.class, "slider")
            .target(JFXSlider::translateXProperty, JFXSlider::translateYProperty)
            .endValue(10)
            .interpolator(Interpolator.LINEAR)
//With the key it is possible to use e.g. a HashMap for providing different types and animation objects.
            .build(key -> myMap.get(key));

The JFXAnimationTemplateConfig describes the general config of the animation like it would be set on a Timeline.

    JFXAnimationTemplateConfig config =
        JFXAnimationTemplateConfig.builder().duration(Duration.seconds(5)).cycleCount(2).build();

The JFXAnimationTemplate is already the general builder which compose the other classes like JFXAnimationTemplateAction and JFXAnimationTemplateConfig. So an animation can have multiple JFXAnimationTemplateActions and a JFXAnimationTemplateConfig which is like a Timeline can have multiple KeyFrames and KeyValuess.

Ah ok that's the reason why the default methods were disabled :wink: I will replace the methods with general implementations.

Yes you are right some examples would be good. So I will create examples with all cases covered.

jfoenixadmin commented 6 years ago

Yes I noticed the similarity between JFXAnimationTemplate and Timeline. but my question was, what's the point of having a builder for JFXAnimationTemplate when it only contains its builder as a field? I mean JFXAnimationTemplate doesn't declare actions/config directly as fields, instead actions/config are stored inside JFXAnimationTemplate.Builder which is declared as a field in JFXAnimationTemplate. so I was wondering why not making the JFXAnimationTemplate as the builder itself instead of creating inner class called Builder inside JFXAnimationTemplate. I'm not sure whether the builder pattern is applied here correctly or not.

schlegel11 commented 6 years ago

Ah ok I got it :blush: You are right I think the JFXAnimationTemplate violates the rules of a normal builder pattern maybe in this case a step builder. I see only one problem that the "config()" method returns a Builder which currently ensures that afterwards only the build method can be called. Also we have to extract the interfaces because a class cant contain multiple inner interfaces. But you are right maybe we can combine this classes.

schlegel11 commented 6 years ago

Ok I removed the default methods. Furthermore I removed the inner Builder class. I extracted the Interfaces to its own files. The varags in build() method were replaced with Map's.

jfoenixadmin commented 6 years ago

@schlegel11 it looks good to me, however one more thing I noticed in JFXAnimationTemplateAction, you created InitBuilder to force calling the target method at the beginning, right? however there is noTargets method which defies this restriction. so I'm not sure whether the InitBuilder is still needed anymore ( I think InitBuilder and Builder inside JFXAnimationTemplateAction can be combined), what do you think?

In case we merged all builders, and we have 1 builder for JFXAnimaionTemplateAction and 1 builder for JFXAnimationTemplateConfig. JFXAnimationTemplate has 2 methods action / config that takes builder: function(builder) , shouldn't this be replaced with Consumer(builder) instead?

schlegel11 commented 6 years ago

Hi, the InitBuilder in JFXAnimationTemplateAction fulfills the same purpose like the Builder in JFXKeyValue to infer the target generic so I think we can't combine them. The noTargets method was just for the case that we don't need to duplicate methods in the InitBuilder for delegation from the Builder. But yes you are right it's a bad design decision in general for the API user so I created the necessary methods.

You are right we could replace the Function with a Consumer in the JFXAnimationTemplate but this would have some drawbacks. If we would just use a Consumer we could have e.g. this case normal:

...
.action(b -> b.animateWhen(....).onFinish(...))
...

but also this case which works but makes no sense:

...
.action(b -> b.animateWhen(....).onFinish(...).build())
...

Furthermore the Function param can act as a Supplier e.g. if we don't want to use the given builder instance but our own:

...
.action(b -> JFXAnimationTemplateAction.builder().animateWhen(....).onFinish(...))
...

The Consumer just can handle the given reference but no new instance.

jfoenixadmin commented 6 years ago

I see your point, maybe its better to have a function here. For me all seems good, no more comments 👍 . Feel free to create a PR if you want Regards,

schlegel11 commented 6 years ago

Hi, great :relaxed: I will submit a PR the next days. Regarding the other integrations like the cutout and how such an animation template is handled in the Rippler we'll have to see what we can do.

The cooperation with you was a lot of fun for me and very pleasant. Thank you very much. If there's anything else we can comment again. Greetings

jfoenixadmin commented 6 years ago

Hi, yes sure, I'll review the cutout and rippler changes and get back to you.

Same for me :) thank you for such a cool feature :) Regards,

rnayabed commented 6 years ago

Glad to see @schlegel11 already working on this. I am working on a JavaFX based POS system with JFeonix, i wanted to use this feature :) Waiting for the PR ...

jfoenixadmin commented 6 years ago

Hi @schlegel11 I reviewed your changes regarding JFXRippler. Updating JFXRippler animation to use animation template instead, looks good to me. However the animation itself is still unchangeable outside the JFXRippler class :p. Regarding the cutout, I think the method createSVGCutout should be part of the RippleGenerator instead of the Rippler class (e.g rippleCutout could be initiated in the constructor)

schlegel11 commented 6 years ago

Hello, Yep you are totally right. I have not yet been able to review/edit the implementations. It was just a quick and dirty work :wink: I have to rework the code in the next days. Ah and I will create another pull request the next days with an AnimationDemo. I create one which is based on Animate.css :wink:

jfx_animation_demo

Greetings

jfoenixadmin commented 6 years ago

That's cool maybe we can add it to the main demo too :) well done @schlegel11 👍

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.