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.

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

Added onStart parameter to FlxTween #1273

Merged
merged 6 commits into from
Aug 20, 2014

Conversation

anissen
Copy link
Contributor

@anissen anissen commented Aug 17, 2014

Changes in FlxTween:

  • complete callback parameter in options is now called onComplete
  • Added onStart callback parameter in options

Notice: A few changes need to be made in the flixel-demos repository - I'll push those shortly

@@ -540,11 +565,13 @@ class FlxTween implements IFlxDestroyable
}

typedef CompleteCallback = FlxTween->Void;
typedef StartCallback = FlxTween->Void;
Copy link
Member

Choose a reason for hiding this comment

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

Might make sense to unify this to a "TweenCallback"?

@MSGhero
Copy link
Member

MSGhero commented Aug 17, 2014

TweenLite has an onUpdate param as well, do you think it'd be useful here too?

@Gama11
Copy link
Member

Gama11 commented Aug 17, 2014

I thought about that as well. It'd basically be the same as NumTween's tween callback.

@gamedevsam
Copy link
Contributor

onStart and onUpdate both seem like good additions to me.

@anissen
Copy link
Contributor Author

anissen commented Aug 18, 2014

Great feedback! I've added an onUpdate callback and made all three callbacks of type TweenCallback :)

@Gama11
Copy link
Member

Gama11 commented Aug 18, 2014

Nice! I don't like the redundancy this introduces though. Like menionted, onUpdate is basically the same as NumTween's tween callback. It has a different signature though, Float->Void... I guess the options are:

  • change onUpdate to FlxTween->Float->Void and get rid of NumTween's tween callback
  • keep onUpdate as is, but add a value property to FlxTween
  • keep both as is

Thoughts?

@anissen
Copy link
Contributor Author

anissen commented Aug 18, 2014

change onUpdate to FlxTween->Float->Void and get rid of NumTween's tween callback

Unless I'm mistaking, that would only work when tweening Float->Float. When tweening a color, for instance, then the resulting value would be an FlxColor and the function signature would thus have to be FlxTween->FlxColor->Void.

keep onUpdate as is, but add a value property to FlxTween

Same argument as above; the type of the value would would depend on the type being tweened.

keep both as is

It would be probably, in some cases, be convenient to have access to the current tween value through the onUpdate callback. Right now I can't really see an elegant solution for doing that, however.

@gamedevsam
Copy link
Contributor

I vote this one:

keep onUpdate as is, but add a value property to FlxTween

@Gama11
Copy link
Member

Gama11 commented Aug 18, 2014

Hm... yeah, I didn't consider that the type of the value varies. It would be akward to represent a color as a Float - the other option is to add a type parameter to FlxTween, which seems like overkill to me. Perhaps we should leave it as is?

@MSGhero
Copy link
Member

MSGhero commented Aug 18, 2014

NumTween's onUpdate:FlxTween->Void you could manually set in the constructor or whatever.

// NumTween
this.onUpdate = function(fxt:FlxTween):Void { value = start + whatever;  };
// ColorTween
this.onUpdate = function(fxt:FlxTween):Void { color.interp() or whatever; };

value and color being properties of Num and ColorTween and not FlxTween. I don't think you need value in FlxTween.

@gamedevsam
Copy link
Contributor

Is there any opposition to merge this? I think it's a good change, even if it doesn't unify the entire tweening system.

Gama11 added a commit that referenced this pull request Aug 20, 2014
Added onStart parameter to FlxTween
@Gama11 Gama11 merged commit 4bb8ddc into HaxeFlixel:dev Aug 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants