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

[WIP] Tween extension #922

Closed
wants to merge 94 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Wend1go
Copy link
Contributor

Wend1go commented Feb 3, 2019

This PR adds a behavior for tween animations.

ToDo:

  • Tweens

    • Object Position
      • Object Position (X/Y)
      • Object X
      • Object Y
    • Object Scale
      • Object Scale (Width / Height)
      • Object Scale X
      • Object Scale Y
    • Object Angle
    • Object Opacity
    • Object Colour
    • Object Variable
  • Add StringList for easings

  • Code cleanup

  • Add Wiki article

  • Add example project

  • Plug the schedule function of each tween to the game engine (doStepPreEvents)

To be done by @4ian:

  • Support for stringlist: #911
  • For objectvar, read the object in the last parameters before objectvar (not only the one before) (to get object variable tweening working)
  • Ensure all tweens are paused/resumed when pausing/returning to scene

Wend1go and others added some commits Jan 23, 2019

Merge pull request #1 from 4ian/master
Merge from upstream
Merge pull request #2 from 4ian/master
Merge from Upstream
Matthias Meike
Initial commit
- currently only position tween and tweenhasfinished works
- probably have to restructure a lot since I have misunderstood how behaviors work

@Wend1go Wend1go referenced this pull request Feb 3, 2019

Closed

(Question) Tween extension #921

@Lizard-13

This comment has been minimized.

Copy link
Contributor

Lizard-13 commented Feb 3, 2019

The two main problems I can see so far:

  • You have to pass object in the behavior conditions/actions, no objectPtr, the first one works with the object and its own methods (related to the next point), the last is used when you work with more than one object or with functions not owned by the objects itself, objectPtr pass the other object as an argument.

  • If you pass object the behavior instance of the object will be used, i.e you won't need to use owner everywhere, the object owning this behavior will be stored in this.owner. And as you're working with behavior instances you've to use gdjs.TweenRuntimeBehavior.prototype in every function working with the behavior instance (every function after creation). If you do it you won't need to use instance nor TweenInstance anymore.

Check the new physics extension, it passes object everywhere, objectPtr for joints because there's a second object, and in the runtime code the behavior uses prototype everywhere because each object has its own body, and the behavior access its owner through this.owner :)

@Lizard-13

This comment has been minimized.

Copy link
Contributor

Lizard-13 commented Feb 3, 2019

Aside the instances handling problem it's looking pretty good.
Easy to access each property state through the identifier name, good idea since there will be a single tween for each property at a time, tweening the same property with two tweens at the same time has no much sense.

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Feb 3, 2019

Excited to see how this goes!

I might have completely misunderstood how behaviors store their data so I decided to already create a PR so that others can have a look before I keep writing the whole thing the wrong way.

Yes it's a good idea :)

@Lizard-13 remarks are valid, the idea of the behavior is to have a behavior per instance of the runtime object, so that it can store things specific to the instance. And so that you don't have to manually manage a global list (that have other issues: risk of leaking things by forgetting to erase things, risk of accessing to an non existing property, etc). Thanks @Lizard-13 btw for taking the time to read and explain this :)
Take a look at other behaviors like the physics one, or gdjs.TopDownMovementRuntimeBehavior to see how they are storing their properties - and see if you can do the same here :)

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Feb 4, 2019

I've added a few comments to help you on behaviors. Basically:

  • Declare all your action/condition/expression on the behavior.
  • Declare the JS functions on the prototype (to make them work on instances, like member functions in object oriented programming languages).
  • Remove the owner and tweenBehavior argument. Instead, owner is this.owner and the tweenBehavior is simply this.
  • Store the tween instance in this._tweens. Any additional property can also be stored in this._xxx (underscore to make it clear that nobody outside the class should use this).
  • Change any function dealing with your tweenList to use this._tweens instead.

And you'll have automatically something working for multiple objects: GDevelop will take care of calling your methods for each object according to the events :)

@Wend1go

This comment has been minimized.

Copy link
Contributor Author

Wend1go commented Feb 4, 2019

Thank you @Lizard-13 and @4ian for the explanations. This should simplify the code a lot. 👍

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Feb 4, 2019

Yup, should make things easier/clearer. Look forward to what we can see with this tweening extension. Could make some really interesting effects! 😄

Let's start simple - and I'll think later of even having some ways of allowing behaviors to render custom things on the scene editor, so that we can visualize tweens betweens positions or this kind of things. (It might be for the not so far future, but I still think at some point to allow objects/behaviors to draw custom things/widgets on the scene editor for things like tiled map or tweens) (but again, let's start simple with this extension, and see how it goes - performance will be an important topic too).

Also I've not made a lot of search on tweening libraries but shifty.js looks lightweight and fast, which is important.

@Wend1go

This comment has been minimized.

Copy link
Contributor Author

Wend1go commented Feb 4, 2019

The issue with tween libraries is that most of them receive updates very rarely. Even popular ones become more or less unmaintained. Shifty.js had its last commit about 2.5 months ago which makes it one of the more "active" projects.
The other benefits are its small size, ease of use and speed which should be about on par with the commercial Greensock library. It is also used in the "rekapi" timeline React extension @blurymind mentioned on trello. So if we add that in the future it would be compatible with the tween extension and we could use the timeline to create more complex animations via graphical editor or even inside the scene designer.
But this is out of scope for this PR.

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Feb 4, 2019

Thanks for the clarifications. :)
Sounds like a good choice, all arguments in favor of shifty.js make sense to me.

Matthias Meike
Declare actions/conditions/expressions on the behavior instead of the…
… extension

Add this._tweens for storeing the tweens inside the behavior instance
@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Feb 4, 2019

To make things a bit more clear, I've created a file about JS features/code style/typing: https://github.com/4ian/GDevelop/blob/master/newIDE/docs/Supported-JavaScript-features-and-coding-style.md - hopefully will be helpful to clarify what should be used where :)

@Lizard-13

This comment has been minimized.

Copy link
Contributor

Lizard-13 commented Feb 5, 2019

I know it's WIP, just wondering if the Position tween should be split in Position X and Y, you may want to move an object in a given axis and only tween the other axis, the same for scale/size.

@Wend1go

This comment has been minimized.

Copy link
Contributor Author

Wend1go commented Feb 5, 2019

Good question. If you want to tween only the X axis you could set the Y axis to MyObject.Y(). But you wouldn't be able to add an additional tween for the Y axis while the X axis is being animated.
I could add 3 Actions:

  • X
  • Y
  • Position (X and Y)

(Same for size)

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Feb 5, 2019

Yes I think it's fair to add the 3 actions (X, Y, position) then! :)

Matthias Meike
Add X,Y / Size,Width,Height / Angle tween functions
Replace arrow function with ES5 functions
@4ian

This comment has been minimized.

Copy link
Owner

4ian commented on Extensions/TweenBehavior/tweenruntimebehavior.js in 45a193d Feb 5, 2019

Should be setWidth I think?

This comment has been minimized.

Copy link
Contributor

Wend1go replied Feb 6, 2019

Good catch.
Yes the size based tween functions are completely untested.

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented on Extensions/TweenBehavior/tweenruntimebehavior.js in 45a193d Feb 5, 2019

I'm not sure we should restrict angle, it's perfectly valid to go to an angle like 720 degrees to make the object turn multiple time, and negative angle are valid too. So you can remove both conditions :)

This comment has been minimized.

Copy link
Contributor

Wend1go replied Feb 6, 2019

Wow didn't know of this. Looks like I can remove all those complicated value checks from my games then. 😁
How does GD handle Integer overflows if you add +1 to the angle forever? (might be an issue if someone uses GD for digital signage or other applications that run 24/7)

This comment has been minimized.

Copy link
Owner

4ian replied Feb 6, 2019

JavaScript numbers are integer up to 9007199254740991 (Number.MAX_SAFE_INTEGER), after which it it's handled as a floating point value and you're loosing precision:
image

Note that if you're doing a full rotation of an object (360 degrees) 60 times per second, you'll need (9007199254740991/360/60)/3600/24/365 = 13 000 years before overflowing and starting to lose precision.

Floating point numbers also have a limit, overflowing hit just give "Infinity" (which might create issues, but you'll likely have encounter other issues before anyway)

All of this can be completly avoided by "just" doing a modulo operation to always have the angle between 0 and 360. GDevelop is doing it at some place, but not moduling the angle for you as it's more convenient if you want to count things like the number of full rotation that you've done, etc...

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented on Extensions/TweenBehavior/tweenruntimebehavior.js in 45a193d Feb 5, 2019

This code if (destroyObjectWhenFinished) { .. } else { .. } is duplicated a lot. Might be worth putting it in a method (this.setupTweenEnding(identifier, destroyObjectWhenFinished)) :)

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented on Extensions/TweenBehavior/tweenruntimebehavior.js in 45a193d Feb 5, 2019

Also beware, the indentation is wrong for the rest of the function and in other functions too. Take the time to install prettier extension for your text editor/ide and get that file automatically formatted ;)

@Wend1go

This comment has been minimized.

Copy link
Contributor Author

Wend1go commented Feb 5, 2019

Current progress:
tween_behavior_progressreport

Remaining issues:

  • The variable animation isn't working. I don't know why yet. Setting the variable doesn't give any error but the value in the object variable isn't updated according to the debugger.
  • For the size animations, I need to filter for Sprite objects. How can I do this?
  • When multiple instances of the same object share the same tween (tween identification string) they somehow interfere with each other and remove their tween animation when one of them ends. Im not sure if it is the behaviour’s fault or the logic blocks that I clicked together (I'm pretty tired, maybe I find the culprit tomorrow 😪 ).
    positiontween_interferance
@Wend1go

This comment has been minimized.

Copy link
Contributor Author

Wend1go commented Mar 10, 2019

thanks for the update.
I'm without internet since the storm yesterday. @4ian would you like to give it a go?

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Mar 10, 2019

Sure, I'll give it a try tomorrow evening. The idea will be to instanciate a shifty scene associated to the runtimescene. Then, GDevelop has callbacks that are called at scene loading/unloading (callbacksRuntimeSceneLoaded, callbacksRuntimeSceneUnloaded). Will need to add a pair for pause/resume (callbacksRuntimeScenePaused, callbacksRuntimeSceneResumed). Inside, get the shifty Scene, pause/resume it and we should be good :)

An interesting "stress test" will be to take your example, add an action to change the scene (verify that tweens from objects are removed). And add an action to start the same scene (verify that the old one is paused) or stop it and return to the previous one (verify that tween are resumed at the position where they were). And stack 2, 3, 4, 5 scenes and see if all tweens are properly paused and resumed when starting a scene or going back to a previous one :D

@jeremyckahn

This comment has been minimized.

Copy link

jeremyckahn commented Mar 10, 2019

That sounds good to me! If you run into any Shifty-related issues I’ll do what I can to help out since the Scene API is still in an Alpha state.

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Mar 13, 2019

Did not have the time to work on this, but still planning to do it this week!

@Wend1go

This comment has been minimized.

Copy link
Contributor Author

Wend1go commented Mar 13, 2019

Take your time. I'll probably still be offline till the end of the week. A transformer station in the neighborhood has burned down.

@jeremyckahn

This comment has been minimized.

Copy link

jeremyckahn commented Mar 13, 2019

Yikes! Take your time and stay safe, everyone! 😬

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Mar 15, 2019

I've pushed changes to:

  • Update to Shifty 2.7.4-alpha.2
  • Use shifty.Scene, that is associated to a gdjs.RuntimeScene, paused/resume if necessary. All Tweenable are created on this scene. See b12b2c0#diff-a835c7461427b8d259a5e2468b068dbaR928 for the interesting parts
  • I've updated the example to allow to start, stop scene.

Example works well like before buuuut not the pausing: when I start a new scene, the tweens from the previous one are still applied!

  • I can see this because I start a tween on Scene 1, press "p" to create a new scene ("Scene 2") (the Scene 1 will stop being rendered by GDevelop. The shifty.Scene associated to it have the pause function called), then I wait a bit (I can launch new tweens in Scene 2 without issues) and then I press "m" to go back to the Scene 1. Scene 2 is stopped (I call .stop() on the shifty.Scene of Scene 2 - though I'm not sure if it's really needed) but I can see that Scene 1 tweens have been updated while I was on the other. It's pretty clear because if I wait for too long, all the tweens will be finished, and object will "teleport" to the end position.
  • I've checked and the game engine seems to properly call the pause/resume. I checked isPlaying:
    Screenshot 2019-03-14 23 45 57
    (0 is Scene 1 (paused), 1 is Scene 2 (played))

@jeremyckahn do you have an idea why the scene will be continuing to play? You can take a look at how I create Tweenable here: b12b2c0#diff-a835c7461427b8d259a5e2468b068dbaR928
Pause/resume is simply this: b12b2c0#diff-a835c7461427b8d259a5e2468b068dbaR967 and as shown before they seems to be properly called.

Also would need your input to know how to "delete"/"unload" a scene. For now when a gdjs.RuntimeScene is unloaded, I call stop on the shifty.Scene and hope that it will drop all references to tweens will be dropped. b12b2c0#diff-a835c7461427b8d259a5e2468b068dbaR959
But maybe that's not the case and we have to stop/remove all the individual tweens to be sure that they are garbage collected?

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Mar 15, 2019

@Wend1go Hope you're all right!
Note that I've pushed on your branch, including a merge from master. Unfortunately, this is showing all the new commits from master on the PR :( You can fix this by closing this PR and re-creating a new one: master...Wend1go:tween_extension

Stay safe!

@jeremyckahn

This comment has been minimized.

I don't think this is necessary. The first time through this function, it will call play but there will be no Tweenables to play. The play state is not used as Tweenables are added to a Scene.

@jeremyckahn

This comment has been minimized.

Copy link

jeremyckahn commented on Extensions/TweenBehavior/tweenruntimebehavior.js in b12b2c0 Mar 15, 2019

This will stop the playing the Tweenables, but if your intention is to remove the Tweenables from the Scene entirely, you will need something like this:

runtimeScene.shiftyJsScene.tweenables.forEach(
  runtimeScene.shiftyJsScene.remove.bind(runtimeScene.shiftyJsScene)
);

Do you think it's worth having some sort of Scene#empty function to go along with Scene#remove?

This comment has been minimized.

Copy link

jeremyckahn replied Mar 15, 2019

To be clear: You should unload the Tweenables after you stop the Scene.

This comment has been minimized.

Copy link
Owner Author

4ian replied Mar 15, 2019

Do you think it's worth having some sort of Scene#empty function to go along with Scene#remove?

Maybe yes. As Shifty is "manually managing memory" by using a linked list, might be useful to explictly have a function to remove all the tween from a scene. Otherwise, people using shifty.Scene might assume that doing a "stop" is enough.
For now I'll use the forEach method :)

This comment has been minimized.

Copy link

jeremyckahn replied Mar 15, 2019

Ok, I can add an empty function this weekend! I can see where the semantic disconnect is coming from; I'll also update the API docs.

As for memory management and the linked list: Tweenables are only ever in the linked list when they are actively running (as in not paused or stopped). pauseing/stopping a Tweenable removes it from the list.

@jeremyckahn

This comment has been minimized.

Copy link

jeremyckahn commented Mar 15, 2019

Also would need your input to know how to "delete"/"unload" a scene. For now when a gdjs.RuntimeScene is unloaded, I call stop on the shifty.Scene and hope that it will drop all references to tweens will be dropped. b12b2c0#diff-a835c7461427b8d259a5e2468b068dbaR959
But maybe that's not the case and we have to stop/remove all the individual tweens to be sure that they are garbage collected?

Yes I think the current approach is problematic — I think you'll need to explicitly empty the Scene as I described in this comment. The removed Tweenables should be garbage collected assuming nothing else has a reference to them after that.

I don't know that this will solve all the problems you're seeing, but I think it should help to make the errant behavior a bit easier to understand. Please give that cleanup code a try and then let's figure it out from there!

@jeremyckahn

This comment has been minimized.

Sanity check: Is this line running more than once and creating multiple Scene instances? I am expecting that you would want it to. If there is only even one Scene (effectively a singleton) then this might cause some of the weird behavior you're experiencing.

(This may be a silly question, but I am not familiar with how GDevelop works. 🙂)

This comment has been minimized.

Copy link
Owner Author

4ian replied Mar 15, 2019

For one given runtimeScene, this will be run only once. Multiple scenes will be created by GDevelop, but only when needed (when you pause a scene and start a new one, a new gdjs.RuntimeScene is pushed on the top of the stack of living scenes).

But this means that when the first tweens are launched, only one shifty.Scene is existing. Then, at some point in the game, a new shifty.Scene will be dynamically created (or maybe more, if you launch other scenes). Then when you stop a scene and go back to the previous one, the gdjs.RuntimeScene is removed from the stack (and shifty.Scene stop is called).

Not sure if your API is actually supporting dynamically adding/removing scenes?

This comment has been minimized.

Copy link

jeremyckahn replied Mar 15, 2019

Ok I think we are on the same page with the intention of this code. 👍

Multiple independent Scenes are supported, as you can see in this CodePen. I don't think that's the issue here. However, Scene does not do any automatic cleanup, if that's that what the assumption here is.

This comment has been minimized.

Copy link
Owner Author

4ian replied Mar 15, 2019

Ok, I'll give it another try with proper cleanup. But still strange that the first scene is continuing while it should be paused... in your example, the difference that I see is that both scene are created at the same time.
Will tell you how it goes with cleanup :)

This comment has been minimized.

Copy link

jeremyckahn replied Mar 15, 2019

Yes I don't think what I'm suggesting will fully solve the problem, but it should hopefully get our mental models more in alignment and make it easier to debug! 🙂

@Wend1go

This comment has been minimized.

Copy link
Contributor Author

Wend1go commented Mar 15, 2019

I'm fine, thanks for asking. ^^
And my internet connection is back again, yay. 🎉

@4ian Shall I

  1. pull your changes
  2. close this PR and
  3. create a new PR

Or do I need to do something else in order to get rid of the commits from master?

@4ian

This comment has been minimized.

Copy link
Owner

4ian commented Mar 15, 2019

You can pull the changes from this branch, as usual (PR and branch are "disconnected". A PR is just something on GitHub that points to a branch). Then on GitHub, close this PR and open a new one, like you did for this one.
For some reason GitHub is showing the commits from master here, but when you look here: master...Wend1go:tween_extension you can see that it's fine, only the commits from the tween_extension branch are displayed.

@Wend1go Wend1go closed this Mar 15, 2019

@Wend1go Wend1go referenced this pull request Mar 15, 2019

Open

[WIP] Tween extension - Reloaded #980

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.