-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
fixed the bug of black-frames #8132
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
fixed the bug of black-frames #8132
Conversation
hi maintainer @perminder-17 |
"Hi @perminder-17, just wanted to check in on this PR for issue #8123 when you have a moment. No rush, just wanted to make sure it doesn't get lost. Thanks for your time!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes you made for adding a new option to saveGif
looks good, but there's something more required for correctly fixing this bug. I'll debug it myself and let you know what could be the best solution, meanwhile feel free to give your thoughts on this PR as well?
@perminder-17 Thanks for the feedback! Sounds good — please go ahead with debugging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks a lot for your patience and for working on this.
After debugging, I think we should replace:
await Promise.resolve();
with:
await new Promise(requestAnimationFrame);
Why: requestAnimationFrame
waits for the next paint. That means (1) before we start capturing it guarantees the canvas has actually drawn once, and (2) after each redraw() it waits until that frame is shown on screen. So when we read pixels, we get the real, presented frame, no more black first frame.
One note: this may still leave timing (like deltaTime/millis) a bit off during GIF export, but that’s a larger, separate issue and we don’t need to solve it in this PR.
Please keep the new saveGif option you added. It’s useful because different workflows need different starts:
For perfect loops or exact ranges, people may want to start from frame 0 (e.g., record frames 0-60 exactly).
For evolving/interactive sketches, starting “from wherever you are right now” is helpful, you can wait for the interesting moment and begin recording then.
The option lets users choose what fits their case. It isn’t required to fix the black-frame bug, but it’s a good feature to support both use cases.
If you can update the PR with the await new Promise(requestAnimationFrame)
change and keep the option as-is, that’d be great. Thanks again!
@perminder-17 hi maintainer !! and once again thanks for maintaining the Project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On line number 383, remove await Promise.resolve();
and add await new Promise(requestAnimationFrame);
. Thanks for your work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
“Hey @perminder-17 , the workflows are awaiting approval. Could you please approve and run them so the checks can complete?” |
@all-contributors please add @ayushman1210 for code |
I've put up a pull request to add @ayushman1210! 🎉 |
Thankyou Maintainer @perminder-17 for your support and maintaining the project |
Thanks for your work and your patience. I can help review your PRs, not to worry. Thanks |
fix(saveGif): Resolve black frames and add 'reset' option
This PR addresses a long-standing bug in
saveGif()
that caused the initial frames of a generated GIF to be black. It also introduces a new feature, as suggested by the @davepagurek , to provide users with more control over the recording process.🐛 The Bug
When calling
saveGif()
, the first few frames of the output GIF were often black. This was caused by a race condition:saveGif
function manually callsthis.redraw()
to render a single frame.redraw()
is asynchronous and only schedules a paint. The pixel capture was happening before the browser had a chance to complete the rendering, resulting in a capture of a blank (black) canvas.✨ The Solution
This PR implements a two-part solution to fix the bug while preserving the function's intended design.
1. Synchronized Frame Capture
The core timing bug is resolved by
await
-ing arequestAnimationFrame
promise immediately afterthis.redraw()
is called. This ensures that thewhile
loop pauses until the frame is fully rendered on the canvas before capturing the pixels. This completely eliminates the black frames.2. New
reset
OptionAs per the @davepagurek feedback, the intentional resetting of
frameCount
is crucial for recording animations from their start. This behavior has been preserved as the default.A new
reset
option has been added to theoptions
object to give users the choice to either reset the animation or record from the current state of the sketch.🚀 Usage
The
saveGif
function now accepts areset
boolean in itsoptions
object, which defaults totrue
.Default Behavior (Resetting the animation)
This will reset
frameCount
to0
and record the animation from the beginning.