[Fixes #119] Fix textured shape infinite loop

[dy-dx]

Fixes https://github.com/elm-lang/core/issues/119, elm/elm-lang.org#186, & should fix elm/compiler#724.

[evancz]

Awesome, thank you for taking a look at those issues!

I think this introduces a bug in which things can be layered wrong. Say there are three overlapping forms, and the middle one is a textured shape. If we render onload, it'll jump to the top. How can we do a fix and avoid this?

[evancz]

Awesome, thank you for taking a look at those issues!

I think this introduces a bug in which things can be layered wrong. Say there are three overlapping forms, and the middle one is a textured shape. If we render onload, it'll jump to the top. How can we do a fix and avoid this?

[dy-dx]

OK yeah, I tested that (applying my change to 0.14.1) and something funny happens...
http://monosnap.com/file/dQsYPkmJewQ38DPjmLnaMpkoe0IwdB

Here's another thing though, I just looked over Collage.js on master again and it looks like some refactoring was done since 0.14.1, and texture() doesn't even receive a redo function anymore. It's out of scope and probably throws an exception:

https://github.com/elm-lang/core/blob/master/src/Native/Graphics/Collage.js#L55

[dy-dx]

OK yeah, I tested that (applying my change to 0.14.1) and something funny happens...
http://monosnap.com/file/dQsYPkmJewQ38DPjmLnaMpkoe0IwdB

Here's another thing though, I just looked over Collage.js on master again and it looks like some refactoring was done since 0.14.1, and texture() doesn't even receive a redo function anymore. It's out of scope and probably throws an exception:

https://github.com/elm-lang/core/blob/master/src/Native/Graphics/Collage.js#L55

[evancz]

Hmm, I wonder if that happened when we added support for Graphics.Collage.text?

My understanding is that the root issue is that normally images are cached by the browser, and there was a time when chrome would not trigger onload if the image was already cached. This changed at some point, and things got loopy. If there is a way to detect if the image is available, we can avoid setting up the onload listener and avoid the loops.

Does that sound like a plausible plan?

[evancz]

Hmm, I wonder if that happened when we added support for Graphics.Collage.text?

My understanding is that the root issue is that normally images are cached by the browser, and there was a time when chrome would not trigger onload if the image was already cached. This changed at some point, and things got loopy. If there is a way to detect if the image is available, we can avoid setting up the onload listener and avoid the loops.

Does that sound like a plausible plan?

[dy-dx]

OK, I tweaked the trace and textured functions a little more and got the original behavior that you predicted: http://monosnap.com/file/V15GgpZ7EUtYIoUyhJGPvCPqJIJgSJ

I think the only way to deal with the async onload problem is to preload all the images before doing any rendering. Like this:
http://www.html5canvastutorials.com/tutorials/html5-canvas-image-loader/

[dy-dx]

OK, I tweaked the trace and textured functions a little more and got the original behavior that you predicted: http://monosnap.com/file/V15GgpZ7EUtYIoUyhJGPvCPqJIJgSJ

I think the only way to deal with the async onload problem is to preload all the images before doing any rendering. Like this:
http://www.html5canvastutorials.com/tutorials/html5-canvas-image-loader/

[evancz]

Maybe we can have a cache of images that are in use associated with a collage? So whenever we see a texture, we add it to the cache, and whenever something loads there we do a rerender. If the image we need is in the last-frame-cache, we can just try to draw it and if it's not ready, we'll still get the rerender when it's done.

One issue with a cache is "how big should it get" but if it only ever holds stuff that was drawn last frame, I think we solve it in a plausible way.

[evancz]

Maybe we can have a cache of images that are in use associated with a collage? So whenever we see a texture, we add it to the cache, and whenever something loads there we do a rerender. If the image we need is in the last-frame-cache, we can just try to draw it and if it's not ready, we'll still get the rerender when it's done.

One issue with a cache is "how big should it get" but if it only ever holds stuff that was drawn last frame, I think we solve it in a plausible way.

[dy-dx]

I gave that a try (quick and dirty), using the image src as a cache key:

      function texture(redo, ctx, src) {
-         var img = new Image();
+         var img = getImage(src);
          img.src = src;
-         img.onload = redo;
+         img.onload = img.onload || redo;
          return ctx.createPattern(img, 'repeat');
+     }
+ 
+     var ImageCache = {};
+ 
+     function getImage(src) {
+       ImageCache[src] = ImageCache[src] || new Image();
+       return ImageCache[src];
      }

Works great actually! No more infinite loop, and the change isn't intrusive at all.

But again, this is a change to the 0.14.1 version... I don't know how to test against master, I can't get it working locally.

[dy-dx]

I gave that a try (quick and dirty), using the image src as a cache key:

      function texture(redo, ctx, src) {
-         var img = new Image();
+         var img = getImage(src);
          img.src = src;
-         img.onload = redo;
+         img.onload = img.onload || redo;
          return ctx.createPattern(img, 'repeat');
+     }
+ 
+     var ImageCache = {};
+ 
+     function getImage(src) {
+       ImageCache[src] = ImageCache[src] || new Image();
+       return ImageCache[src];
      }

Works great actually! No more infinite loop, and the change isn't intrusive at all.

But again, this is a change to the 0.14.1 version... I don't know how to test against master, I can't get it working locally.

[evancz]

To make this change practical, the cache needs to be able to forget stuff. If we use a new image every minute, the cache will just grow forever, so I don't think this is a full solution yet.

In terms of getting it into master, once we have something that has reasonable caching behavior I can test it out on my build. So when it's ready, just make sure the PR is against master and I can work out any extra details. It may be easier to get things building locally soon, so keep an eye on elm-discuss for news about that.

[evancz]

To make this change practical, the cache needs to be able to forget stuff. If we use a new image every minute, the cache will just grow forever, so I don't think this is a full solution yet.

In terms of getting it into master, once we have something that has reasonable caching behavior I can test it out on my build. So when it's ready, just make sure the PR is against master and I can work out any extra details. It may be easier to get things building locally soon, so keep an eye on elm-discuss for news about that.

[dy-dx]

How's this look?

      function texture(redo, ctx, src) {
-         var img = new Image();
-         img.src = src;
-         img.onload = redo;
+         var img = getImage(redo, src);
          return ctx.createPattern(img, 'repeat');
+     }
+ 
+     var ImageCache = {};
+ 
+     function getImage(redo, src) {
+         var img = ImageCache[src];
+         if (!img) {
+             img = ImageCache[src] = new Image();
+             img.src = src;
+             img.onload = function() {
+                 redo();
+                 ImageCache[src] = null;
+             }
+         }
+         return img;
      }

edit: I updated the pull request so you can test it. Hope it works!

[dy-dx]

How's this look?

      function texture(redo, ctx, src) {
-         var img = new Image();
-         img.src = src;
-         img.onload = redo;
+         var img = getImage(redo, src);
          return ctx.createPattern(img, 'repeat');
+     }
+ 
+     var ImageCache = {};
+ 
+     function getImage(redo, src) {
+         var img = ImageCache[src];
+         if (!img) {
+             img = ImageCache[src] = new Image();
+             img.src = src;
+             img.onload = function() {
+                 redo();
+                 ImageCache[src] = null;
+             }
+         }
+         return img;
      }

edit: I updated the pull request so you can test it. Hope it works!

[evancz]

I think this means everything just reloads tons of times, but maybe not in an infinite loop. So it feels like this cache is not doing it's work right.

I'd probably structure the rendering code to pass around an old cache and a new cache (instead of a redo function). This would mean that you'd look in the old cache for an image source string, only firing off a request if it's not there. You then add the source string to the new cache, indicating that it's getting loaded.

I don't know about all this really, but that's the direction to go.

[evancz]

I think this means everything just reloads tons of times, but maybe not in an infinite loop. So it feels like this cache is not doing it's work right.

I'd probably structure the rendering code to pass around an old cache and a new cache (instead of a redo function). This would mean that you'd look in the old cache for an image source string, only firing off a request if it's not there. You then add the source string to the new cache, indicating that it's getting loaded.

I don't know about all this really, but that's the direction to go.

[dy-dx]

Argh, my pull request actually goes into an infinite loop when there are 2 different image sources being rendered. I'm awful at this, thank you for bearing with me.

Anyway -- here's something that works. It is based on your other comment about a "last-frame-cache":

var OldImageCache = {};
var ImageCache = {};

function texture(redo, ctx, src)
{
    var img = getImage(redo, src);
    return ctx.createPattern(img, 'repeat');
}

function getImage(redo, src)
{
    var img = OldImageCache[src] || ImageCache[src];
    if (!img)
    {
        img = ImageCache[src] = new Image();
        img.src = src;
        img.onload = redo;
    }
    ImageCache[src] = img;
    return img;
}

function update(div, _, model)
{
    /* ... */
    OldImageCache = ImageCache;
    ImageCache = {};
}

In this example, redo() gets called a total of 2 times:

main : Element
main =
  collage 640 480
    [
      textured "http://placehold.it/100x100" (circle 100)
    , textured "http://placehold.it/200x200" (circle 100)
    , textured "http://placehold.it/200x200" (circle 100)
    ]

---

I'm confused by your latest comment --

For me at least, image.src = "foo" almost always loads an image asynchronously, even when it's already in the browser's cache. So we need to rerender the entire collage when any image texture's onload event fires.

Therefore, we can't avoid passing the redo function to texture(), right?

[dy-dx]

Argh, my pull request actually goes into an infinite loop when there are 2 different image sources being rendered. I'm awful at this, thank you for bearing with me.

Anyway -- here's something that works. It is based on your other comment about a "last-frame-cache":

var OldImageCache = {};
var ImageCache = {};

function texture(redo, ctx, src)
{
    var img = getImage(redo, src);
    return ctx.createPattern(img, 'repeat');
}

function getImage(redo, src)
{
    var img = OldImageCache[src] || ImageCache[src];
    if (!img)
    {
        img = ImageCache[src] = new Image();
        img.src = src;
        img.onload = redo;
    }
    ImageCache[src] = img;
    return img;
}

function update(div, _, model)
{
    /* ... */
    OldImageCache = ImageCache;
    ImageCache = {};
}

In this example, redo() gets called a total of 2 times:

main : Element
main =
  collage 640 480
    [
      textured "http://placehold.it/100x100" (circle 100)
    , textured "http://placehold.it/200x200" (circle 100)
    , textured "http://placehold.it/200x200" (circle 100)
    ]

---

I'm confused by your latest comment --

For me at least, image.src = "foo" almost always loads an image asynchronously, even when it's already in the browser's cache. So we need to rerender the entire collage when any image texture's onload event fires.

Therefore, we can't avoid passing the redo function to texture(), right?

[evancz]

That's looking really close! My last comment is basically suggesting the changes you made, but without any global variables. Caches would be passed around as arguments. Imagine having two collages on screen each showing different textures and throwing out the cache from the other!

We don't necessarily want to redraw the frame that triggered the image load, because frames may have happened in between and we'd be jumping back in time! I forget if redo is doing the current frame or the most recently rendered frame, but that's something we'd want to take into account. Maybe it does need to still be an argument though, I'm not super sure.

My understanding is that if the browser has the image cached, it is available immediately when you try to render it. Maybe I am wrong about that though?

[evancz]

That's looking really close! My last comment is basically suggesting the changes you made, but without any global variables. Caches would be passed around as arguments. Imagine having two collages on screen each showing different textures and throwing out the cache from the other!

We don't necessarily want to redraw the frame that triggered the image load, because frames may have happened in between and we'd be jumping back in time! I forget if redo is doing the current frame or the most recently rendered frame, but that's something we'd want to take into account. Maybe it does need to still be an argument though, I'm not super sure.

My understanding is that if the browser has the image cached, it is available immediately when you try to render it. Maybe I am wrong about that though?

[dy-dx]

OK, I'll give it a shot.

Regarding browser image caching: From my testing with Chrome, the only way to work with an image without having to use the onload callback is if the browser is already caching the image and you visit the page via hyperlink or by pressing Enter in the location bar. Refreshing the page, however, tells Chrome to redownload the image (or settle for a 304 Not Modified response).

[dy-dx]

OK, I'll give it a shot.

Regarding browser image caching: From my testing with Chrome, the only way to work with an image without having to use the onload callback is if the browser is already caching the image and you visit the page via hyperlink or by pressing Enter in the location bar. Refreshing the page, however, tells Chrome to redownload the image (or settle for a 304 Not Modified response).

[dy-dx]

Uh oh! redo does draw an old frame when it gets called from an async callback.
How can we redraw the latest frame instead?

[dy-dx]

Uh oh! redo does draw an old frame when it gets called from an async callback.
How can we redraw the latest frame instead?

[evancz]

I'm not sure what the current state of things is here, but all the Graphics.* modules have moved to evancz/elm-graphics so it makes sense to retarget stuff like this.

[evancz]

I'm not sure what the current state of things is here, but all the Graphics.* modules have moved to evancz/elm-graphics so it makes sense to retarget stuff like this.