Feature: Implemented resetShader() #3467

sanketsingh24 · 2019-01-21T13:05:32Z

Implemented resetShader() method, on the basis of its namesake in processing.

Features:

Resets the previously defined shader to default shader.
Is chainable.

Work Done:

Implemented the method in material.js.
Added Documentation.
Added a manual test which is based on toon shading. (Influenced by toonShader example in processing and in p5.js).
Need suggestions for implementing a small example in documentation.

Spongman · 2019-01-21T19:00:37Z

i wonder if it might be more in line with existing framework naming to call this function noShader() ? (simimlar to noFill(), noStroke(), etc... ?)

sanketsingh24 · 2019-01-21T19:48:33Z

@Spongman thanks for the nice suggestion, main influence for the name was certainly processing, and I feel resetShader() literally resets the shader (like resetMatrix()), what's your influence for noShader() (in a literal way)?

sanketsingh24 · 2019-01-23T09:08:13Z

Also need suggestions for whether the sketch should be merged, and if yes, where it should be present.

sanketsingh24 · 2019-01-23T17:20:40Z

@Spongman @limzykenneth @AdilRabbani @lmccart would you please review my implementation if you have some time to spare?

limzykenneth · 2019-01-23T17:54:19Z

I'm not familiar with WebGL's implementation so I don't usually look at issues relating to webgl mode.

sanketsingh24 · 2019-01-27T14:40:42Z

I think it would be helpful and constructive for me if I get to know why this PR is not being reviewed, it will surely help me to rectify my mistakes in the future. 😄

AdilRabbani · 2019-01-30T11:20:59Z

Hey @sanketsingh24 I am busy with some other work these days but I'll try to have a look at this by the weekend. In the mean time, @kjhollen and @stalgiag might be able to give you additional feedback. I believe they can surely review this, better than me! 😄
PS. if a PR doesn't get reviewed in a short time, that doesn't necessarily mean, something is wrong with the PR. Sometimes, people are busy with their own schedule and they won't be able to give feedback at once but whenever they're free they'll surely be willing to help with any contributions! 😇

stalgiag · 2019-01-31T03:09:10Z

Thanks @AdilRabbani! Very well said.

@sanketsingh24 I will start reviewing WebGL related pull requests next week. If this PR hasn't been reviewed before then I will check it out in order of importance. There is a small backlog of PRs but I'll be putting in significant time looking over things starting next week. Thanks for your contribution!

AdilRabbani · 2019-02-04T21:43:44Z

Hey @sanketsingh24, the test example you added is not able to load styles.css and stats.js file. Is it just me or does it happen for you as well?

Other than that, this looks good to me. I would request @stalgiag to further review this PR, before it can be merged.

sanketsingh24 · 2019-02-05T09:23:56Z

@AdilRabbani Oh while testing, the whole folder was somewhere else, and I didn't update the path before pushing, will do it now. Sorry and thanks for testing and pointing it out 😄.

stalgiag · 2019-02-08T02:03:47Z

Hi @sanketsingh24 ! I think it is a great idea to add this feature. It may be a little heavily dependent on changes to when shader uniforms are set such as the types of changes proposed by #3076. The reason I bring this up is because I was playing around with your manual test example and I changed the code in draw to:

function draw() 
{
  background(0);
  var dirY = (mouseY / height - 0.5) * 2;
  var dirX = (mouseX / width - 0.5) * 2;
  directionalLight(204, 204, 204, -dirX, -dirY, -1);

  ambientMaterial(0, 255, 255);
  shader(toonShader);
  translate(-150, 0, 0);
  sphere(120);
  
  translate(300, 0, 0);
  resetShader();
  ambientMaterial(0, 255, 255);
  sphere(120);
  
}

As you can see the lighting doesn't update before rendering the first sphere. It is possible that this should be considered unrelated to this particular PR but thought worth initiating a discussion about what the user should expect to happen when resetShader() is called in draw.

sanketsingh24 · 2019-02-12T13:49:07Z

@stalgiag I think I need to dig a little deeper on this stuff, also on what user should expect. As my exams are coming, I need some time off, but surely I will do in this fortnight.

sanketsingh24 · 2019-02-26T16:57:59Z

@stalgiag, I have a doubt, should using translate() also affects the directionallight() It seems so here-

Code-

function draw() 
{
  background(0);
  var dirY = (mouseY / height - 0.5) * 2;
  var dirX = (mouseX / width - 0.5) * 2;
  directionalLight(204, 204, 204, -dirX, -dirY, -1);

  ambientMaterial(0, 255, 255);
  shader(toonShader);
  translate(-150, 0, 0);
  sphere(120);
  
  translate(300, 0, 0);
  ambientMaterial(0, 255, 255);
  sphere(120);
}

Mouse pointer was in between the two spheres, couldn't capture it in the screenshot.

stalgiag · 2019-02-26T19:07:58Z

Hi @sanketsingh24 it does look a little unexpected in your screenshot but I believe everything is still working as expected. I cross-tested in Processing and received different results. It is important to note that this example uses per-vertex lighting and also that the lighting direction is being modified by the mouse. This is different than the modifying the lighting origin point. With directional lights it is as thought there are rays of light moving from -infinite to infinite in the direction specified, as with sunlight.

sanketsingh24 · 2019-02-26T20:24:28Z

Oh alright I got that

sanketsingh24 · 2019-02-28T13:44:42Z

Ok I think I have an explaination for this,

function draw() 
{
  background(0);
  var dirY = (mouseY / height - 0.5) * 2;
  var dirX = (mouseX / width - 0.5) * 2;
  directionalLight(204, 204, 204, -dirX, -dirY, -1);

  ambientMaterial(0, 255, 255);
  shader(toonShader);
  translate(-150, 0, 0);
  sphere(120);
  
  translate(300, 0, 0);
  resetShader();
  ambientMaterial(0, 255, 255);
  sphere(120); 
}

Because curFillShader contains the light shader described by directionalLight(), resetShader() resets it and removes directional lighting. Also, if I add this snippet to the upper code-

function mouseClicked() {
  shader(toonShader); 
}

Just for the first iteration of draw(), or specifically, when redraw() is called after mousepressed(), the sketch shows the correct behaviour before the toonShader is lost completely by resetting it.

Also, if shader() is not used in setup() and just used in draw(), the toonshader is overwritten by ambientmaterial, here-

var toonShader;
var shaderEnabled=true;

 function preload() {
  toonShader = loadShader('vert.glsl', 'frag.glsl');
}

 function setup() {
  createCanvas(windowWidth, windowHeight, WEBGL);
  noStroke();
 // shader(toonShader);
  toonShader.setUniform('fraction', 1.0);
}

function draw() 
{
  background(0);
  var dirY = (mouseY / height - 0.5) * 2;
  var dirX = (mouseX / width - 0.5) * 2;
  directionalLight(204, 204, 204, -dirX, -dirY, -1);

  ambientMaterial(0, 255, 255);
  shader(toonShader);
  translate(-150, 0, 0);
  sphere(120);
  
  translate(300, 0, 0);
  sphere(120);
}

Here, toonShader has no effect, only ambientmaterial is seen.

I think that shader() doesn't work as expected when used in draw(), here ambientMaterial() overwrites it, but when shader() is in setup(), all is good.

For your example, using resetshader() in draw(), after one iteration of draw(), toonshader(defined in setup() ) would be completely lost, and for the next iteration, the toonshader won't be applied as ambientmaterial() will overwrite, and therefore, desired output is not achieved.

I don't know how to fix this right now, maybe can be fixed in a new issue.

@stalgiag @outofambit @Spongman @AdilRabbani can you please check and review if I have caught the correct error?

Spongman · 2019-02-28T19:32:25Z

this is going to conflict quite heavily with #3535 if that gets merged.

stalgiag · 2019-02-28T19:35:19Z

Yes apologies for not making that more clear @sanketsingh24 . As @Spongman says #3535 is an update to the #3076 that I mentioned earlier, and is very likely to be merged soon. Your work with resetShader will be heavily dependent on the changes made by that PR.

Spongman · 2019-02-28T20:55:18Z

oops, sorry, i missed that you had already mentioned that.

this should be reasonably simple to implement wrt. #3076. just setting user*Shader to null, should suffice.

stalgiag · 2019-02-28T23:48:09Z

#3535 was just merged. @Spongman's idea for how to reset the shader with those changes in mind is a good one. Thanks for the patience @sanketsingh24 !

sanketsingh24 · 2019-03-01T03:09:45Z

Ok alright I would make the changes and test once, really nice changes :)

sanketsingh24 · 2019-03-01T13:04:22Z

Ok, the new changes were easy to do, and solves this issue, but still, please see these cases-

var toonShader;
var shaderEnabled=true;

 function preload() {
  toonShader = loadShader('vert.glsl', 'frag.glsl');
}

 function setup() {
  createCanvas(windowWidth, windowHeight, WEBGL);
  noStroke();
  //shader(toonShader);
  toonShader.setUniform('fraction', 1.0);
}

function draw() 
{
  background(0);
  var dirY = (mouseY / height - 0.5) * 2;
  var dirX = (mouseX / width - 0.5) * 2;
  directionalLight(204, 204, 204, -dirX, -dirY, -1);

  shader(toonShader);
  ambientMaterial(0, 255, 255);
  translate(-150, 0, 0);
  sphere(120);
  
  translate(300, 0, 0);
  //resetShader();
  ambientMaterial(0, 255, 255);
  sphere(120); 
}

ambientMaterial() still overshadows shader() when the latter is only used in draw(), don't know if this is an error but for this code -

var toonShader;
var shaderEnabled=true;

 function preload() {
  toonShader = loadShader('vert.glsl', 'frag.glsl');
}

 function setup() {
  createCanvas(windowWidth, windowHeight, WEBGL);
  noStroke();
  shader(toonShader);
  toonShader.setUniform('fraction', 1.0);
}

function draw() 
{
  background(0);
  var dirY = (mouseY / height - 0.5) * 2;
  var dirX = (mouseX / width - 0.5) * 2;
  directionalLight(204, 204, 204, -dirX, -dirY, -1);

  //shader(toonShader);
  ambientMaterial(0, 255, 255);
  translate(-150, 0, 0);
  sphere(120);
  
  translate(300, 0, 0);
  //resetShader();
  ambientMaterial(0, 255, 255);
  sphere(120); 
}

the output is as expected here.

sanketsingh24 · 2019-03-06T17:53:48Z

Ok, the upper issue was resolved in #3574, so @stalgiag please review.

Spongman · 2019-03-06T18:06:50Z

shouldn't this also be resetting userStrokeShader ?

sanketsingh24 · 2019-03-07T11:10:17Z

Yes @Spongman I think _tex should also be null?

Spongman · 2019-03-07T20:28:49Z

i'm not sure about _tex, isn't this just supposed to revert back to the build-in shader? resetting the texture seems to be orthogonal to this.

stalgiag · 2019-03-07T22:59:45Z

I agree with @Spongman. We can keep the shaders and textures as separate ideas when rendering.

sanketsingh24 · 2019-03-08T08:33:13Z

Also, regarding the unit tests, shall I place them at /test/unit/webgl/p5.shader.js?

stalgiag · 2019-03-08T23:06:40Z

@sanketsingh24 that seems good. You can just set the custom shader with one of the private _get...Shader() renderer functions as is done in this test and then test that resetShader works afterwards.

sanketsingh24 · 2019-03-12T17:29:17Z

@stalgiag I added a unit test, and also resolved a merge conflict, please review.

stalgiag · 2019-03-18T18:42:43Z

@sanketsingh24 I just wanted to add a note to say that I haven't forgotten about this. I am focusing on the tickets for the 0.8.0 release right now but I definitely have not forgotten about this and will look it over one last time as soon as I get a chance.

stalgiag

This works well @sanketsingh24 ! Thanks for handling all of the requested changes while the GL renderer was undergoing a number of changes. This is a great utility for people using custom shaders.

Implemented resetShader() in material.js

b33e044

outofambit added the Area:WebGL label Feb 3, 2019

Updated path for resetshader example

2939ab2

Merge remote-tracking branch 'upstream/master' into feature-resetshader

d19827c

sanketsingh24 added 2 commits March 1, 2019 19:12

Incorporated changes from processing#3535

531d598

Merge remote-tracking branch 'upstream/master' into feature-resetshader

6cd6bb3

sanketsingh24 mentioned this pull request Mar 6, 2019

should setUnform work on a shader before shader() is called? #3574

Closed

3 tasks

Merge remote-tracking branch 'upstream/master' into feature-resetshader

b310ba2

added strokeshader to resetshader

a046093

sanketsingh24 added 2 commits March 12, 2019 16:48

added unit tests

364034d

resolved merge conflict

9a8c3f9

stalgiag approved these changes Mar 28, 2019

View reviewed changes

stalgiag merged commit 00999a3 into processing:master Mar 28, 2019

sanketsingh24 deleted the feature-resetshader branch March 29, 2019 04:40

Uh oh!

Feature: Implemented resetShader() #3467

Feature: Implemented resetShader() #3467

Uh oh!

Conversation

sanketsingh24 commented Jan 21, 2019

Uh oh!

Spongman commented Jan 21, 2019

Uh oh!

sanketsingh24 commented Jan 21, 2019

Uh oh!

sanketsingh24 commented Jan 23, 2019

Uh oh!

sanketsingh24 commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

limzykenneth commented Jan 23, 2019

Uh oh!

sanketsingh24 commented Jan 27, 2019

Uh oh!

AdilRabbani commented Jan 30, 2019

Uh oh!

stalgiag commented Jan 31, 2019

Uh oh!

AdilRabbani commented Feb 4, 2019

Uh oh!

sanketsingh24 commented Feb 5, 2019

Uh oh!

stalgiag commented Feb 8, 2019

Uh oh!

sanketsingh24 commented Feb 12, 2019

Uh oh!

sanketsingh24 commented Feb 26, 2019

Uh oh!

stalgiag commented Feb 26, 2019

Uh oh!

sanketsingh24 commented Feb 26, 2019

Uh oh!

sanketsingh24 commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Spongman commented Feb 28, 2019

Uh oh!

stalgiag commented Feb 28, 2019

Uh oh!

Spongman commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stalgiag commented Feb 28, 2019

Uh oh!

sanketsingh24 commented Mar 1, 2019

Uh oh!

sanketsingh24 commented Mar 1, 2019

Uh oh!

sanketsingh24 commented Mar 6, 2019

Uh oh!

Spongman commented Mar 6, 2019

Uh oh!

sanketsingh24 commented Mar 7, 2019

Uh oh!

Spongman commented Mar 7, 2019

Uh oh!

stalgiag commented Mar 7, 2019

Uh oh!

sanketsingh24 commented Mar 8, 2019

Uh oh!

stalgiag commented Mar 8, 2019

Uh oh!

sanketsingh24 commented Mar 12, 2019

Uh oh!

stalgiag commented Mar 18, 2019

Uh oh!

stalgiag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sanketsingh24 commented Jan 23, 2019 •

edited

Loading

sanketsingh24 commented Feb 28, 2019 •

edited

Loading

Spongman commented Feb 28, 2019 •

edited

Loading