Skip to content

material.js shader() example / remove Mandel #3722

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

Closed
wants to merge 2 commits into from
Closed

material.js shader() example / remove Mandel #3722

wants to merge 2 commits into from

Conversation

hsab
Copy link
Member

@hsab hsab commented May 2, 2019

addresses #1954

resolves #3720

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

@stalgiag let me know what you think 👍

@stalgiag
Copy link
Contributor

stalgiag commented May 3, 2019

Cool! I think I had a bit of a misunderstanding about what you were going to do but this looks good. There is something about this feels like it makes more sense as a setUniform example since the reused shader makes the second shader object a bit unnecessary. Something like:

let shade;
let showRedGreen = false;

function preload() {
  shade = loadShader('shader.vert', 'shader-gradient.frag');
}

function setup() {
  createCanvas(100, 100, WEBGL);
  noStroke();
}

function draw() {
  if (showRedGreen) {
    shade.setUniform('colorCenter', [1.0, 0.0, 0.0]);
    shade.setUniform('colorBackground', [0.0, 1.0, 0.0]);
    shade.setUniform('offset', [0, sin(millis() / 2000) + 1]);
  } else {
    shade.setUniform('colorCenter', [1.0, 0.5, 0.0]);
    shade.setUniform('colorBackground', [0.226, 0.0, 0.615]);
    shade.setUniform('offset', [sin(millis() / 2000), 1]);
  }
  shader(shade);
  quad(-1, -1, 1, -1, 1, 1, -1, 1);
}

function mouseClicked() {
  showRedGreen = !showRedGreen;
}

would explain setUniform well. But what you have here already also works as a shader example. There is also the opportunity to do both and to show separate approaches. What do you think?

@hsab
Copy link
Member Author

hsab commented May 3, 2019

@stalgiag What you have makes perfect sense for setUniform. However it seems to not fully translate the function of shader(). Perhaps Mandel was more illuminating in that regard as the user was tangibly interacting with two distinct shaders rather than two instances.

I highly agree with your final note. I think keeping this example PR as is for shader() and also updating the setUniform to what you have illustrated is a wholesome solution that underlines different approaches.

Perhaps we can point the reader to check the other reference example to see the alternative? Do you think this is appropriate for the docs examples? If so should I just add a comment in the example code:

// Click within the image to toggle
// the shader used by the quad shape
// visit https://p5js.org/reference/#/p5/setUniform
// for a different approach, utilizing setUniform() function
// Click within the image to toggle
// the uniform variables used by the shader
// visit https://p5js.org/reference/#/p5/shader
// for a different approach, utilizing shader() function

Edit: also should I close this PR and submit a new one with setUniform? Or would it be better to do two PRs. I guess the latter?

Thanks again @stalgiag

@stalgiag
Copy link
Contributor

stalgiag commented May 3, 2019

That sounds good to me. I don't think there are other instances of this kind of cross-referencing examples thing but I think it works well as an instructional tool.

It is your call about whether to do them as separate PRs. I don't think it is completely necessary since you are editing two related examples.

As an sidenote, this is mostly personal preference but you might find it easier to work with a branch (eg hsab/p5js:shaderMethodExamples). I find it makes keeping my work-in-progress inline with master a bit more manageable.

@hsab
Copy link
Member Author

hsab commented May 3, 2019

Yeah branching is definitely the way to go. I didn't intend to really commit or do PR was mostly checking the codebase and the grunt tasks and decided to jsut give docs:dev a try. Ended up requesting a PR. Will close this one, branch and recommit the changes.

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