Animation Framework Overhaul

[jwoolston]

Continuing from #567, it seems that there is a general consensus that the animation system needs an overhaul.

Everyone seems to agree that the GL thread should be responsible for managing animation time steps. The exact means of doing this seems to be up in the air still so as @AndrewJo suggested, I am creating a new issue here to continue the discussion.

---

TODO

• Rewrite base animation class.
• Rewrite animation queue
• Add and test new ColorAnimation3D
• Adapt and test BezierPath3D
• Adapt and test CatmullRomPath3D
• Adapt and test RotateAnimation3D
• EllipticalOrbitAnimation3D
• Adapt and test RotateAroundAnimation3D (deprecated for EllipticalOrbitAnimation3D)
• Adapt and test RotationAnimation (deprecated and consolidated into RotateAnimation3D)
• Adapt and test ScaleAnimation
• Adapt and test TranslateAnimation3D
• Fully test all core animation functionality
• Update RajawaliExamples to use new animation code.
• Update Rajawali Wiki to use new animation code.

[jayschwa]

I propose an API that looks something like this:

// for user:
play(repeat_count) // will always reset animation
pause(bool) // false is unpause
// for render loop:
update(time) // absolute, not a delta

Absolute time will make it easier to calculate repeats (e.g. (now - start) / duration).

[jwoolston]

@jayschwa so pause here would set an internal flag in the animation which caused it to ignore the update call right?

[jayschwa]

Right. At a very high-level, I'm thinking something like this:

public void play(int repeats) {
    mStart = uptimeMillis();
    mRepeats = repeats;
}

public void pause(bool pause) {
    if (pause) {
        mPause = uptimeMillis();
    else { // unpause
        pause_duration = uptimeMillis() - mPause;
        // prevent anim from lurching forward after unpause
        mStart += pause_duration;
        mPause = 0;
    }
}

public void update(time) {
    if (mPause == 0) {
        repeat_count = (time - mStart) / duration;
        animation_percent = (time - mStart) % duration;
        if (repeat_count < mRepeats) {
            // mutate Object3D to <animation_percent> complete
        }
    }
}

[jwoolston]

Should the RajawaliRenderer keep a list of animations and have the requisite methods to add and remove them so the library can ensure their thread safety similar to children, cameras and such?

[AndrewJo]

To handle pause on per-object basis, you'd probably want to do something similar to way Three.js does it. It's JavaScript but I'm just pasting the code in here as a reference.

Note: Three.js has an AnimationHandler class which keeps a list of Animation classes.

THREE.Animation.prototype.pause = function() {

    if ( this.isPaused === true ) {

        THREE.AnimationHandler.addToUpdate( this );

    } else {

        THREE.AnimationHandler.removeFromUpdate( this );

    }

    this.isPaused = !this.isPaused;

};

[AndrewJo]

Animation stuff from Three.js:
https://github.com/mrdoob/three.js/tree/master/src/extras/animation

I think adapting this code to Rajawali is a good place to start. Why reinvent the wheel?

[jwoolston]

@AndrewJo If I understand correctly then you propose having a static class which keeps a list of all animations and receives an update call from RajawaliRenderer and subsequently calls update on all of the registered animations?

[jwoolston]

I suppose your last comment answers mine

[AndrewJo]

Would static class be better or do we want to have one instance of non-static class in the RajawaliRenderer?

[jwoolston]

Hmm, that is debatable. Static tends to simplify thread safety but if we make a thread safe implimentation and use an instance in the renderer, we have the potential for having animation scene switching without needing to recreate the activity.

[jayschwa]

I think adapting this code to Rajawali is a good place to start. Why reinvent the wheel?

Can't argue with that. I'll defer to whoever actually puts in the sweat on this task. I was just throwing out some ideas since I think the current animation class has a bit of a bloated API.

Would static class be better or do we want to have one instance of non-static class in the RajawaliRenderer?

Wouldn't a static class be shared amongst renderer instances? I think we want to avoid that. Each instance (e.g. active wallpaper vs. preview) should have their own manager.

[jwoolston]

@jayschwa I had not considered that possibility. I agree.

[AndrewJo]

So something like this would make sense then.

public class RajawaliRenderer extends Renderer {
    ...
    protected AnimationHandler mAnimationHandler;
    ...

    protected void render() {
        ...
        mAnimationHandler.update(deltaTime);
        ...
    }

And AnimationHandler.update method would iterate through a List and call mAnimationList.get(i).update(deltaTime) basically.

[jwoolston]

Yes, except I agree with @jayschwa that absolute time might make more sense, especially if we use uptime because it will pause during sleep.

[jwoolston]

Also, using absolute time allows us to defer to the implimentation of the animation when it comes to what that time value represents.

[AndrewJo]

Wouldn't animation pause automagically when the phone goes to sleep anyways because render() method wouldn't be called during sleep?

[jwoolston]

True. I guess the only real reason is for handling repeating easily.

[AndrewJo]

With time deltas, you can still tell when to repeat or stop because if you take the current time - start time, you get the entire duration of the animation. If (currentTime - startTime) is greater than the duration set for the animation, stop, or repeat depending on the option.

Start time would be set when the play() method is first called for that specific animation of course.

[jwoolston]

@AndrewJo Wouldn't this require the animations to then check the absolute time anyway?

[AndrewJo]

I guess. There's like gazillions of ways to skin this cat. But this is what this thread is for: to discuss implementation details.

I'm personally used to time delta based animations coming from JavaScript world. I wouldn't mind implementing it using absolute time either. Let's see what @MasDennis has to say since he's the original brain behind this framework.

[ToxicBakery]

I second delta time based animations. I believe delta time to be the more standardized approach in engines.

[MasDennis]

Good discussion here. I agree that it needs a bit of work.
The original interface is derived from Android's animation classes. I prefer to keep it as close as possible.
However, what aspects of the current API do you think make it bloated @jayschwa ?

I'm personally used to delta time based animations as well. I've used them with JavaScript, XNA, Ogre. I think they're more or less the standard in 3D engines. If it was up to me I'd stick to delta time.

Let's use a non-static class in the main renderer. For thread safety as well as consistency. The texture manager is also non-static and sits in the main renderer. I'm not a big fan of static classes and singletons. Unless they're used to get better performance.

Thanks to whoever is going to pick this up. I'm currently working on the skeletal animation classes (bug fixing, improving, serialization, writing a Blender-Rajawali workflow tutorial) so I can't start working on this right now. I will however pick this up if nobody else can in the coming few weeks.

[jwoolston]

I too will be grateful for this, especially if it comes in the near future. The thread safety issues are making debugging the motion through my scenegraph extremely painful.

To summarize what has been agreed upon so far:

• An animation manager class, which is a thread safe non-static class member of the RajawaliRenderer.
  • The manager keeps a list of registered animations
  • RajawaliRenderer calls an update method on the animation manager, which subsequently calls it on all animations.
• There is a bit of a split on whether absolute time or delta time should be used, however things seem generally in favor of delta time if for no other reason than to stick with the norm. @jayschwa and I are in favor of absolute time. I can't speak for him but one reason I am in favor of it being absolute is to making only a single call to get current time, resulting in more consistent animations, especially if many are running at.

[ToxicBakery]

Alrighty, this is my concept of how animations 'should' be done.

Branch:
https://github.com/ToxicBakery/Rajawali/tree/animation-rewrite

Implementation:
https://github.com/ToxicBakery/Rajawali-Wallpaper-And-Activity-Example/blob/animation-rewrite/src/com/testing/rajawali/MyRenderer.java

Hit me with your comments.

[jwoolston]

@ToxicBakery damn you're fast.

[ToxicBakery]

Powered by Newcastle.

[jwoolston]

Powered by Newcastle.

Crate or regulated drip? http://xkcd.com/323/
Mine is some home brewed Meade.

[ToxicBakery]

Oh btw, I know the overall decision was for a class to manage the animations but it just seemed pointless. If you look at the number of changes in the renderer to support animations, it just did not make sense in my opinion.

[jwoolston]

@ToxicBakery I don't have a problem with it being handled internal to RajawaliRenderer. I do see a potential concurrency issue which also affects mChildren and mPlugins and potentially mLights in BaseObject3D. I am creating an issue shortly for the later, but I'll explain it briefly here. Essentially CopyOnWriteArrayList does not seem to protect from the case of removing a member during iteration so external locking is still required, or for each/iterator type which we have already discussed has GC issues. As far as addressing that goes, its probably better to wait until I get my other issue posted because it will explain things in greater detail.

[jwoolston]

ok thats how i felt about it. I am leaving them for children and plugins though since they were already in existance. Plus adding for cameras...might be useful

[ToxicBakery]

Sounds good, just FYI to everyone. @jwoolston @MasDennis @jayschwa @AndrewJo

The animation changes are essentially finished. The remaining work lies in the following.

Tasks

A) Finish testing, this will be covered by updating the RajawaliExamples. At the point I feel like I have fully tested everything other than Andrew's EllipticalOrbitAnimation which I believe his wallpaper is proof enough that it works.

B) Update the RajawaliExamples. I will head this up myself in a branch. Simple enough.

C) Updating the wiki is problematic. The RajawaliExamples and the Wiki would ideally be updated at the exact same time. I do not think this is really possible with Github, is it? Additionally it would be good to get help from anyone that has a few minutes for doing the updates. Should be really simple just a little bit of verification and some minor updates.

Misc

Most of the code changes are in the Animation3D class and the new animations. If anyone wants to code review the changes, I won't take any offense to it.

[jwoolston]

@ToxicBakery as long as you give me some notice, I have plenty of time to help with that. Ideal times for me would be in the mornings PST. If you want precisely the same time, no I dont believe simultaneous update is possible.

[jwoolston]

@ToxicBakery I could use a break from the task queue. What do you need me to do? Also, AnimationQueue throws a big wrench in things for the task queue...I'm not sure how it should fit in.

[ToxicBakery]

Really the thing to do is update the docs (wiki). I should be able to solo the RajawaliExamples pretty quickly tomorrow. If I am not busy at work tomorrow I can probably do it in the morning pretty quickly.

AnimationQueue may be something that should be moved in the proposed AnimationManager. Actually when I was going over the queue, I am really not sure it is all that useful. It does not do much more than what the user could implement themselves. A more robust system for the queuing may be ideal. I will have think on this as I have more animation changes I want to make I just have not gone through enough of the process to determine what exactly the changes entail/need.

[jwoolston]

Ok. For now I will assume the AnimationQueue to be in flux and just not support it ATM.

[AndrewJo]

I don't use the AnimationQueue for my intro cinematic in my live wallpaper. I fired off the start() for the next animation when previous animation ended.

[MasDennis]

Great work guys. Can't believe how quickly Rajawali has improved thanks to your contributions.

[jayschwa]

@AndrewJo I just added you as a collaborator. I thought I already did that but this wasn't the case. Welcome!

Awesome. It's about time!

In general, I've been pleased with the recent flurry of activity on the project. There was a period of time where I feared the project would languish, but getting some fresh blood on board in recent weeks has assuaged those concerns. Welcome! 🐃

[MasDennis]

@jayschwa +1. I've been very pleased with all this activity. It's hard to keep up with it actually 😮

[AndrewJo]

Just finished porting the RajawaliExamples to the new animation framework! Going through each examples and testing it now... Once it's done, I'll commit it.

[AndrewJo]

There seems to be a bug in the Catmull-Rom spline animation. It seems to be a problem in the animation framework itself and not the RajawaliExamples so I'll still go ahead and commit it.

I might also add that the animations are much more buttery smooth in the examples and lighter on the resources since we're not spawning Timer instances! 👍

[AndrewJo]

Committed to my branch on my repo. @MasDennis, would you mind pulling my RajawaliExamples' animation-update branch? I don't have collaborator status on the other repo.

Everything works except animation queues and Catmull-Rom splines.

[MasDennis]

I just added you and @jwoolston as collaborators on both the RajawaliExamples and RajawaliWallpaperTemplate repositories.

[AndrewJo]

Pushed animation-update branch to RajawaliExamples.

[jwoolston]

I just added you and @jwoolston as collaborators on both the RajawaliExamples and RajawaliWallpaperTemplate repositories.

Awesome. Danke schon. <- Insert umlaut here

@ToxicBakery When I started working on updating the wiki it occurred to me that the examples Dennis created there were more or less still correct, so I made some modifications and explained the major changes. If I missed something overwhelmingly obvious, please point it out.

[ToxicBakery]

Sounds about right. For the most part not much has changed. We dumped some of the ridiculous constructors, past that you have to register the animations with the renderer.

[jwoolston]

Ok. Should I add a EllipticalOrbitAnimation bit to it?

[ToxicBakery]

I do not think it is really necessary. It will exist in the RajawaliExample library. Also the wiki page is getting a but unmanageable. We should probably look at sectioning it.

[jwoolston]

Yea I was realizing that the other day. I'm going to have my hands full with explaining the new task queue and scene graph when I finish it (that one is going to be long I expect). I think we should have some pages dedicated not so much to usage but mechanics, that way people can understand the inner workings a little easier...might eliminate some otherwise trivial support issues.

[jwoolston]

@ToxicBakery I have merged your animation branches into my task queue branches in the library and examples. At the moment all I did to integrate your animations is replace the implimentation of RajawaliRenderer.registerAnimation() and RajawaliRenderer.unregisterAnimation() to call the queue methods. Should we leave it this way or remove those methods and have users call the queue methods?

[ToxicBakery]

The users should probably call the queue methods to keep it consistent I suppose.

[ToxicBakery]

@MasDennis I am satisfied with the code as it stands to move into the master. The wiki can be updated pretty quick to match and the RajawaliExamples are essentially good to go except for the animation example which I am about to go look at and the catmulrom example which I think you mentioned fixing (might have mixed something up there).

So, if you are happy with the changes feel free to do the honors.

EDIT
Animation example is now fixed on the animation-rewrite branch.

[jwoolston]

Animation example is now fixed on the animation-rewrite branch.

Woo! I thought I broke it. I'm excited, maybe I won't be beating my head on my keyboard over updating the scene graph as an object moves now.

[jwoolston]

Also, I'd like to point out that this has been a pretty epic issue thread.

[AndrewJo]

LOL. Yeah, this has got to be the longest issue thread for Rajawali.

[Davhed]

Iz in yer thread addin mah posts!

[jwoolston]

I do believe this has concluded!