AnimationFrameController: requestAnimationFrame made easy

Question

I am looking for feedback on what I can improve to a tool I have been using for several months now, my AnimationFrameController. I find requestAnimationFrame an incredibly hassle and hard to control, so this controller does it for you. Are there better/more efficient ways to do this? The below is an example of how to use the code and the ES6 version of the source.

The main reason I built this is to be able to quickly throw together animation or things you can do every frame, but in the requestAnimationFrame function you get exactly one call, and you better hope it calls all animations etc... With my animationFrameController its as easy as going AF.add( myFunction ) and it will be called continuously until it gets removed (by AF.remove) or it returns false itself and cancels itself out. The animationFrame ends when no more handlers remain.

"use strict";

/*
 * AnimationFrameController 1.0
 */

class AnimationFrameController {
	
  constructor( autostart = true ){
		
    this.autostart = true;
    this.paused = true;
    this.time = 0;
    this.fps = 0;
    
    this.callees = [];
    this.calleesTime = [];
		
  }
	
  get time(){
    return this._time;
  }
	
  set time(v){
    this.fps = (v - this.time) / 1000;
    this._time = v;
    return this._time;
  }
	
  get paused(){
    return this._paused;
  }
	
  set paused(v){
    this._paused = v;
    if( this._paused === false ){
      this.loop( this.time );
    } 
  }
	
  add( ...handlers ){
	
    handlers.forEach( handler => {
      if(handler instanceof Function){
        this.callees.unshift( handler );
        this.calleesTime.unshift( this.time );
      }
    });
    if(this.callees.length && this.autostart && this.paused) this.paused = false;
		
  }
	
  remove( ...handlers ){
	
    handlers.forEach( handler => {
      let index = -1;
      while((index = this.callees.lastIndexOf( handler )) >= 0){
        this.callees.splice( index, 1 );
        this.calleesTime.splice( index, 1 );
      }
    });
    if(this.callees.length <= 0) this.paused = true;
		
  }
	
  loop( time ){
		
    try {
	
      if( !this.paused ){
	
        window.requestAnimationFrame( this.loop.bind(this) );
		
        let delta = time - this.time;
        this.time = time;
		
        this.callees.map(( handler, index ) => {
          let progress = time - this.calleesTime[index];
          return handler( delta, progress ) === false ? handler : true; 
        }).forEach(handler => {
          return handler !== true ? this.remove(handler) : 0; 
        });
		
      }
	
    } catch(e) {
	
      this.paused = true;
      console.warn('An error has occurred while running the AnimationFrame.', e);
		
    }

  }

}





/* IMPLEMENTATION */

var AF = new AnimationFrameController;
var button = document.getElementById('clickme');
var animation = false;
var x = 0, y = 0;

button.addEventListener('click', function( event ){
  var fromX = x;
  var fromY = y;
  var toX = Math.round( Math.random() * (window.innerWidth - button.clientWidth) );
  var toY = Math.round( Math.random() * (window.innerHeight - button.clientHeight) );
  AF.remove( animation );
  AF.add(animation = function( delta, progress ){
    var now = progress / 2000;
    if( now >= 1 ){
      x = toX;
      y = toY;
      return false;
    } else {
      x = fromX + (toX - fromX) * now;
      y = fromY + (toY - fromY) * now;
    }
  });
});

AF.add(function(){
  button.style.left = x + 'px';
  button.style.top = y + 'px';
});

<input type="button" value="click me" id="clickme" style="position:absolute;" />

(You can find an ES5 version over at github if you want to try it in al older browser: https://github.com/jasperlinsen/AnimationFrameController)

Answer 1

What about two AnimationFrameControllers

That will work just fine?

As a games/visualisation programmer I would not use your AnimationFrameController. It is just a callback stack, with ugly bits and would be difficult to use (you are catching my throws), GC unfriendly, carries a bad bug if I use AnimationFrameController.pause = false; and should be a singleton

A breakdown of your code.

Don't use class, its is really bad and makes it so hard to write good javascript. AND You will only ever need one instance of AnimationFrameControler so it should be a singleton.

class AnimationFrameController {
  constructor( autostart = true ){
    this.autostart = true;
    this.paused = true;
    this.time = 0;
    this.fps = 0;

callees & calleesTime do not need to be exposed, programmers will just side step your add/remove function trying to take short-cuts then if you ever change the behaviour you will have a list of upset emails. "Why your code break?" Don't expose what should not be exposed.

Also why keep two arrays, why not just a simple object added to callees array {callback : callback, time : time}

    this.callees = [];     
    this.calleesTime = []; 
  }

Just bad a function call to get a exposed value. Use get to get abstract values not object properties.

  get time(){
    return this._time;
  }

  set time(v){
    this.fps = (v - this.time) / 1000;
    this._time = v;
    return this._time;
  }

Again pause is a property of AnimationFrameController why force a function call to get its value.

  get paused(){
    return this._paused;
  }

BUG: This function is will generate additional requestAnimationFrame loops on top of the current loops.

  set paused(v){
    this._paused = v;  
    if( this._paused === false ){ // if not paused 
      this.loop( this.time );     // call loop which on the 3rd line requests an animation fame without checking if 
                                  // there is one currently pending..
    } 
  }

  add( ...handlers ){
    handlers.forEach( handler => {
      if(handler instanceof Function){
        this.callees.unshift( handler );
        this.calleesTime.unshift( this.time );
      }
    });

Bad style this code would not pass a linter without a pile of opt outs. Always add { }

    if(this.callees.length && this.autostart && this.paused) this.paused = false;
  }
  remove( ...handlers ){
    handlers.forEach( handler => {

Why let for a function scope? Incorrect scope declaration can lead to bugs, use var if the variable is function scoped.

      let index = -1;
      while((index = this.callees.lastIndexOf( handler )) >= 0){
        this.callees.splice( index, 1 );
        this.calleesTime.splice( index, 1 );
      }
    });

Missing {}

    if(this.callees.length <= 0) this.paused = true;
  }
  loop( time ){

This try catch would be very annoying during development, completely unacceptable in release code.

You are not responsible for the functions you call and you should not attempt to circumvent any error handling system they have in place by adding a cover all try catch. If there is a potential error to be thrown by your code then check for it and handle it. Don't use try catch unless you are at the application level.

try {

  if( !this.paused ){

    window.requestAnimationFrame( this.loop.bind(this) );

Is there any particular reason you use let here???

    let delta = time - this.time;
    this.time = time;

The next section is just wrong for way too many reasons. This is an animation controller, the number one enemy of animations is GC and you are just creating dead memory for no reason. Also forEach is SLOW SLOW SLOW just use a standard for loop it is much more efficient.

    this.callees.map(( handler, index ) => {
      let progress = time - this.calleesTime[index];
      return handler( delta, progress ) === false ? handler : true; 
    }).forEach(handler => {
      return handler !== true ? this.remove(handler) : 0; 
    });

Above code rewrite. The following will not use up memory, and be an order of magnitude quicker.

    var i; // << hoist
    for (i = 0; i < callees.length; i += 1) {
        if (callees[i]( delta, time - this.calleesTime[i] ) === false) { 
            this.remove(callees[i]);
            i--; // so you don`t skip a item
        }
    }



  }

} catch(e) {
  this.paused = true;
  console.warn('An error has occurred while running the AnimationFrame.', e);

}