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

CSS tween branch #49

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

cainmartin
Copy link
Collaborator

Added a basic CSS tweening system to Tina. This is not a specific feature request / bug, however, the existing outstanding issues (#40-#42) need a working CSSTween class. So I have taken the first steps to implementing the tweening mechanism.

Description of changes

  • The CSSTween class is now inheriting from the NestedTween class.
  • Added a helper function named CSSMap that maps the property to the suffix
  • Updated the Transition object so that it will apply the suffix when appropriate (profiling indicates there has been very little impact on performance)
  • Added new example file "examples/css"

The available CSS properties are:

  • width
  • height
  • left
  • right
  • top
  • bottom
  • opacity

Here is a working JSFiddle:
https://jsfiddle.net/64ruc9oy/2/

Caveats:

  • There is a difference in the way CSS / Sprites handle coordinates - so it was not possible to implement TransitionRelative class in the same way. This may require some refactoring to implement a method that makes sense.
  • There is no support CSS3 transforms as yet - this may require a change to the way properties are passed to the transform objects. Currently a user specifies the properties they wish to tween, however in the case of a transform - the property and the action is not the same (i.e. transform: rotate(7deg); ) - therefore some kind of mapping may be required.

…he following

CSS attributes: top, bottom, left, right, height, width, opacity.

Caveats: Does not currently support relativeTransitions - this may require a
refactor of the way coordinates are handled for CSS objects (vs sprites).
There is also currently no support for transforms.
Fixed formatting
Fixed formatting
Fixed formatting
Fixed formatting
Fixed formatting
Fixed formatting
Uncommented the script link to point to the main repository
Fixed formatting
@bchevalier
Copy link
Contributor

Hi,
Thanks for the PR!

I can see the problems with supporting webkitTransform and relative tweening. I also think it can wait.

About the suffix property this._suffixMap on the Transition object, I think it's too bad that every non-css transition would have to hold an empty object, taking more processing power and memory than necessary.

You mentioned profiling for performance and did not notice much difference. When I first made benchmark tests for TINA, it could do 12 million tweenings per second (on my machine). Now it is down to 8.5 million for the exact same benchmark. Although I never noticed a difference after each change I made. The changes you introduced to the update methods do not look performance free to me. Do you think there could be another way to do it?

@cainmartin
Copy link
Collaborator Author

Hi!

Thanks for getting back to me.

Yes, I do agree with regards to this._suffixMap. I thought about where to put that for quite some time, but I agree that is not ideal. Ideally, it would belong in the CSSTween object and only be passed on when a CSSTween is created. My reservation was because currently we would need to route that additional information through the NestedTween object - creating some specific code for what is again, a more general object.

Thinking aloud, with no regards to performance - if a generic interface was provided on the object we are tweening, much of this could be hidden within that object. All of the objects that are in that object graph would be unaware of what type of object they were passing on (DOM, sprite, whatever), and would not need any specific code to handle it. In that scenario the suffix could be set in the CSSTween object (where it belongs) and only applied when worked upon in the Transition object. I'd need to look into how it would affect performance though.

With regards to performance - you are correct. My test was unfortunately flawed. Would it be possible to get access to the test case that you are using? The reduction in performance you are seeing is quite dramatic - is that for all types of tween? Or when switching to a CSSTween?

If it is only on CSSTweens - then the string concatenation that is required to set the CSS values is almost certainly the issue. The Style object requires a unit of measurement to recognise the change - so as far as I can tell, I can't perform the Tween without a string concatenation. The + operator is probably the fastest method to achieve this too - so it will be a challenge to improve on.

However - if you can give me access to your tests I can certainly try some options.

Apologies for the long reply - hope that helps. :)

@bchevalier
Copy link
Contributor

Hi again and sorry for the late replies,

About this._suffixMap, I think we should think about what could be a convenient API for TINA. Do you think it should be possible to write a CSS Tween as follow?

var myCssTween = new TINA.CSSTween(myDiv, ['width'])
.from({
        width: '100%'
}).to({
        width: 200
}, 1000)
.start();

The tweening library GSAP is able to tween between properties with different units, e.g from px to %. In this case, I think the suffix map should belong to the transition, not the tween.

Can you provide an example of how the generic interface you are talking about would be used?

About the performance, the figures I gave was not including your changes. I wanted to illustrate that a lot of small modifications that appeared to have no impact on performance had, in the end, a significant negative impact on performance. And that even if you were not able to monitor any difference, there probably is.

In the case of the string concatenation, I think you do not have a choice, as you said, CSS properties need the units to be specified. The issue is the check for a suffix in the map, that now happens for every tween, even non-CSS tweens.

Here is the code I use for benchmarking:

var TINA = require('./src/index.js');

var requestAnimFrame = function(callback){
    global.setTimeout(callback, 1000 / 60);
};

var clock = global.performance || Date;

function addFigureSeparator(nStr) {
    nStr += '';
    x = nStr.split('.');
    x1 = x[0];
    x2 = x.length > 1 ? '.' + x[1] : '';
    var rgx = /(\d+)(\d{3})/;
    while (rgx.test(x1)) {
        x1 = x1.replace(rgx, '$1' + ',' + '$2');
    }
    return x1 + x2;
}

var myTimer = new TINA.Timer().useAsDefault();
var nbTweensForUpdate = 100000; // A high number of tweens
var benchmarkDuration = 2000; // A long enough duration
var aBigDuration = 2375627280; // A number so big we are sure the tweens will not reach the end

var nUpdates = 0;
var totalUpdateDuration = 0;
var running = true;
function updateLoop(time) {
    // Measuring update duration
    var start = clock.now();
    TINA.update(time);
    var updateDuration = clock.now() - start;
    totalUpdateDuration += updateDuration;

    nUpdates += 1;
    if (running === true) {
        requestAnimFrame(updateLoop);
    }
}

var endPosition = { x: 1 };
for (var i = 0; i < nbTweensForUpdate; i += 1) {
    var myObject = { x: 0 };
    TINA.Tween(myObject, ['x'])
        .to(endPosition, aBigDuration)
        .start();
}
// first initialisation
// to make sure all the tweens have been instanciated
TINA.update();

// Starting benchmark test after 500ms to make sure the window (/node) is idle
setTimeout(function () {
    // Setting up animation update
    updateLoop();

    setTimeout(function () {
        running = false;
        myTimer.removeAll();
        var updatesPerSecond = nbTweensForUpdate * nUpdates * 1000 / totalUpdateDuration;
        console.log('Updates per second (TINA):   ', addFigureSeparator(updatesPerSecond.toFixed(0)));
    }, benchmarkDuration);
}, 500);

I run it in node, but to test CSS tweening you would have to run it in a browser.

@cainmartin
Copy link
Collaborator Author

Hi,

I took a good look at the GSAP source - it's CSS handling is very flexible, I like what it's doing there. In theory I don't see why we couldn't do mixed units. There are some complexities to be sure - for instance mixing px with percentages - I think they are doing some smarts to detect when the window is resized, as you can change the window size and the tween will pick up the change with no problems. It looks to me (although I haven't 100% confirmed this) that they are detecting 'dirty' values and recalculating the tween on window resize.

I have taken on a client for the next week or so - so I'm going to have to put this on the back-burner unfortunately - but I will get back to it as soon as I've cleared the deck.

Cheers,

CM

…no longer a per frame check for the CSS Map on non-CSS transitions. Instead, a transition function is selected at the time the transition is created and is passed to the update each frame. While this adds an additional function call - it reduces the memory overhead, and has the advantage of centralising the transition function code.
@cainmartin
Copy link
Collaborator Author

Hi! Sorry - it's been a while.

I've made an update that should rectify the concerns with the initial submission.

What I've done is split the code that does the interpolations out into their own functions. When the Transition object is created, it maps the appropriate interpolation function to the requested update function. The update function passes on the appropriate variables, and the context.

There are two versions of the interpolation function - one with and one without the suffix map. The upshot is the only checks for CSS occur on initialisation, there are no checks being made during the transition, and there are no extra objects created unless it's actually a CSS tween. The other benefit is that the interpolation code is now centralised and there is less code duplication.

Initial tests for performance look promising.

Let me know what you think.

@bchevalier
Copy link
Contributor

Thanks for the update,

Actually one of the changes you introduced:

function updateI(object, t, interpFunc) {
    var p = this.prop;
    object[p] = interpFunc(this, p, t);
  }

could be simplified into:

function updateI(object, t) {
    var p = this.prop;
    object[p] = this.interpFunc(p, t);
  }

Since you actually put the interpFunc property on the transition, which is the object calling the update method on itself.

And the definition of the transformation function:

function standardTrans(context, p, t) {
    return context.from[p] * (1 - t) + context.to[p] * t;
 }

could also be simplified:

function standardTrans(p, t) {
    return this.from[p] * (1 - t) + this.to[p] * t;
 }

Now my only problem is that one more function call was added for non-CSS tweens, also one more property is added to each transition, even if they do not correspond to CSS transitions. There should be a way to avoid both issue.

…w transition type, TransitionCSS - this is responsible for managing transitions for CSS objects, and is only created when a CSS object is presented. The CSS map (provides suffixes for CSS properties) is now a property of the TransitionCSS object.
…w transition type, 'TransitionCSS' - this is responsible for managing transitions for CSS objects, and is only created when a CSS object is presented. The CSS map (provides suffixes for CSS properties) is now a property of the TransitionCSS object.
@cainmartin
Copy link
Collaborator Author

Understood - I think they are great points.

I've gone back to the drawing board a bit - I think there was a far more obvious, and easier solution that I should have investigated.

Instead of jumping through hoops to route the correct update function for the CSS objects, i've simply created a new Transition object, named TransitionCSS. The AbstractTween object now creates this TransitionCSS object only if a CSS object is found.

This completely separates the CSS transitions from the standard transitions. It should also make adding additional features to the CSS transition object a simpler process.

Additionally i've removed the CSSMap from the AbstractTween object (I was never comfortable with this dependency) and given that to the TransitionCSS object.

Any questions, let me know.

@bchevalier
Copy link
Contributor

So now it looks like the extra need for memory and performance impacts exclusively CSS Tweens. Just one more question, wouldn't it be possible to rollback the changes in Transition.js?

var TransitionCSS = require('./TransitionCSS');
var TransitionRelative = require('./TransitionRelative');
var easingFunctions = require('./easing');
var interpolationFunctions = require('./interpolation');

Choose a reason for hiding this comment

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

Indentation is getting a bit out of whack :)
Luckily GitHub makes that very visible. Could you please check your editor's settings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup - doesn't look good. But sorted now. Thanks for the heads up! :)

@cainmartin
Copy link
Collaborator Author

Thanks for the feedback guys - I was out of the office today on some unexpected business. But I'll be able to respond quicker tomorrow.

I've given some feedback and made a few changes based on your comments. I've also reverted the Transition.js file back to it's original state.

Cheers!

@@ -45,6 +45,11 @@ function AbstractTween(object, properties) {
}
}

// Determine if we are are tweening a CSS object
if (typeof CSSStyleDeclaration !== 'undefined') {
this._css = (object instanceof CSSStyleDeclaration) ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it's not necessary to keep the _css`` properties onthis``` since the variable is only used locally

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry actually no, my bad

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.

3 participants