#6164 Fixes inability to display single-frame GIFs #6171
Conversation
- Replaces the callback example with static gif - Adds new basic animated gif example - Minor whitespace fixes
|
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page! |
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
Thanks for taking this on! I think this looks good, I just had one idea I left in a comment for some extra optimization. But if it turns out that that doesn't work, your current implementation is good too!
src/image/loading_displaying.js
Outdated
| // so we have to reset it to the first frame | ||
| pImg.drawingContext.putImageData(frames[0].image, 0, 0); | ||
|
|
||
| pImg.gifProperties = { |
There was a problem hiding this comment.
How do you feel about adding the if statement back just on this block here? If there's just one frame in the gif, if we don't set gifProperties, we can maybe save a few cpu cycles by avoiding trying to calculate what frame it should be on by treating it like a normal image from here on out. At least I think so anyway, your new tests will help confirm it 🙂
There was a problem hiding this comment.
I think that should fix the test that's currently failing, which has the title "single frame gif should not have gifProperties". But if there are other complications, it's not the end of the world if they do have that, so we can talk about updating the test if we can't get it to work otherwise.
There was a problem hiding this comment.
Wow this is great feedback thanks! It's exactly why I was hoping to contribute :)
I totally agree and have submitted the conditional! But I'm kinda new to PRs so I'm not sure if they were added to this one...it looks like they were?
I took some time to look at where else gifProperties is being used but I think this is safe because the first frame is stored into the pImg, which is what eventually gets drawn. One thing to note is that this may prevent the ability add extra frames later (for example, to make use of the automatic GIF features), but that might not be intended anyways
There was a problem hiding this comment.
Right, I think we don't have any officially documented APIs for adding frames to imported gifs so I think we should be good!
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
Great work @ozramos!
|
@all-contributors please add @ozramos for code |
|
I've put up a pull request to add @ozramos! 🎉 |
ghost
mentioned this pull request
1 participant
Resolves #6164
Changes:
\docs\yuidoc-p5-theme\assets\rockies.jpg->rockies.gif)Screenshots of the change:
Manual test is here:
./test/manual-test-examples/p5.Image/load-gifs.html\docs\yuidoc-p5-theme\assets\arnott-wallace-eye-loop-forever.gifPR Checklist
npm run lintpasses