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

Don't clear canvas by resetting width and height #54

Closed
jwmerrill opened this Issue Dec 15, 2014 · 3 comments

Comments

Projects
None yet
2 participants
@jwmerrill
Contributor

jwmerrill commented Dec 15, 2014

This issue is part of trying to make animations run more smoothly in Firefox.

Currently, on each draw of a collage, the following is executed:

https://github.com/elm-lang/core/blob/9687580e6f7b012cdee712c2f14471b272c85488/src/Native/Graphics/Collage.js#L324

function nextContext(transforms) {
    while (i < kids.length) {
        var node = kids[i];
        if (node.getContext) {
            node.width = w;
            node.height = h;
            node.style.width = w + 'px';
            node.style.height = h + 'px';
            ++i;
            return transform(transforms, node.getContext('2d'));
        }
        div.removeChild(node);
    }
    var canvas = makeCanvas(w,h);
    div.appendChild(canvas);
    // we have added a new node, so we must step our position
    ++i;
    return transform(transforms, canvas.getContext('2d'));
}

Resetting the canvas node's height and width serves two purposes:

  1. Clearing the canvas
  2. Resetting any transforms that were applied to the canvas on the previous frame.

Unfortunately, it appears that resetting the canvas this way also produces a canvas-sized amount of garbage on every frame in Firefox.

Clearing the canvas can also be accomplished by calling ctx.clearRect(0, 0, w, h). However, this does not reset the transforms. Transforms can be reset through disciplined use of ctx.save() and ctx.restore(), but there are currently saves that are never restored:

https://github.com/elm-lang/core/blob/9687580e6f7b012cdee712c2f14471b272c85488/src/Native/Graphics/Collage.js#L313

function transform(transforms, ctx) {
    ctx.translate(w/2, h/2);
    ctx.scale(1,-1);
    var len = transforms.length;
    for (var i = 0; i < len; ++i) {
        var m = transforms[i];
        ctx.save();
        ctx.transform(m[0], m[3], m[1], m[4], m[2], m[5]);
    }
    return ctx;
}

There are two problems here:

  1. No one ever saves before or restores after the translate and scale in the first two lines of this function
  2. When there are additional transforms passed, it appears that the save inside the for loop is not matched anywhere by a corresponding restore.

I think there is a potentially large win to be had by using transforms in a more disciplined way so that it is possible to avoid resetting the canvas width and height on every draw if they have not changed.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Dec 15, 2014

Member

That is a great find! I can imagine that some saves were placed accidentally, or lived through a refactor that they should not have. Are you interested in taking a swing at putting them in the right places? Based on what I recall about the architecture of the rendering code, there should be distinct transitions between levels:

  • step into a group
  • step out of a group
  • step out of everything to embed HTML
  • step back into everything to pick back up with another canvas

I imagine all saves should be on the stepping in and all restores should be on the stepping out. Overall, this sounds like a very plausible improvement!

Member

evancz commented Dec 15, 2014

That is a great find! I can imagine that some saves were placed accidentally, or lived through a refactor that they should not have. Are you interested in taking a swing at putting them in the right places? Based on what I recall about the architecture of the rendering code, there should be distinct transitions between levels:

  • step into a group
  • step out of a group
  • step out of everything to embed HTML
  • step back into everything to pick back up with another canvas

I imagine all saves should be on the stepping in and all restores should be on the stepping out. Overall, this sounds like a very plausible improvement!

@jwmerrill

This comment has been minimized.

Show comment
Hide comment
@jwmerrill

jwmerrill Dec 15, 2014

Contributor

This might be a little premature. I'm doing some experiments with different ways of clearing the canvas, and they don't totally bear out what I asserted above. I'm going to close this until I collect a little more evidence.

Contributor

jwmerrill commented Dec 15, 2014

This might be a little premature. I'm doing some experiments with different ways of clearing the canvas, and they don't totally bear out what I asserted above. I'm going to close this until I collect a little more evidence.

@jwmerrill jwmerrill closed this Dec 15, 2014

@jwmerrill

This comment has been minimized.

Show comment
Hide comment
@jwmerrill

jwmerrill Dec 16, 2014

Contributor

Here's my inconclusive test: http://jsbin.com/vicawitefi/2/edit?js,output

In this version, you switch between strategies by commenting/uncommenting. I'm not seeing a huge difference between them.

Contributor

jwmerrill commented Dec 16, 2014

Here's my inconclusive test: http://jsbin.com/vicawitefi/2/edit?js,output

In this version, you switch between strategies by commenting/uncommenting. I'm not seeing a huge difference between them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment