Skip to content
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

Implement saveGif as a native p5 function #5709

Merged
merged 64 commits into from
Sep 5, 2022

Conversation

jesi-rgb
Copy link
Member

@jesi-rgb jesi-rgb commented Jul 3, 2022

🚧 This branch will be the work in progress for the implementation of the public saveGif function for the '22 GSoC edition. 🚧

Save Gif

resolves #5118

This PR resolves issue #5118, in which a thorough discussion is had about whether p5.js should include a native GIF encoding solution or not.

How it works

In the previously mentioned issue some proposals were discussed about how the interface for this functionality should be. The easiest I found to both implement and use was the following. The saveGif function admits at most 3 arguments for now:

saveGif(filename, duration, delay)
  • filename for the name of the file downloaded to your computer
  • duration duration in seconds that you wish to download from your animation
  • delay delay in seconds that you wish to wait before recording the animation. The animation will nevertheless have a duration of what you specified on the duration argument.

As an example, I think it is best to use it within the mouse/keyboard events:

 function mousePressed() {
   if (mouseButton === RIGHT) {
     saveGif('mySketch', 3, 1);
   }
 }

This function will take all the current frames of the present animation, save them in a buffer and then proceed to generate a palette, apply this palette to every frame, optimize for transparency and encode it in a GIF blob, that will then force to download.

Functionality can be extended in lots of places but I believe that the current state works very conveniently by adding just 3 extra lines of familiar code. I plan on keep contributing to this functionality even after the GSoC to make it as convenient as possible.

Changes

  1. Most important change is in src/image/loading_displaying.js, which is where the main saveGif function lives. Alongside this function, the functions _generateGlobalPalette and _pixelEquals were also added.
  2. Second important change is in src/image/image.js, where the old saveGif function was renamed to a more obscure and explicit encodeAndDownloadGif.
    1. Alongside this change, the file src/image/p5.Image.js was also changed to consider this new function name.
    2. Same goes with the test test/unit/image/downloading.js, which was updated to evaluate the encodeAndDownloadGif function.

⚠ Some other changes

Some other changes were made for the sake of either speed or convenience. These changes are completely opinionated and may include breaking changes that I may not be aware of, so please consider them carefully.

  1. The package.json file is modified to include the package gifenc, created by Matt DesLauriers. He personally reached out to me regarding this topic and it turns out that the package includes two key functions that make the GIF rendering not only much easier, but also much faster.
    1. Naturally, the package-lock.json file also changes.
  2. The Gruntfile.js is also changed. In order to include Matt's package, we needed to first compile the javascript code to babel code, before uglifying it. The uglify module wouldn't process the ES6 notation for some reason, although I've seen in some places that the project is already accounting for that. That is the babel module that appears in lines like grunt.loadNpmTasks('grunt-babel');
    1. Also, the ecmaVersion wouldn't pass the npm run build nor npm run lint nor any other command of this kind altogether. So I bumped up the version to 8 and it seems to be working fine.

Again, all these changes are considered from a rather ignorant and novel standpoint, so maybe I am making very questionable decisions here. I ask the community to pay special attention here, though it does not need to be said that I will try my best for this to be as close to perfect as it can be.

Screenshots of the change:

These are some screenshots of the tool in use. We wrote the function keyPressed and we are tracking the 's' key. When pressed, a gif will be rendered and downloaded. A beautiful sketch by @TomasMiskov is presented.

SCR-20220808-ixh

During the saving and rendering, a little paragraph appears indicating the progress. I feel like this is very important since, even though it usually only takes a couple of seconds, a couple of seconds in web time is an almost infinite amount of time.

SCR-20220808-ixk

The resulting gif is presented here, just shy over 1MB in size:
mySketch (36)

PR Checklist

Copy link
Contributor

@endurance21 endurance21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jesi-rgb good follow up buddy!
left some comments, for next iteration.

@@ -523,10 +533,14 @@ module.exports = grunt => {
grunt.loadNpmTasks('grunt-contrib-clean');
grunt.loadNpmTasks('grunt-simple-nyc');

//this library converts the ES6 JS to ES5 so it can be properly minified
grunt.loadNpmTasks('grunt-babel');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taking jesus solution as of now and moving ahead...

*/
p5.prototype.saveGif = async function(
fileName,
duration,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the second argument is signifying both time and number of frame, is it okay to name it as duration?

how about, count or number ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Count or number are, in my honest opinion, more confusing. In the end, the animation will have a duration of either 30 seconds or 30 frames, which, I think, is understood correctly. Also, the default behaviour is seconds, so those who do not mess with the options will be comfortable.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, lets make it more user friendly.
We will go with the current name then.

// We first take every frame that we are going to use for the animation
let frames = [];

if (document.getElementById('progressBar') !== null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

progressBar sounds so common name, lets name it something that would be unique that perhaps user wont be using .

something like - p5.gif.progressBar

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great, will fix it ASAP!

// stop the loop since we are going to manually redraw
this.noLoop();

while (count < nFrames + nFramesDelay) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. count variable is not intuitive ( what type of count ?)
  2. nframes+ nFrameDelay is making this less perciveable,cannot we make another variable and name it significantly like let xyz = a +b , whree xyz would be some intuitive name rather than writing a+b.

}

// stop the loop since we are going to manually redraw
this.noLoop();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot we let the loop run as it is and meanwhile we extract the pixel colors ?
using this maybe - https://developer.mozilla.org/en-US/docs/Web/API/Canvas_API/Tutorial/Pixel_manipulation_with_canvas

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are doing exactly what you say! We are enabling the loop back just before extracting the pixel colors and start encoding.

What we cannot do is loop while recording the frames, because we need all the frames in order, one by one, and we extract the frame information from the current Canvas context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that make sense a lot!
Great !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be possible to use the existing loop(), but I believe that would require augmenting draw() to keep track of the export frame count, and to record a frame at the end of each export frame. That said, I think I personally prefer this method where we manually call redraw(), as it keeps draw() simple and also it means you can read the gif saving function more easily without having to jump around the codebase to see the data flow.

to render some frames. So we just wait for the frame
to be drawn and immediately save it to a buffer and continue
*/
this.redraw();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@jesi-rgb
Copy link
Member Author

@endurance21 Well! The example errors we talked about are fixed! To be honest, there was nothing to fix on the first place.

Either grunt or firefox or something was caching the old code and preveting the new code to be used in the example. I just removed every p5.js file under /lib and re-built again.

Unless something else can be added, I think we are ready!

}

// stop the loop since we are going to manually redraw
this.noLoop();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that make sense a lot!
Great !

*/
p5.prototype.saveGif = async function(
fileName,
duration,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, lets make it more user friendly.
We will go with the current name then.


// initialize variables for the frames processing
let frameIterator = nFramesDelay;
frameCount = frameIterator;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if it is good to set the system variable from here? and also could you please elaborate why it is necessary ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

frameCount is a variable exposed by the p5.js library upon instancing. Accesing frameCount returns the index of the frame we are currently in, and we can overwrite it in order to travel to a given frame.

This is part of the delay feature. We calculate how many frames we should wait for before recording. Imagine setting delay = 3, which makes it 3 * 60 = 180 (assuming a frameRate of 60). We have to wait until frame 180 to start recording. Cool, simply set frameCount = 180 and then keep on as usual.

This also makes sure that the user, if delay = 0, can record their sketch from the very beginning without extra hassle, since this would effectively set frameCount = 0, even if the animation has already started.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, could this be related to the sketch you mentioned earlier that doesn't work correctly when recording? If a sketch depends partially on frameCount but also has some updates that don't (e.g. a physics simulation that updates each draw()), I wonder if rewinding frameCount might put the sketch into an unexpected state. Maybe there should be a way to keep counting from the current state? e.g. passing undefined instead of a number?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it probably will do that. But for sketches not based on that, I cannot think of a way to somehow attach their time component to something.

I don't know how other systems approach this, but I thought at least we could have support for those who use frameCount as a way of dealing with time, since it is a common practice in shader languages. I myself find using it a lot, and also learned about it in some Coding Train video, which may increase the chance of people using it.

This does not solve the problem at all, of course, but at least we are considering a subset of the users for this matter.

I don't know if implementing a robust solution for every sketch is very difficult, but being already familiar with the codebase, I can work on improving it over the next year independently of GSoC or other events, if I can find a way to do it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure! I think just supporting frameCount for now is totally fine, as long as we leave space in the API to support other things in the future without breaking older code. So the future uses to consider are:

  • sketches that rely on state updates in draw(): I did some tests, it seems like doing state updates in draw() works fine for gif recording! so this is handled already
  • sketches that rely on millis(): that can be done later if we provide a mock implementation of millis, which won't affect any of the code you've already written, so everything's good here for this PR!
  • sketches that mix frameCount with state updates (or millis()): the only issue is that rewinding will bring frameCount out of sync with the others. If we don't rewind, then they're in sync. We don't have to support no rewinding in this PR, e.g. in the future we could support it by supplying delay: undefined and removing the check to ensure it's a number. If that looks ok, then everything here is good without changes 🙂 I just want to make sure we're aware of it in case there's a different API we want for it in the future

@davepagurek
Copy link
Contributor

Also, I notice that gifs turn out all black if I call saveGif inside setup(). I'm not sure exactly why this is happening yet, since this is all that happens between setup() and draw(). I suspect it might have something to do with the first draw is getting called after setup finishes regardless of noLoop() being called, so there could be some double-drawing? e.g. adding this to the top of saveGif fixes it:

if (this._setupDone) {
  await new Promise((res) => window.requestAnimationFrame(res));
}

That's a bit of a hack though, so in any case, I think it's also fine to add something to the documentation saying it has to be called outside of setup for now.

@jesi-rgb
Copy link
Member Author

jesi-rgb commented Sep 2, 2022

@davepagurek yes, I knew that calling within setup causes problems and wanted to make it possible for those that may unintentionally put it there to just work.

Unfortunately, the snippet of code you shared didn't work for me! I'll just state that in the documentation for now, while we manage some ways to make this work :)

@davepagurek
Copy link
Contributor

Unfortunately, the snippet of code you shared didn't work for me! I'll just state that in the documentation for now, while we manage some ways to make this work :)

Sounds good! Also, that's interesting that it doesn't work, I guess there's more to it than I thought 🤔 Definitely out of scope for this PR then! If you feel like sending the sketch it doesn't work for, I can poke around in it and see if I can debug some more!

@endurance21
Copy link
Contributor

endurance21 commented Sep 5, 2022

@jesi-rgb great work!
I have reviewed the PR and tested it locally, and happy to say that the animation to gif feature is working just fine and is in the stage to be made available to the public and thus merging now.

Disclaimer :
This PR contains the one basic discussion regarding the "babel parsing the es6-es5 code and the role of uglifyJs ", which is still a matter of discussion within the group, however for now the solution provided by this pr seems reasonable to that discussion and if a better solution is discovered in near future, it shall definitely be incorporated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature for saving gif files
4 participants