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

Update & optimize #3

Merged
merged 9 commits into from
Apr 1, 2016
Merged

Update & optimize #3

merged 9 commits into from
Apr 1, 2016

Conversation

dy
Copy link
Contributor

@dy dy commented Mar 31, 2016

Hello @mattdesl!

This is the lightweight version of #2.

Changes:

  • Update tests to use [email protected]
  • Process n×m dim canvas more carefuly
  • Take fragment shader source as the only argument var draw = create(glslify('./shader.frag'), opts?). It is compatible with old API.
  • Optimize performance of draw - it is 2-3 times faster in some cases now.
  • Process float values by default, if possible, and fallback to Uint8 if not.

It is a major change, because float processing does not clamp values to 0..1 range now. In every other aspect it is minor change.

Looking forward for review.

@dy dy mentioned this pull request Mar 31, 2016
else {
var pixels = new Uint8Array(w * h * 4)
gl.readPixels(0, 0, w, h, gl.RGBA, gl.UNSIGNED_BYTE, pixels)
pixels = new Float32Array(pixels).map(function(p) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Float32Array#map doesn't work in Safari and probably some other older browsers. 😄

@mattdesl
Copy link
Contributor

mattdesl commented Apr 1, 2016

Nice work!

I made a couple comments. The only weird thing for me is the API, which could be used in any of these ways:

create(shader)
create({ shader: shader })
create(shader, { shader: shader })
create(null, { shader: shader })

I think it's better to keep a smaller API surface area even if it introduces an extra few keystrokes. This way any code using this library is forced to be consistent:

create({ shader: shader })

😄

module.exports = function (shader, opt) {
if (!opt) {
//just options
if (typeof shader === 'object' && !shader.fragShader) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small thing here – I think it's best to reduce the peer dependency on fragShader and gl-shader and just assume a more generic interface that is basically { gl, bind() } which is what we had before. 😄 But I think this part will go away if we keep the options simple as they were before.

@dy
Copy link
Contributor Author

dy commented Apr 1, 2016

@mattdesl ok, fixed all comments.
I agree with the API consistency point, but I would stick to <process>(<what>, <how>?) scheme, which is create(shader, opts?) in this case. It sets UX to the first place and quite easy and intuitive to remember, user doesnt need to remind the options key for the shader: is it a shader, fragShader, etc. It’s not a big deal here though.

@mattdesl
Copy link
Contributor

mattdesl commented Apr 1, 2016

Yeah I agree create(shader, opt?) is better since shader is required. But we can remove it as an option from the opt param.

@mattdesl
Copy link
Contributor

mattdesl commented Apr 1, 2016

I can merge and fix the API to the new style though, and make a breaking change. Thanks for all this!

@mattdesl mattdesl merged commit 5ba6869 into Experience-Monks:master Apr 1, 2016
@dy
Copy link
Contributor Author

dy commented Apr 1, 2016

Ok, great! I’ll then align nogl-shader-output then to comply with the new API.

@mattdesl
Copy link
Contributor

mattdesl commented Apr 1, 2016

Merged & cleaned up code / docs in 2.x. Thanks!

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.

2 participants