Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FlxTween: add .wait(), and .then() for chaining, closes #1064 #1614

Merged
merged 14 commits into from
Oct 4, 2015
Merged

FlxTween: add .wait(), and .then() for chaining, closes #1064 #1614

merged 14 commits into from
Oct 4, 2015

Conversation

larsiusprime
Copy link
Member

This lets you go from this:

FlxTween.tween(this, { y:endY }, time, { onComplete: 
    function(f:FlxTween):Void {
        f.cancel();
        new FlxTimer().start(DISPLAY_TIME, function(t:FlxTimer):Void {
            t.cancel();
            FlxTween.tween(this, { y:startY },time, { onComplete: finishAnimation } );
        });
    }
});

To this:

FlxTween.tween(this, { y:endY }, time).thenWait(DISPLAY_TIME).thenTween(this, { y:startY }, time, {onComplete: finishAnimation } );

@MSGhero
Copy link
Member

MSGhero commented Sep 24, 2015

Is this sort of a Promise thing? I'm actually trying to implement those in my project right now.

@Tiago-Ling
Copy link
Contributor

@larsiusprime Very nice! Chaining callbacks makes the code much more readable - does it change anything performance-wise?

@larsiusprime
Copy link
Member Author

I think the two syntaxes I showed here will be about the same, but I haven't measured it.

Also, tweens have never been particularly optimized with regards to performance -- it's using a big fat dynamic object to hold all your tweenable values, which it accesses with reflection, so even if this syntax added some weight it probably wouldn't be a big deal.

But the whole point of tweens (as I understand it, I could be wrong) is an easy and simple way to animate, and it just has to be "fast enough" to not be the bottleneck.

@Tiago-Ling
Copy link
Contributor

Yeah, i was actually hoping that somehow this could improve performance - even removing it from the call, the anonymous object still exists inside the tween right?

@larsiusprime
Copy link
Member Author

Well, we should really profile it first to see if it's actually causing a bottleneck -- usually drawing, not update functions like this, are the culprit. Unless inefficient tweens are one of the top 10 causes of slowdown, or eating lots of memory, it's usually not worth optimizing it no matter how "bad" it looks.

@Tiago-Ling
Copy link
Contributor

I know, it's not a bottleneck at all, at least here. If your entire game is made around FlxTweens it could matter though. Anyway i just wondered if there was any additional reason besides ease of use and readability for this change. 👍

@larsiusprime
Copy link
Member Author

Yeah, that makes sense! This is just a syntax upgrade -- it might have the slight benefit of not having to instantiate a new FlxTimer(), but I'm not sure that matters at all.

If we did want to optimize out that dynamic object, I'm not sure it could be done without making the syntax a lot less convenient... I guess we'll just wait to see if anyone is using tweens for EVERYTHING 0_0

@larsiusprime
Copy link
Member Author

It looks like the Travis CI build that are failing is in RPG Interface, related to FlxUI, and only in legacy:

/home/travis/haxe/lib/flixel-ui/git/flixel/addons/ui/FontDef.hx:309: characters 3-7 : String should be openfl.text.TextFormatAlign
/home/travis/haxe/lib/flixel-ui/git/flixel/addons/ui/FontDef.hx:309: characters 3-7 : String should be openfl._legacy.text.TextFormatAlign

@Gama11
Copy link
Member

Gama11 commented Sep 27, 2015

I don't really like how this is VarTween-specific, there are a lot more tween types after all. I guess it is the one most commonly used, but it still makes for an inconsistent API.

Interested in @JoeCreates' opinion on this since he seemed to have strong opinions on this in #1064. Also wondering if and how this affects his refactor concerning #1087.

@larsiusprime
Copy link
Member Author

It could probably be refactored to not require VarTween

@larsiusprime
Copy link
Member Author

@Gama11 Okay, did a quick refactor. What do you think now?

Gama11
Gama11 reviewed Oct 1, 2015
View reviewed changes
flixel/tweens/FlxTween.hx

typedef TweenParams = OneOfNine<VarTweenParams,AngleTweenParams,ColorTweenParams,NumTweenParams,CircularMotionParams,CubicMotionParams,LinearMotionParams,QuadMotionParams,QuadPathParams>;

enum TweenType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling this "tween type" conflicts with FlxTween.PINGPONG etc which are also considered tween types. It could see that causing quite a bit of confusion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative?

@larsiusprime
Copy link
Member Author

@Gama11 that better?

@Gama11
Copy link
Member

Gama11 commented Oct 1, 2015

Definitely less ambigious, yeah.

Gama11
Gama11 reviewed Oct 1, 2015
View reviewed changes
flixel/tweens/FlxTween.hx

enum TweenMethod
{
None_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is TweenMethod.None actually needed for anything?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, are the _ suffixes necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"None" is needed, yes, at least in this implementation. I use it for implementing the delays.

The suffixes are unfortunately necessary. I couldn't come up with a good naming convention otherwise:

attempt 1: remove "Tween"

AngleTween;   //<---- these look fine
ColorTween;
NumTween;
VarTween;
CircularMotionTween;   //<---- these conflict with their corresponding classes
CubicMotionTween;
LinearMotionTween;
QuadMotionTween;
QuadMotionTween;

attempt 2: just use one word

Angle;   //<----these look fine
Color;
Num;
Var;
Circular; //<----Circular what? Cubic what?
Cubic;
Linear;
Quad;
Quad;

attempt 3: end with "tween"

AngleTween;   //<---- these conflict with the classes now
ColorTween;
NumTween;
VarTween;
CircularMotionTween;   //<---- these are okay
CubicMotionTween;
LinearMotionTween;
QuadMotionTween;
QuadMotionTween;

@Gama11
Copy link
Member

Gama11 commented Oct 1, 2015

Do they conflict even if you use the fully qualified name? (TweenType.AngleTween instead of just AngleTween for example).

@larsiusprime
Copy link
Member Author

Maybe not, I haven't checked. Would it be confusing though?

@larsiusprime
Copy link
Member Author

It does work fully-qualified. Is it okay if the names are otherwise the same as the classes?

@Gama11
Copy link
Member

Gama11 commented Oct 1, 2015

Not sure...

This might not be the best approach to begin with. You're collecting the data needed to be able to act as a "tween factory" later when the tween whose turn it is is needed. That data is the "method" (kind of awkward naming here) and the data (dynamically typed).

What about creating all the tweens in advance, which means .then()'s signature would be FlxTween->FlxTween? Usage would look something like

FlxTween.tween(this, { y:endY }, time).
    thenWait(DISPLAY_TIME).
    thenTween(FlxTween.tween(this, { y:startY }, time, { onComplete: finishAnimation } ));

@larsiusprime
Copy link
Member Author

Yeah, that could work I guess

@larsiusprime
Copy link
Member Author

@Gama11 how's it look now?

@Gama11
Copy link
Member

Gama11 commented Oct 1, 2015

All those param typedefs etc can be removed now right?

@larsiusprime
Copy link
Member Author

@Gama11 : yup. Just forgot.

Gama11
Gama11 reviewed Oct 1, 2015
View reviewed changes
flixel/tweens/FlxTween.hx
* @param params
* @return
*/
public function waitThen(Delay:Float, Tween:FlxTween):FlxTween
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there really much of a point to this combined function? It doesn't really add anything, and it's not really shorter either. Compare:

wait(1).then(tween)
waitThen(1, tween)

The first one seems more readable to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that's fine. One less function to maintain.

Gama11
Gama11 reviewed Oct 1, 2015
View reviewed changes
flixel/tweens/FlxTween.hx

/**
* After this tween has finished, do this tween next
* @param method
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well delete these empty @param tags, especially considering they're out of sync with the method signature.

Gama11
Gama11 reviewed Oct 1, 2015
View reviewed changes
flixel/tweens/FlxTween.hx
@@ -621,7 +718,7 @@ typedef TweenOptions = {
?onUpdate:TweenCallback,
?onComplete:TweenCallback,
?startDelay:Null<Float>,
?loopDelay:Null<Float>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, didn't know Haxe allowed a trailing comma here for the last element. :)

@larsiusprime
Copy link
Member Author

Good now?

@Gama11
Copy link
Member

Gama11 commented Oct 1, 2015

I've added some more comments, don't think you've addressed those yet?

Gama11
Gama11 reviewed Oct 1, 2015
View reviewed changes
flixel/tweens/FlxTween.hx

_thens.splice(0, 1);

if (then.delay <= 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it might fail with wait(0) (which should just be "no wait" - doesn't make sense if written like that but the delay could be dynamically set from some variable that happens to become 0 at some point)? And what's the expected result for negative values?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on it...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right you were, it needs to call onEnd() again in the case that delay is <= 0 AND the tween is null, signifying it's a delay that should terminate instantly and run the next "then" command.

@larsiusprime
Copy link
Member Author

@Gama11 good now? Just tested it and it seems to work fine. Could probably use a demo eventually.

@larsiusprime
Copy link
Member Author

@Gama11 Can we merge this today? I think I've addressed everything.

@Gama11 Gama11 changed the title FlxTween: add .thenWait(), .thenTween(), etc, for chaining FlxTween: add .wait(), and .then(), for chaining, closes #1064 Oct 4, 2015
@Gama11 Gama11 changed the title FlxTween: add .wait(), and .then(), for chaining, closes #1064 FlxTween: add .wait(), and .then() for chaining, closes #1064 Oct 4, 2015
Gama11 added a commit that referenced this pull request Oct 4, 2015
@Gama11
Merge pull request #1614 from larsiusprime/tween_chain
7ead441 
FlxTween: add .wait(), and .then() for chaining, closes #1064
@Gama11 Gama11 merged commit 7ead441 into HaxeFlixel:dev Oct 4, 2015
Gama11 added a commit that referenced this pull request Oct 4, 2015
@Gama11
FlxTween: tween chaining (#1614) code cleanup
f191f9b
@larsiusprime larsiusprime deleted the tween_chain branch October 20, 2015 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@larsiusprime @MSGhero @Tiago-Ling @Gama11