-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Addresses issue #6587 #6665
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
base: main
Are you sure you want to change the base?
Addresses issue #6587 #6665
Conversation
|
@diyaayay looks like the PR is up here! I'm also going to give it a review soon when I get the chance, but it would be great to get your take on it too! |
davepagurek
left a comment
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 for taking this on! I've added some comments here on some bits I think we'll need in order to make this work on WebGL 1 and WebGL 2.
Also once the pieces are all working, it would be great to make a p5 editor sketch using a newly built version of p5 so that we can test it out!
src/webgl/p5.RendererGL.js
Outdated
| ); | ||
| } | ||
| // Create a shader for cubemap conversion | ||
| const cubemapShader = this._pInst.createShader( |
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.
Just above this, we create and cache a shader so that we only ever need one copy. Can we add that caching here too?
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 have applied caching mechanism to possibly all the shaders. I am not sure whether this aligns with what you were suggesting through this @davepagurek .PLease have a look into the changes I've made.
Thanks.!
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 want to wrap all the existing shader sources in a caching function, since those are just the sources for one part of a shader (e.g. just the vertex or fragment source, and often not the full source either, since we need to add a prefix to it.) A caching function would be good for this createShader call (since you're still creating a new shader every time this line is hit, currently.) A few lines up, this.diffusedShader already does caching, but can be refactored to use the same caching system you use here if you're factoring that out into a common pattern.
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.
oh actually I see that you've added that in _getCubemapShader. I think rather than making a new shader here, we can just use const cubemapShader = this._getCubemapShader() instead.
src/webgl/p5.Texture.js
Outdated
| : gl.getExtension('OES_texture_half_float'); | ||
| const supportsHalfFloatLinear = supportsHalfFloat && | ||
| gl.getExtension('OES_texture_half_float_linear'); | ||
| const supportsCubemap = gl.getExtension('OES_texture_cube_map'); |
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 this might only be necessary for WebGL 1, is that something you can double check? If it's built into WebGL2, then this line would be supportsCubemap = webglVersion === constants.WEBGL2 || gl.getExtesion('OES_texture_cube_map')
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.
yes , You are absolutely correct .
WebGL2 supports cubemap textures natively, so we don't need to explicitly check for the OES_texture_cube_map extension in most cases.
Mybad!
I'll just update this one too.
|
|
||
| const vec2 invAtan = vec2(0.1591, 0.3183); | ||
|
|
||
| vec2 SampleSphericalMap(vec3 v) |
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.
Might be worth adding a comment mentioning https://learnopengl.com/PBR/IBL/Diffuse-irradiance as a source so that we can refer back to it in the future
src/webgl/shaders/cubeFragment.glsl
Outdated
| @@ -0,0 +1,23 @@ | |||
| #version 330 core | |||
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.
Right now this shader will only work in WebGL 2 mode. For shaders we want to work in both versions, we add these macros to the start https://github.com/processing/p5.js/blob/main/src/webgl/shaders/webgl2Compatibility.glsl by concatenating it here:
p5.js/src/webgl/p5.RendererGL.js
Lines 83 to 85 in bda63ff
| for (const key in defaultShaders) { | |
| defaultShaders[key] = webgl2CompatibilityShader + defaultShaders[key]; | |
| } |
...and by adding an appropriate prefix for the vertex/fragment shaders like this:
p5.js/src/webgl/p5.RendererGL.js
Lines 1745 to 1748 in bda63ff
| this._defaultLightShader = new p5.Shader( | |
| this, | |
| this._webGL2CompatibilityPrefix('vert', 'highp') + | |
| defaultShaders.lightVert, |
To make this file use it, it would involve taking out the #version (that's added by the prefix we add), and replacing out and in with the macros OUT and IN which get #defined to the appropriate keyword depending on what WebGL version is being used. It also means not explicitly declaring out vec4 FragColor and instead using OUT_COLOR, which we also automatically add a definition for.
src/webgl/shaders/lighting.glsl
Outdated
| uniform bool uUseImageLight; | ||
| // texture for use in calculateImageDiffuse | ||
| uniform sampler2D environmentMapDiffused; | ||
| uniform sampler2D environmentMapDiffusedCubemap; |
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.
Should this be samplerCube?
src/webgl/shaders/lighting.glsl
Outdated
| vec3 worldNormal = normalize(vNormal); | ||
| vec2 newTexCoor = mapTextureToNormal( worldNormal ); | ||
| vec4 texture = TEXTURE( environmentMapDiffused, newTexCoor ); | ||
| vec4 texture = textureCube( environmentMapDiffusedCubemap, newTexCoor ); |
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.
Previously, we were using the TEXTURE, which refers to a macro in https://github.com/processing/p5.js/blob/main/src/webgl/shaders/webgl2Compatibility.glsl to be either texture2D in WebGL1, or texture in WebGL2.
We need to do something similar for cubemap textures. In that file we'd have to #define TEXTURE_CUBE to either be textureCube in WebGL 1, or just texture in WebGL 2, and then use TEXTURE_CUBE(...) here.
|
Thanks a lot @davepagurek for these suggestions ! I'll just look into these and get back to you soon. |
src/webgl/p5.RendererGL.js
Outdated
| defineStrokeJoinEnum('BEVEL', 2); | ||
|
|
||
| const lightingShader = readFileSync( | ||
| const getCachedShader= (shaderKey, shaderSource)=>{ |
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 this would need to be not just a const, but a property on p5.RendererGL, as each instance of a renderer will need its own shaders. We just want the ability to share those shaders within the renderer.
src/webgl/p5.RendererGL.js
Outdated
| ); | ||
| const webgl2CompatibilityShader = readFileSync( | ||
| )); | ||
| const webgl2CompatibilityShader = getCachedShader('webgl2CompatibilityShader',readFileSync( |
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 want to wrap all of these in getCachedShader -- these are not full shaders, just pieces of shader source that will combine with other pieces later to turn into a full shader.
src/webgl/p5.RendererGL.js
Outdated
| const lightingShader = readFileSync( | ||
| const getCachedShader= (shaderKey, shaderSource)=>{ | ||
| if(!shaderCache[shaderKey]){ | ||
| shaderCache[shaderKey]=p5.createShader(shaderSource); |
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.
Shaders also have both a vertex and a fragment shader, not a single shader source, so you'd need to add two parameters to the function and pass both into createShader.
That said, your _getCubemapShader implementation on its own does all the caching we need, so we might not need this in addition to that.
src/webgl/p5.Shader.js
Outdated
| uniform.texture.src._animateGif(this._renderer._pInst); | ||
| } | ||
| break; | ||
| case gl.SAMPLER_CUBE: |
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.
Is this block the same as the one above? If so, we could do:
case gl.SAMPLER_2D:
case gl.SAMPLER_CUBE:
gl.activeTexture(gl.TEXTURE0 + uniform.samplerIndex);
uniform.texture =
data instanceof p5.Texture ? data : this._renderer.getTexture(data);
// ......to handle both cases with the same code
davepagurek
left a comment
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 for working on these changes! I left a few notes on the shader caching. I think if you were to try running this right now on a sketch using imageLight(), you'd run into some runtime errors. For next steps, I'd encourage you to try making a test sketch, running grunt browserify to generate lib/p5.js, then using that in the sketch to test out whether everything still runs. That will help ensure that you catch all the issues that one might run into when using the feature more easily than just looking at the code.
Once that's working, it would also be great to put that up in a p5 editor sketch so people looking at the PR can also test it out!
Yeah sure @davepagurek ..!Thankyou for the suggestions , I'll just make these changes and get back to you soon :-) |
Hey @davepagurek ! I tried running this test sketch after building p5.js Sketch:
|
src/webgl/p5.RendererGL.js
Outdated
| ); | ||
|
|
||
| defaultShaders = { | ||
| immediateVert: renderer.getCachedShader('immediateVert',readFileSync( |
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.
One issue with this current structure is that we're calling getCachedShader in the in _resetContext`, so on load, it will try to make a bunch of shaders.
We maybe want to keep the shader source code in objects outside the constructor as it was before, and then we only call getCachedShader when we need to use the shader. e.g.:
_getCubemapShader() {
return this.getCachedShader(
'defaultCubemapShader',
this._webGL2CompatibilityPrefix('vert', 'mediump') +
defaultShaders.cubemapVertexShader,
this._webGL2CompatibilityPrefix('frag', 'mediump') +
defaultShaders.cubemapFragmentShader
)
}
src/webgl/shaders/cubeFragment.frag
Outdated
| void main() | ||
| { | ||
| vec2 uv = SampleSphericalMap(normalize(localPos)); | ||
| vec3 color = texture(equirectangularMap, uv).rgb; |
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.
You might need to use the TEXTURE macro here so that this will be translated to either texture() or texture2d() based on the environment it's being run in.
src/webgl/shaders/lighting.glsl
Outdated
| vec3 worldNormal = normalize(vNormal); | ||
| vec2 newTexCoor = mapTextureToNormal( worldNormal ); | ||
| vec4 texture = TEXTURE( environmentMapDiffused, newTexCoor ); | ||
| vec4 texture = TEXTURE_CUBE( environmentMapDiffusedCubemap, newTexCoor ); |
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 wonder if the naming of texture is an issue here since in a WebGL 2 environment, TEXTURE_CUBE is also #defined as the builtin function texture()? Does the error go away if you rename this variable?
src/webgl/p5.RendererGL.js
Outdated
| }); | ||
| } | ||
| // Use the diffusedShader for rendering | ||
| newFramebuffer.draw(() => { |
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 not the cause of the error you're seeing, but I think we probably no longer need this newFramebuffer.draw(...) block since we are no longer using this framebuffer as the texture. (I think that means we actually need to call newFramebuffer.free() here to avoid a small memory leak?)
src/webgl/p5.RendererGL.js
Outdated
| this._pInst.rect(0, 0, width, height); | ||
|
|
||
| // Capture the rendered face and store it in the faces array | ||
| faces[i] = this._pInst.get(0, 0, width, height); |
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 probably not the source of the error, but we might need to put this outside of the draw(...) block, and use faces[i] = newFramebuffer.get() instead of this._pInst.get(...).
|
Thankyou @davepagurek for your suggestions, but that fragment shader error unfortunately didn't get resolved. i'll just again have a look at what's going wrong here .. |
src/webgl/shaders/cubeVertex.vert
Outdated
| @@ -0,0 +1,13 @@ | |||
| // Adapted from: https://learnopengl.com/PBR/IBL/Diffuse-irradiance | |||
| layout (location = 0) in vec3 aPos; | |||
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.
This might also need to be changed into IN vec3 aPos in order to also work in WebGL 1 environments
src/webgl/shaders/lighting.glsl
Outdated
| vec3 worldNormal = normalize(vNormal); | ||
| vec2 newTexCoor = mapTextureToNormal( worldNormal ); | ||
| vec4 texture = TEXTURE( environmentMapDiffused, newTexCoor ); | ||
| vec4 texture = TEXTURE_CUBE( environmentMapDiffusedCubemap, newTexCoor ); |
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.
OH you know what the issue might be? TEXTURE_CUBE takes in a vec3 as the coordinate instead of a vec2. The compiler probably doesn't know what to do with the combination of inputs we're giving it here, and that could be causing some more errors further on.
I think we can pass the normal directly in, like: TEXTURE_CUBE(environmentMapDiffusedCubemap, worldNormal)
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.
OH you know what the issue might be?
TEXTURE_CUBEtakes in avec3as the coordinate instead of avec2. The compiler probably doesn't know what to do with the combination of inputs we're giving it here, and that could be causing some more errors further on.I think we can pass the normal directly in, like:
TEXTURE_CUBE(environmentMapDiffusedCubemap, worldNormal)
Yes, you're correct! In many GLSL implementations, the textureCube function takes a vec3 as the first argument, representing the 3D direction vector in the cube map space. So, using TEXTURE_CUBE(environmentMapDiffusedCubemap, worldNormal) is likely the correct way to sample from the cube map based on the normal vector.
Thanks a lot for looking into this @davepagurek !!
|
@davepagurek I fixed some errors somehow, now I'm stuck with this one. Could you give some suggestion on this one too?!
|
@davepagurek Thanks a lot for pointing this out..! I apologise deeply for the confusion here, and for the delay too.. Sketch with the latest build: |
|
Sorry for the extreme delay @davepagurek ! |
|
Sketch with the latest changes: |
src/webgl/shaders/cubeVertex.vert
Outdated
| OUT vec3 localPos; | ||
|
|
||
| uniform mat4 projection; | ||
| uniform mat4 view; |
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 don't have our uniforms named projection or view, we actually had our uniforms with name uModelViewMatrix and uProjectionMatrix i think
| // Adapted from: https://learnopengl.com/PBR/IBL/Diffuse-irradiance | ||
| IN vec3 localPos; | ||
|
|
||
| uniform sampler2D equirectangularMap; |
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 you have hard-coded the value of equirectangularMap to "0" which won't work or its actually a float value.
src/webgl/p5.RendererGL.js
Outdated
| // Get the cubemap shader | ||
| const cubemapShader = this._getCubemapShader(); | ||
| this._pInst.shader(cubemapShader); | ||
| cubemapShader.setUniform('equirectangularMap', 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.
This can't be hardcoded to 0. It must have a variable, a sampler2D texture specifically.
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.
Actually I had this in mind that in WebGL (and OpenGL), when you set a texture uniform in a shader, you also need to specify which texture unit the texture is bound to.
If you see the next 2 lines:
this.GL.activeTexture(this.GL.TEXTURE0);
this.GL.bindTexture(this.GL.TEXTURE_2D, input);Here, this.GL.activeTexture(this.GL.TEXTURE0) activates texture unit 0, and this.GL.bindTexture(this.GL.TEXTURE_2D, input) binds the texture input to texture unit 0.
But lets just keep it simple and stick to what you have suggested. That seems to work fine.!
| @@ -0,0 +1,22 @@ | |||
| // Adapted from: https://learnopengl.com/PBR/IBL/Diffuse-irradiance | |||
| IN vec3 localPos; | |||
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.
In my opinion I feel for maximum compatibility, we can do this one in GL ES 100 instead of 300? Using attribute instead of IN, using the spacial variable gl_FragColor instead of defining an OUT_COLOR, and using texture2D() instead of texture()? What you feel?
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.
Well I started from that only , and was getting a lot of errors and from Dave's suggestion of using the macros instead resolved the errors , I again tried doing what u have suggested today, but strangely that gives in more errors! So I think we should stick to using these macros only :-)
You can refer to Dave's suggestion here in this comment:
#6665 (comment)
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.
Hmmm...It looks like you need to use both webgl-1 and webgl-2 modes on shaders.
| // look up the texture from the diffusedTexture map | ||
| let diffusedLight = this.getDiffusedTexture(this.activeImageLight); | ||
| shader.setUniform('environmentMapDiffused', diffusedLight); | ||
| shader.setUniform('environmentMapDiffusedCubemap', diffusedLight); |
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 am not sure but I feel diffusedLight is also a sampler2D texture, could it be passed as a uniform with samplerCube texture?
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.
Here, diffusedLight is obtained from getDiffusedTexture, which returns a CubemapTexture object. Since environmentMapDiffusedCubemap in the shader expects a cubemap texture (samplerCube), so I think diffusedLight is being correctly assigned to the environmentMapDiffusedCubemap uniform in the shader, which is of type samplerCube.
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.
Oooh...sorry. I overlooked this, previously we had diffusedLight with sampler2D texture so I got confused. I see you have updated getDiffusedTexture()
src/webgl/p5.RendererGL.js
Outdated
| let captureProjection = new p5.Matrix(); | ||
| captureProjection.perspective(Math.PI / 2.0, 1.0, 0.1, 10.0); | ||
|
|
||
| cubemapShader.setUniform('projection', captureProjection.mat4); |
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 should not set uniforms and again create a new uniform. I think we already have our uniform named uProjectionMatrix.
src/webgl/p5.RendererGL.js
Outdated
|
|
||
| this.GL.clear(this.GL.COLOR_BUFFER_BIT | this.GL.DEPTH_BUFFER_BIT); | ||
|
|
||
| cubemapShader.setUniform('view', captureViews[i].mat4); |
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.
Same with this. We should not calculate the view matrix. We already had set up the modelVeiw matrix as "uModelViewMatrix".
|
From my honest opinion I feel like you don't need to work anymore on the shaders part. vertex shaders looks good, Also fragment shaders looks correct to me. The only thing I want to recheck is, are we getting the correct uniforms in fragment shaders from the CPU? |
src/webgl/shaders/cubeFragment.frag
Outdated
| void main() | ||
| { | ||
| vec2 uv = SampleSphericalMap(normalize(localPos)); | ||
| vec3 color = texture(equirectangularMap, uv).rgb; |
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 feel like, texture() works on webgl-2 and texture2D() works on webgl-1. You may use TEXTURE() to run on both the modes.
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 had tried that earlier, tried that again today as per your suggestion but both give the same results.
#6665 (comment)
So i think we should go with texture only (not super sure however) :-)
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.
hmm...what kind of errors? If you want your code to run on both environments I think TEXTURE shouldn't give errors. I don't know why? We can debug it by first writing the code for webgl-1 (changing it to GL ES 100 version I mentioned above, let me know if texture2D works there), similarly change it to version 300 and let me know if texture() works there?
Sorry if it seems a long approach for you, But I seriously don't know why TEXTURE() is giving you errors. Also, can you share me the screenshots of the errors you are getting.
| @@ -0,0 +1,13 @@ | |||
| // Adapted from: https://learnopengl.com/PBR/IBL/Diffuse-irradiance | |||
| layout (location = 0) IN vec3 aPosition; | |||
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.
This looks good, but I feel like do we actually need this layout(). Can it be done just with IN vec3 aPosition;
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.
Well honestly speaking , I'm also not sure about what exactly it signifies here, but as i searched on web , I found this
If your application code (typically written in a language like C++ or JavaScript) explicitly binds attribute locations using glBindAttribLocation, you can omit the layout(location = 0) in the shader. The application code would look something like this:
glBindAttribLocation(program, 0, "aPosition");If your application does not do this, you should keep layout(location = 0) to ensure that the attribute location is explicitly defined.
Also , just to mention , I tried running the test sketch by omitting this, and the results were similar , So I think we should keep it for a safer side as its not causing any error.
What say?!
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.
Yes, this should not give any errors. Since, vertex shaders are just dealing with the positions, I think we don't have layout(...) at other files and they are working perfectly.
But the main reason in my mind is, since we only use layout with in we can't use it with attributes, where I think is layout used only for webgl-2? If that the case, layout should be omitted.




Addresses issue: Use Cubemaps for Light Feature Performance Improvement #6587
Changes to this solution are still in progress.
I would love to have suggestions to this . Thank you..!
PR Checklist
npm run lintpasses