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

material.js shader() example #3720

Closed
wants to merge 1 commit into from
Closed

material.js shader() example #3720

wants to merge 1 commit into from

Conversation

hsab
Copy link
Member

@hsab hsab commented May 2, 2019

addresses #1954

extends the loadShader example to allow for toggling between 2 shaders on mouse click/press.

@welcome
Copy link

welcome bot commented May 2, 2019

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

This looks good! I am a little on the fence about the mandelbrot shader example recurring so much in the reference examples. Especially because I think the vertex shader makes it so that quad() begins to look unfamiliar and something like using rect() to replace it is off the table. Which would be fine if it was immediately clear why the vertices are being modified. I also think that the 'p' uniform feels like a magic number. Reference examples should shoot for lowest possible complexity to indicate how a method works. I think the examples/examples (need a better word for these) is a better place for something like the mandelbrot shader.

Anyways! All of that is mostly unrelated to this PR and the added gradient shader is a pleasant step in the right direction. If you felt compelled to replace the mandelbrot shader with something more inline with the gradient shader in this example then that is obviously welcome but I am happy to merge this as is and will do so in a couple of days unless you tell me to hold off :-)


void main() {

vec2 st = vPos.xy + vec2(0,pos);
Copy link
Member Author

@hsab hsab May 2, 2019

Choose a reason for hiding this comment

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

@stalgiag I agree, and yet the reason why I opted for the Mandel shader was exactly the familiarity of its reoccurrence throughout the reference manual. But the fact that it could seem like a blackbox for users/beginners is undeniable.

Perhaps I should change pos to offset. Then the shader could coherently be extended so that:
vec2 st = vPos.xy + vec2(xOffset,yOffset);

color1 and color2 could perhaps be uniforms as well. That way the shader could potentially be reused with a certain level of modularity that is visually distinguishable:

  • moves along X || moves along Y || moves on XY plane || static
  • full-color control

this could allow two instances of this shader to properly reflect the effects of shader() for this particular example/commit:
shader1: blue-orange moving vertically
shader2: red-green moving horizontally

what do yo think?

Copy link
Member Author

@hsab hsab May 2, 2019

Choose a reason for hiding this comment

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

or even better uniform vec2 offset; rather than uniform float xOffset and uniform float yOffset, giving:
vec2 st = vPos.xy + offset.xy;

Copy link
Member Author

@hsab hsab May 2, 2019

Choose a reason for hiding this comment

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

as for the quad() and rect() situation, we could perhaps opt in for something like this. Which as a solution, relies primarily on:
gl_FragCoord.xy / resolution.xy
This allows for eliminating +-1 values for the quad and simply using rect(0,0,width,height);
Yet ambiguity might arise from setUniform('resolution',[width,height]);

On a side note, I tried looking for docs on the webgl vs p2d coordinate differences and failed to find any. Are you aware of such in the codebase/references?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick response! I like the offset with two gradient shader instances idea. I understand the the impulse to repeat the Mandel shader. It might be a good idea for familiarity sake but it also seems like a good time to start moving away from it and maybe filing an issue to simplify the other examples away from the Mandel shader as well.

I'll leave it up to you as to whether to change the vertex shader. I don't have extremely strong feelings and it would be rect(-width/2, -height/2, width, height) which when combined with setting the resolution uniform doesn't provide the largest gains in terms of simplicity.

As for coordinate differences, the getting started with webgl wiki explains this a bit. This decision dates back to the first work on WebGL mode in 2015. There are pros and cons to doing this but I agree that maybe it makes sense to try to find another more prominent place for this in documentation. Not sure if there are other mentions that I am missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the info, although I think it would have been rect(-width/2, -height/2, width*2, height*2), I decided to just opt in for quad().

I'm gonna close this pull req, and open another one with the latest commit.

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.

3 participants