Animated GIF masking #5508
Animated GIF masking #5508
Conversation
|
🎉 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! |
resolves processing#5174 When the mask method is called on an Image that contains an animated GIF, the mask is applied to all of its frames.
yifanmai
force-pushed
the
yifanmai/gif-mask
branch
from
f77ab5d to
4828bfa
Compare
|
@yifanmai thank you for this well-written pull request! Sorry for the delay in reading it but luckily there are no merge conflicts yet. The functionality looks good and I appreciate your thoughtful unit tests. I was worried that the performance would be terrible but it wasn't as bad as I feared. The one minor issue that I see is that the first frame painted into the GIF will not be masked due to the way that p5.Image works with frames behind the scenes. I don't think that this PR necessarily needs to account for that as there are workarounds and this fix could come down the road. I would be happy to merge this as-is but I am not as active in the repo right now so I think it would be good to get an opinion from @jesi-rgb who is working on GIF saving functionality this summer. For anyone to reference, here is an example hosted on the editor with a build from this PR. |
|
hey @yifanmai and @stalgiag!! thanks so much for counting on me for this review. while I can definitely review the code and check if something is wrong or at least clashes with what I'm working on, I don't feel quite prepared for this task just yet. All in all, I'm a beginner in this whole javascript and open source thing, but i'll try my best! That said, if you guys think there's someone better suited for this task do not hesitate in including them! |
src/image/p5.Image.js
Outdated
| this.height | ||
| ); | ||
| } | ||
| this.drawingContext.putImageData(prevFrameData, 0, 0); |
There was a problem hiding this comment.
Maybe this is what @stalgiag was referring to, but would this mean that we leave the gif's current frame in its previous, unmasked state? If so, maybe we need to get the current frame index and draw the newly masked frame instead of drawing prevFrameData
There was a problem hiding this comment.
I tried drawing the current frame after the masking is done, but this causes the new test on the current frame to fail. This is surprising to me because I expect that if I put the masked frame using putImageData and then fetch the pixels using get, I should get the masked image back. Also, calling img.reset in the test (to draw the first frame) causes the test to pass. Any suggestions as to what might be going on?
There was a problem hiding this comment.
Are you still getting a test failure? I just tried running locally but it seems to work OK!
There was a problem hiding this comment.
the failing unit tests were due to #5772 .
There was a problem hiding this comment.
Ok great! If that was the only issue, I can take another look through today to make sure I haven't forgotten about anything, but then this PR could be good to go!
| for (let i = 0; i < img.width; i++) { | ||
| for (let j = 0; j < img.height; j++) { | ||
| let alpha = i < 5 && j < 5 ? 255 : 0; | ||
| assert.strictEqual(img.get(i, j)[3], alpha); |
There was a problem hiding this comment.
Thanks for adding tests for the existing static image behaviour too, this is great!
|
Hi @jesi-rgb, it is OK that you are new; I am also a new contributor to this project! Any feedback from you is appreciated. :) |
Resolves #5174
Changes:
PR Checklist
npm run lintpasses