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

Gl puppeteer #1007

Merged
merged 8 commits into from
Apr 14, 2019
Merged

Gl puppeteer #1007

merged 8 commits into from
Apr 14, 2019

Conversation

tech4GT
Copy link
Member

@tech4GT tech4GT commented Apr 11, 2019

@jywarren Now running gl using puppeteer!! 🎉 🎉 🎉
Okay so I am doing it using the localhost demo right now but porting this to image-sequencer.min.js should be easy enough!
Also, some code cleanup is required but this works. :D
cc @Divy123

Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
@tech4GT
Copy link
Member Author

tech4GT commented Apr 11, 2019

Well, figuring out how to run our sequencer.run(callback) method inside of puppeteer's evaluate method was the tricky part, since that function needs to return the value . Currently I have wrapped the whole thing up in a promise, but if someone at @publiclab/is-reviewers has a better solution, I would love to hear it! 😄

@tech4GT tech4GT requested a review from jywarren April 11, 2019 17:55
@jywarren
Copy link
Member

jywarren commented Apr 11, 2019 via email

@tech4GT
Copy link
Member Author

tech4GT commented Apr 11, 2019

Well I ran it on the Mario image coz that's all I had but this absolutely looks like the desired result.
image_0
image_1

@tech4GT
Copy link
Member Author

tech4GT commented Apr 11, 2019

Oh, got the grid image!
image_0
image_1

@jywarren
Copy link
Member

jywarren commented Apr 11, 2019 via email

@tech4GT
Copy link
Member Author

tech4GT commented Apr 11, 2019

I think we can generalise this. Maybe have this as a part of pixelmanipulation api itself. So any module can run in headless context with a flag in options.

@vibhorgupta-gh
Copy link

vibhorgupta-gh commented Apr 12, 2019

@tech4GT This is awesome! The promise approach is neat, I had precisely that in mind when I was thinking of solutions to this.
Perhaps this could be generalised in a separate module? Pixel manipulation works very specifically on the pixel level, maybe the headless code could go in a separate js module inside the _nomodule folder which can be called from all the other modules if not running in a browser environment OR if a flag is passed.
@jywarren the webgl-distort module looks like a nice follow-up to this, thoughts?

@jywarren
Copy link
Member

jywarren commented Apr 12, 2019 via email

@tech4GT
Copy link
Member Author

tech4GT commented Apr 12, 2019

Okay so this makes sense. I'll make this code into an api which the module can call if it is not supported on node.

if(!options.inBrowser){
this.output = require('path-to-api').runInBrowserContext(input);
callback();
}
else{
/* Actual module code here */
}

@jywarren
Copy link
Member

jywarren commented Apr 12, 2019 via email

@tech4GT
Copy link
Member Author

tech4GT commented Apr 12, 2019

@jywarren I also need the dist Image sequencer script since I do not want to rely on sequencer.publiclab.org
Is it hosted somewhere?

@jywarren
Copy link
Member

jywarren commented Apr 12, 2019 via email

@tech4GT
Copy link
Member Author

tech4GT commented Apr 12, 2019

Uh, I am not sure. I want to get the latest dist files. How do we get them in the demo anyway?

@tech4GT
Copy link
Member Author

tech4GT commented Apr 12, 2019

I mean the demo must be consuming them from somewhere?

@tech4GT
Copy link
Member Author

tech4GT commented Apr 12, 2019

Also I think this might be a good time to move over to webpack, as the modules get more complex this is going to get difficult to manage with browserify. Like right now the tests are failing since grunt-browserify is trying to browserify puppeteer. I was looking at the documentation but there is no way we can ignore a require in the latest version. I'll try to figure out a workaround here.

@jywarren
Copy link
Member

jywarren commented Apr 12, 2019 via email

Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
@tech4GT
Copy link
Member Author

tech4GT commented Apr 13, 2019

@jywarren Got this! So a couple of things here
I figured rather than consuming the file over the network, we can just use the stable branch as a dependency in the is-app and consume the dist file locally. Which is what I have done.
Also I have mode the new api available from nomodule so please take a look at that.
One Issue I ran into was that puppeteer modules cannot be tested with tape so currently ignoring it in benchmarks until we can find a better way to benchmark these modules.

@tech4GT
Copy link
Member Author

tech4GT commented Apr 13, 2019

So we are basically done here, all that is left now is to serialize the options object and use it while running in browser context. 😄

Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
@tech4GT
Copy link
Member Author

tech4GT commented Apr 14, 2019

Okay, this is ready to merge from my side @jywarren 🎉

Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
@tech4GT
Copy link
Member Author

tech4GT commented Apr 14, 2019

Final api looks like:

var step = this;

    if (!options.inBrowser) {
      require('../_nomodule/gl-context')(input, callback, step, options);
    }
    else {
     /* browser specific code */
    }

Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

This is AWESOME @tech4GT !!! A few small comments below. 🎉

src/modules/_nomodule/gl-context.js Outdated Show resolved Hide resolved
test/core/modules/benchmark.js Outdated Show resolved Hide resolved
@harshkhandeparkar
Copy link
Member

@tech4GT how heavy is this on the RAM?

@tech4GT
Copy link
Member Author

tech4GT commented Apr 14, 2019

Hmm, we are running a full chrome tab in headless mode, so I would say about 250mb.

@harshkhandeparkar
Copy link
Member

That's not too much. But does running a headless browser hinder performance?

@tech4GT
Copy link
Member Author

tech4GT commented Apr 14, 2019

I mean it's going to be less effective than running natively in node obviously.

Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
Signed-off-by: tech4GT <varun.gupta1798@gmail.com>
@tech4GT
Copy link
Member Author

tech4GT commented Apr 14, 2019

@jywarren Ready to merge?

@jywarren jywarren merged commit c3e8c3f into publiclab:main Apr 14, 2019
@jywarren
Copy link
Member

Fantastic!!!

Let's do webgl-distort next! Fantastic work and thanks everyone for input!

@vibhorgupta-gh
Copy link

@jywarren I would love to work on webgl-distort!

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

Successfully merging this pull request may close these issues.

4 participants