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

Added FisheyeGl Module #61

Merged
merged 7 commits into from
Jul 29, 2017
Merged

Added FisheyeGl Module #61

merged 7 commits into from
Jul 29, 2017

Conversation

ccpandhare
Copy link
Collaborator

I have added the FisheyeGl module and have added documentation for the same.
Can you help me out with the documentation?
I am not able to understand what a, b, Fx, Fy, x, y mean.

@jywarren
Copy link
Member

They're various lens characteristics - see

https://jywarren.github.io/fisheyegl/example/

This is a good example of where we need to heavily use the options parameter, and the web UI will be a bit more complex.

@jywarren
Copy link
Member

Ah but let's include this in via npm, and we may need to then publish fisheyegl on there.

@jywarren
Copy link
Member

OK - published latest version to npmjs: https://www.npmjs.com/package/fisheyegl - v0.0.2

@ccpandhare
Copy link
Collaborator Author

Thanks!

But, the file fisheyegl.js I have added in Image Sequencer isn't the exact same as jywarren/fisheyegl:master/dist/fisheyegj.js. I had to modify a few things in it for the following reasons:

Require

The original fisheyegl.js uses the following code:

(function(exports){

  if (typeof module === 'undefined')
    exports = FisheyeGl;
  else
    module.exports = FisheyeGl;

})(typeof exports === 'undefined'? this['FisheyeGl']={}: exports);

Browserify is not able to handle this. This is the Error thrown after browserifying and running:

TypeError : require('./fisheyegl') is not a function.

I was using it like this: require('./fisheyegl')({...options...})

So I simply changed it to :

module.exports = FisheyeGl

because anyways we are going to browserify it, so the if-else clause of the original fisheyegl.js is taken care of.

Shader Files

The original fisheyegl.js requires hardcoded shader file paths and makes XHR requests to get these files.

I feel this would be an inconvenience to the user as we plan to export ImageSequencer as a single file image-sequencer.js.

If we stick to the original fisheyegl.js, then the user not only has to save the shader files separately (even if he wishes to use default shader files) but also has to pass in the path to the shader files every time he uses the fisheye-gl module.

So what I did was, I used CommonJS Require to simply require in the Shader files instead of making an XHR Request. For example, I converted the vertex shader file vertex.glvs to vertex.glvs.js like this :

module.exports = "\
Contents of\n\
The shader file.\n\
\n\
";

So now the shader files are just bundled in image-sequencer.js. The added benefit of this is that no XHR Requests are created, hence eliminating CORS Errors (On a local HTML file) -- I Could not run the original FisheyeGl on my machine until I started an Apache server due to this issue.

A small Bug

Original fisheyegl.js file:

var selector = "#canvas" || options.selector;

Shouldn't that be

var selector = options.selector || "#canvas";

?

So I changed these things... Did I do the right thing? What do you think? What should be done?

@ccpandhare
Copy link
Collaborator Author

Also, the demo for FisheyeGl is now up at https://ccpandhare.github.io/image-sequencer/examples/fisheye

@ccpandhare
Copy link
Collaborator Author

Now fisheyegl is being required in from npm.

@jywarren
Copy link
Member

jywarren commented Jul 29, 2017 via email

@ccpandhare
Copy link
Collaborator Author

Merging!

@ccpandhare ccpandhare merged commit 8f5e9f0 into publiclab:master Jul 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants