-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Make shaders define what they get used for. #7256
Conversation
Great job so far, Garima! The idea is that when you call Currently, methods like
check which uniforms are defined, and if any are defined, the function returns true. This allows us to apply fill shaders when One possible solution would be to remove these methods that check whether uniforms and attributes are defined. Instead, we can directly set the appropriate user shader based on the method the user calls, such as Also, we should modify the function
We could probably return the user-defined fill shader if present, or light shader if no fill is set. I guess this should work for fill shaders since |
src/webgl/p5.Shader.js
Outdated
@@ -1012,6 +1012,10 @@ p5.Shader = class Shader { | |||
return this.uniforms.uStrokeWeight !== undefined; | |||
} | |||
|
|||
isImageShader() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this method because we don't want our imageShader(yourShader)
method to work depending on some uniforms (uImageSampler
for your case). We could probably take this one out.
src/webgl/material.js
Outdated
this._renderer._useNormalMaterial = false; | ||
} | ||
// Always set the shader as a fill shader | ||
this._renderer.userFillShader = s; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
Thanks for the suggestions @perminder-17 ! |
src/webgl/p5.RendererGL.js
Outdated
} | ||
return fill; | ||
if (this._tex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the OR operator here and combine it into a single if statement? Something like:
if(this._enableLighting || this._tex)
return this._getLightShader();
Can this make the code cleaner instead of using two separate if blocks?
Great work @Garima3110 so far. I tested it, and it seems to be working well. But to be double sure, I'll test it again to make sure nothing is breaking. In the meantime, you should focus on completing the documentation for the fill shader to meet the first week's target. The documentation should include: (these are just my thoughts, feel free to add your own explanations if you have any).
Also, talking about the
You can remove the method and update the function here as well: (just you did for fill shaders)
and then we can move to the docs of Let me know if I think we also need to alter some tests created for shaders, will update you for the tests as we completes the docs and code of this project. Thanks again for your work. :) |
Thanks for the follow up @perminder-17 |
I have done all changes in docs and examples for fill shaders, please have a look @perminder-17 |
Just an update, previously i mentioned the wrong block of code by mistake. I just saw when you pushed it you need to remove the isStrokeShader() code. I have edited the above comment now.
|
src/webgl/p5.Shader.js
Outdated
@@ -579,9 +579,6 @@ p5.Shader = class Shader { | |||
const modelViewProjectionMatrix = modelViewMatrix.copy(); | |||
modelViewProjectionMatrix.mult(projectionMatrix); | |||
|
|||
if (this.isStrokeShader()) { | |||
this.setUniform('uPerspective', this._renderer._curCamera.useLinePerspective ? 1 : 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we use uPerspective
uniform in our line.vert
file here:
p5.js/src/webgl/shaders/line.vert
Line 31 in 4fbbaf5
uniform int uPerspective;
So, we could keep them without the if block as well. Otherwise we would not be able to use uPerspective
unfiorm and could throw errors.
src/webgl/material.js
Outdated
* process each pixel on the canvas. This only affects the fill (the inside part of shapes), | ||
* not the strokes (outlines). | ||
* If you want to apply shaders to strokes or images, use the following methods: | ||
* - **[strokeShader()](#/p5/strokeShader)**: Applies a shader to the stroke (outline) of shapes, allowing independent control over the stroke rendering using shaders. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I was thinking to add these references at the last.
Like at first we could have our fill shader explanation.
At the middle we could have our example sketches how it works.
After the sketches we could have the references of strokeShader()
and imageShader()
. Let me know if that sound's okay to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that too makes sense, I'll do that no worries!
As we're making breaking changes in this PR, we’ll end up with two distinct methods for applying shaders: The advantage of renaming it would be creating a clearer, more defined structure: Here's how it looks as of now: Screencast.from.15-09-24.01.13.59.AM.IST.webm |
Thank you for raising this discussion @perminder-17 ! Well, I would also like to add something to your suggestion, I see the potential value in renaming This approach aligns with how we’ve handled similar features, where the core functionality remains intuitively named, while specialized methods are introduced for distinct use cases. |
Looks good @perminder-17 ! I agree with @Garima3110, I don't think With |
I think I also agree with @lukeplowden and @Garima3110. Maybe the way to think about it is not in terms of fills and strokes, but in terms of materials and strokes. I suppose fill is actually just one component of an object's material, in addition to other properties like texture or shininess. Strokes are a separate system which adds something on top of the base material. In that view of everything, the material system is the main one you interact with and includes the most stuff, so treating it like the default in its naming feels right to me. One thing that's not super clean about the model I just described is that images are kind of just a separate system, and regular objects that have a material and stroke can't go through the image system, and likewise, images don't get materials and strokes. I think that's fine, since we want images to always draw an image, so making overriding of image() very separate and explicit probably will help avoid accidental overrides and confusion. So I see image shaders as maybe more of a convenient way for developers to change small bits about image drawing and still take advantage of the fit and fill options of image() rather than a core part of the material/stroke system I guess. |
Thankyou @lukeplowden @davepagurek and @Garima3110 for your thoughts.
We are now planning to create a method that will only affect image calls. In simple terms, we are separating our methods because in many scenarios, we have seen that sometimes fill gets applied to images and sometimes it doesn't, which causes problems and confusion see here: #6327 , #6564 . To avoid accidental shaders being applied and also to avoid confusion, we will create a new method called I’d also like your thoughts on something related to the question, which may seem obvious but I want to confirm. Do you think the |
Okay, @Garima3110 currently, the For example, if you see the code Line 1374 in 5fbde13
we check if For now, it’s worth disabling the fill shader effect for images, as @davepagurek's inclination was also towards not using fill shader for effecting image. For doing that, We can update the function p5.js/src/webgl/p5.RendererGL.js Line 1768 in 5fbde13
by adding an extra condition which could handle isTextureShader() .
Then, for Also, we could add example of by shaderHooks for only |
src/webgl/material.js
Outdated
* The shader will be used for: | ||
* - Strokes only, regardless of whether the uniform `uStrokeWeight` is present. | ||
* | ||
* @method strokeShader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question @davepagurek, since we need to add docs for strokeShader utilising shaderHooks. I see we have a method with same name strokeShader
in shaderHooks as well. Do you think the method created in shaderHook PR will override this method (because they are with the same name)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've renamed the shader hooks method now to be baseStrokeShader
, but I'm just commenting here to mention that we'll probably need to update the examples for that after this is merged. Currently, the shaders made with it are applied via shader()
. I think after this change, they will need to be applied with strokeShader()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing! @davepagurek , thanks for the reminder
Thank you for bringing this up! @perminder-17
From my perspective, the motivation behind introducing a separate Considering the issues mentioned (#6327, #6564), I would say that the |
Ahh @perminder-17 you're right, those methods are going to clash, so we'll have to rename something. Maybe the hooks ones should be renamed to |
src/webgl/p5.RendererGL.js
Outdated
} | ||
} else if (!fill /* || !fill.isColorShader()*/) { | ||
return this._getColorShader(); | ||
if(fill && !fill.isTextureShader()){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aah....._getRetainedFillShader()
is for shapes, not images. You need to modify getImmediateFillShader()
, specifically where we are checking
if (this._tex) { ..... }
to properly handle images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change the condition to :
if(fill && !fill.isTextureShader()){ | |
if(fill ){ |
since, isTextureShader()
is not checking for image. it's done in _getImmediateFillShader()
src/webgl/p5.RendererGL.js
Outdated
_getRetainedImageShader() { | ||
const imageShader = this.userImageShader; | ||
if (imageShader && imageShader.isTextureShader()) { | ||
return this._getLightShader(); // Return light shader if the shader is a texture shader | ||
} | ||
return imageShader || this._getColorShader(); // Default to color shader if no image shader is defined | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for these many conditions.
Just you have to do is:
If a texture is present, it then checks if the imageShader
is either missing or not set up for handling textures. If that’s the case, use light shader. Otherwise, it simply returns the existing imageShader
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you should rename it to getImmidiateImageShader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p5.js/src/webgl/p5.RendererGL.Immediate.js
Line 510 in e624307
p5.RendererGL.prototype._drawImmediateFill = function(count = 1) { |
Also, we need to create something like _drawImmediateFill
for images where we could use _getImmidiateImageShader
in the function to make it working.
How about |
That also works for me. I guess the stroke one is the only naming conflict because the shader used internally for images is the material shader, and we're just using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to everyone, @Garima3110 and @davepagurek . Things have started coming at a good form. Will request @Garima3110 to add visual tests for all the shaders and could remove/update the previous test where it used the function isTextureShader()
or isLightShader()
which we currently don't have, that's the main reason of our tests to fail.
src/webgl/p5.RendererGL.js
Outdated
_getImmediateImageShader() { | ||
const imageShader = this.userImageShader; | ||
if(imageShader) { | ||
return imageShader; | ||
} | ||
return this._getImmediateModeShader() ; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could probably make just one method for all three, _getImmediateImageShader()
, _getImmediateFillShader()
and _getRetainedFillShader()
and could name as _getFillShader()
something like that.
The only problem could be with the overriding of imageShader()
with the beginShape/endShape part, so we could probably add a logic here:
p5.js/src/webgl/3d_primitives.js
Line 3437 in 5810754
this.beginShape(); |
something like:
this._drawingImage = true;
this.beginShape();
// ...
this.endShape();
this._drawingImage = false;
and for the _getFillShader()
I am talking about, it could look something like:
getFillShader() {
if (this._drawingImage) {
return this.userImageShader;
} else if (this.userFillShader) {
return this.userFillShader;
} else if ...
Really thanks to @davepagurek for his suggestions on this.:)
src/webgl/p5.RendererGL.js
Outdated
point: this.GL.createBuffer(), | ||
image: [ | ||
new p5.RenderBuffer(3, 'imageVertices', 'imageVertexBuffer', 'aPosition', this, this._vToNArray), | ||
new p5.RenderBuffer(2, 'imageUVs', 'imageUVBuffer', 'aTexCoord', this, this._flatten) | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be reverted, I don't think we need this.
src/webgl/p5.RendererGL.Immediate.js
Outdated
* and drawing the image geometry. | ||
* @private | ||
*/ | ||
p5.RendererGL.prototype._drawImmediateImage = function(count = 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably remove the whole function. Since the logic with merging it, this._drawingImage
will take care of that.
src/webgl/p5.RendererGL.Immediate.js
Outdated
let fillShader = this._getImmediateFillShader(); | ||
let imgShader = this._getImmediateImageShader(); | ||
|
||
if (this.textureMode === constants.IMAGE && imgShader) { | ||
usimageShader(imgShader); | ||
this._drawImmediateImage(count); | ||
} | ||
else { | ||
shader(fillShader); | ||
this._drawImmediateFill(count); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be reverted.
src/webgl/p5.RendererGL.Immediate.js
Outdated
if(imageShader){ | ||
// We have an image shader, normalize UV coordinates based on texture dimensions | ||
if (this._tex !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need any extra checks, Could be reverted.
Thanks for all the suggestions to you @perminder-17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great so far, we are just on the last bits.
src/webgl/p5.RendererGL.js
Outdated
else if (this._enableLighting || this._tex) { | ||
return this._getLightShader(); | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably remove this else block, it's obvious that it could return this._getColorShader();
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could just write return this._getColorShader();
without the else block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok @perminder-17 I'll soon push these suggested changes.
Thankyou!
test/unit/visual/cases/webgl.js
Outdated
` | ||
); | ||
p5.strokeShader(strokeshader); | ||
p5.noFill(); | ||
p5.strokeWeight(2); | ||
p5.rect(-20, -20, 40, 40); | ||
screenshot(); | ||
resolve(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I could see, this code produces a white canvas, so it's not good to test strokeShader this way. Probably could do with the baseShaderHooks, or maybe the examples for reference you added for strokeShaders.
test/unit/visual/cases/webgl.js
Outdated
` | ||
precision mediump float; | ||
void main() { | ||
gl_FragColor = vec4(gl_FragCoord.x / 100.0, 0.5, gl_FragCoord.y / 100.0, 1.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably could just be a vec4(1.0,0.0,0.0,1.0)
red color. Actually not sure what this will produce, so could just add a simple visual test for it. And could turn on lights()
, texture(img)
and all won't effect any thing in the result.
test/unit/visual/cases/webgl.js
Outdated
); | ||
p5.imageShader(imgShader); | ||
p5.texture(img); | ||
p5.noStroke(); | ||
p5.rect(-25, -25, 50, 50); | ||
screenshot(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imageShader() applies to image()
function and not with texture(img) followed with rect. So, it could be a better check to just see weather imageShader() is not applying to these. But we could also add a test for image(img,0,0) to test weather imageShader probably applies to image or not.
src/webgl/material.js
Outdated
* } | ||
* `; | ||
* | ||
* function setup() { | ||
* createCanvas(100, 100, WEBGL); | ||
* fillShader = createShader(vertSrc, fragSrc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, we could include a shaderHooks example for fillShader, by removing one example.
Just a question: Suppose we render something, like a rectangle with a red color, and then apply the But here's where it gets interesting: If we use p5.js/src/webgl/p5.RendererGL.js Line 1057 in c377b53
image() function to render the contents of the framebuffer (FBO) back onto the main canvas or target . p5.js/src/webgl/p5.RendererGL.js Line 1170 in c377b53
panorama(img) , likely because it uses filter shaders. So, I am thinking if we could disable imageShader() when applying filters?
Or maybe we could add some extra conditions to _getFillShader() by adding a boolean variable maybe (this._drawingFilter) at filter block, where when image() is called with filters, how @davepagurek, can you give a thumbs up if you agree? |
I completely agree with the suggestion made above by @perminder-17 . The key issue lies in how So according to me also, introducing a condition to disable |
Agreed, filters shouldn't be affected here. If you're curious, I was talking a bit here #7270 (comment) about refactoring how some of these things work so that there are "safe" functions we can call internally without user code leaking in. For now I think we just have to know to disable user shaders here. |
src/webgl/p5.RendererGL.js
Outdated
return this._getNormalShader(); | ||
_getFillShader() { | ||
// If a filter like BLUR is applied, disable imageShader | ||
if (this._drawingFilter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested it? I think you need to include this._drawingFilter
= false
initally and then in filter(...args){
you need to make it true so it could catch the filter() calls.
@@ -133,8 +133,10 @@ p5.RendererGL.prototype.drawBuffers = function(gId) { | |||
this.retainedMode.geometry[gId].vertexCount > 0 | |||
) { | |||
this._useVertexColor = (geometry.model.vertexColors.length > 0); | |||
const fillShader = this._getRetainedFillShader(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add an extra condition here,
let fillShader;
if (this._drawingFilter && this. userFillShader){
fillShader = this.userFillShader;
} else {
fillShader = this._getFillShader();
}
src/webgl/p5.RendererGL.js
Outdated
} | ||
// If drawing an image, use the image shader or light shader | ||
else if (this._drawingImage) { | ||
if(this.userImageShader){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like, instead of adding an extra condition can we just do something like
if(this.userImageShader && !this._drawingFIlter){
// your code
} else{
return this._getLightShader();
}
src/webgl/p5.RendererGL.js
Outdated
@@ -575,6 +575,7 @@ p5.RendererGL = class RendererGL extends Renderer { | |||
this.userFillShader = undefined; | |||
this.userStrokeShader = undefined; | |||
this.userPointShader = undefined; | |||
this.userImageShader = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
above this, where every boolean variables are defined and set to false, you need to set there this._drawingFilter = false;
and at the function
filter(...args) {
// where the image call is present, above that set the
this._drawingFilter = true;
this._pInst.image(fbo, ....)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright @perminder-17 working on these changes.!
@perminder-17 I have made all the changes, please have a look and let me know if any other thing needs to be updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thankyou so much @Garima3110 for working through everything. I request @davepagurek to give a round of review.
here's the build : https://editor.p5js.org/aman12345/sketches/fzhPwc8Hr (from the main branch)
This is a work in progress addressing the issue ##6759
PR Checklist
npm run lint
passes