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

Fix image flickering in Graphics.Collage.sprite #456

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@nphollon
Contributor

nphollon commented Dec 4, 2015

I am resurrecting @silverio's pull request #191 from earlier this year. This fixes the performance problem with sprite sheets (see #419 and #332).

Here is a comparison that highlights the improvements. Each of these examples animates this sprite at 30 fps.

The first example uses Graphics.Element.croppedImage. This function inserts an img node into the DOM. The position of the cropping frame gets set in an onload callback. This means that for a moment when the image is loaded, the crop frame is positioned at (0, 0). Leads to a fierce flicker.

The second example uses the current implementation of sprite, which lurks unpublished in Graphics.Collage. On Chrome, I see the frame-rate plummet after about a second. On Firefox, the animation simply doesn't work.

The third example uses the sprite implementation in this pull request. It works great on my machine in both Firefox and Chrome. No flickering or drop in fps.

What do other people see?

EDIT: Fixed hyperlink to the examples page

@thisredone

This comment has been minimized.

Show comment
Hide comment
@thisredone

thisredone Dec 12, 2015

This is not the only place this bug is present.
You might want to also cache the Graphics.Collage.textured
renderForm with form.ctor of FShape calls drawShape which calls setFillStyle which then calls the texture function which also creates a new Image every time.
What's even worse is that it seems that every time an image is created the img.onload forces the update which then creates even more images which do the same over and over again.

It's surprising that this issue has been present for so long. It makes images in Graphics.Collage effectively unusable (and most likely quite frustrating).

Note: the title of this PR doesn't reflect the nature of the problem. It reads like a feature addition but it's actually more like a major bug fix.

thisredone commented Dec 12, 2015

This is not the only place this bug is present.
You might want to also cache the Graphics.Collage.textured
renderForm with form.ctor of FShape calls drawShape which calls setFillStyle which then calls the texture function which also creates a new Image every time.
What's even worse is that it seems that every time an image is created the img.onload forces the update which then creates even more images which do the same over and over again.

It's surprising that this issue has been present for so long. It makes images in Graphics.Collage effectively unusable (and most likely quite frustrating).

Note: the title of this PR doesn't reflect the nature of the problem. It reads like a feature addition but it's actually more like a major bug fix.

@nphollon nphollon changed the title from Image caching for Graphics.Collage.sprite to Fix image flickering in Graphics.Collage.sprite Dec 14, 2015

@nphollon

This comment has been minimized.

Show comment
Hide comment
@nphollon

nphollon Dec 14, 2015

Contributor

@thisredone Thanks for pointing that out. I renamed the issue to highlight the problem instead of the solution. I will take a look at Graphics.Collage.textured when I get a chance.

Contributor

nphollon commented Dec 14, 2015

@thisredone Thanks for pointing that out. I renamed the issue to highlight the problem instead of the solution. I will take a look at Graphics.Collage.textured when I get a chance.

marcinb referenced this pull request in marcinb/elms-farm Jan 9, 2016

Use separate image files for tiles.
Because of an Collage bug making croppedImage flicker
when crop position changes,
see https://github.com/elm-lang/core/pull/456
@villasv

This comment has been minimized.

Show comment
Hide comment
@villasv

villasv Jan 30, 2016

Indeed this makes Craphics.Collage quite unusable for me, because I was doing an Elm demo that had a rolling background. Except in the Edge browser, the flickering makes the game simply unplayable.

Is there any workaround? Maybe specifically applied to croppedImage?

villasv commented Jan 30, 2016

Indeed this makes Craphics.Collage quite unusable for me, because I was doing an Elm demo that had a rolling background. Except in the Edge browser, the flickering makes the game simply unplayable.

Is there any workaround? Maybe specifically applied to croppedImage?

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz May 10, 2016

Member

Sorry for the slow reply. It has been busy times. All the Graphics.* modules have moved to evancz/elm-graphics so it makes sense to retarget stuff like this. There is a lot going on still, but I expect to be thinking about improving <canvas> support and the learning avenue in the future.

Member

evancz commented May 10, 2016

Sorry for the slow reply. It has been busy times. All the Graphics.* modules have moved to evancz/elm-graphics so it makes sense to retarget stuff like this. There is a lot going on still, but I expect to be thinking about improving <canvas> support and the learning avenue in the future.

@evancz evancz closed this May 10, 2016

@nphollon

This comment has been minimized.

Show comment
Hide comment
@nphollon

nphollon May 11, 2016

Contributor

@evancz Should I re-open this PR over on evancz/elm-graphics?

Contributor

nphollon commented May 11, 2016

@evancz Should I re-open this PR over on evancz/elm-graphics?

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz May 11, 2016

Member

Anything that is adding to the public API, I'd rather not add in right now. If there's also a bug fix in here, the bug fix should be moved.

Member

evancz commented May 11, 2016

Anything that is adding to the public API, I'd rather not add in right now. If there's also a bug fix in here, the bug fix should be moved.

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