-
-
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
Added a Method (panorama(img)
) which adds a sphereMapped Background.
#6808
Conversation
The PR is ready for your review, @davepagurek. Please share your thoughts on it. Additionally, I'm unsure whether to store the sphereMapping file separately or use it directly within a function. Also calling @haroon10725 as you were intrested in solving this. Please share your thoughts on this one. |
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 work on this so far!
src/webgl/p5.RendererGL.js
Outdated
@@ -1676,6 +1685,14 @@ p5.RendererGL = class RendererGL extends p5.Renderer { | |||
return this._getImmediateStrokeShader(); | |||
} | |||
|
|||
_getSphereMapping(img) { | |||
this.sphereMapping = this._pInst.createFilterShader( |
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.
It looks like this is creating a new shader every frame. Maybe you can wrap lines 1689-1691 call in if (!this.sphereMapping) { ... }
? I noticed the frame rate seems to drop in your video when you start using the sphere map background, I'm hoping it's just because of this and it'll run faster afterwards.
src/webgl/shaders/sphereMapping.frag
Outdated
@@ -0,0 +1,32 @@ | |||
#version 300 es |
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.
For maximum compatibility, do you think we can do this one in GL ES 100? So, taking out the #version
line, and using attribute
instead of in
, using the spacial variable gl_FragColor
instead of defining an out fragColor
, and using texture2D()
instead of 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.
Oh.. you mean varying
instead of in
? I guess attribute
is supported in vertex shaders only?
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.
oops yes you're right, that should be varying
src/webgl/p5.RendererGL.js
Outdated
@@ -1613,6 +1621,7 @@ p5.RendererGL = class RendererGL extends p5.Renderer { | |||
properties.quadraticAttenuation = this.quadraticAttenuation; | |||
|
|||
properties._enableLighting = this._enableLighting; | |||
properties.sphereMapping = this.sphereMapping; |
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 the shader doesn't need to be pushed/popped since we can just create it once and then reuse it with different images, so we can probably take this line out
src/webgl/p5.RendererGL.js
Outdated
@@ -1104,6 +1111,7 @@ p5.RendererGL = class RendererGL extends p5.Renderer { | |||
this._pInst.noStroke(); | |||
this._pInst.blendMode(constants.BLEND); | |||
this._pInst.shader(this.filterShader); | |||
this.filterShader.setUniform('uNewNormalMatrix', this.uNMatrix.mat3); |
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.
Since this only is used in the sphere mapping shader, can we do this and the inverseTranspose
line in getSphereMapping
where we're setting another uniform already?
src/webgl/shaders/sphereMapping.frag
Outdated
out vec4 fragColor; | ||
|
||
void main() { | ||
vec4 viewModelPosition = uModelViewMatrix * vec4(faPosition, 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 uModelViewMatrix
here is going to be made for the filter shader's rectangle, so I'm not sure it's what we want for doing reflections. Does it work if you set vViewPosition
to all zeros, since we're assuming the position of the camera is negligible, assuming the texture is on an infinitely large sphere? And if so, can we just do n = normalize(vGlovalNormal.xyz)
directly without doing a reflect
?
src/webgl/material.js
Outdated
out vec3 vNormal; | ||
out vec3 faPosition; | ||
out vec2 vTexCoord; | ||
out vec3 fvNormal; |
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 use these in the shader, we can probably take them out from here
src/webgl/shaders/sphereMapping.frag
Outdated
vec2 suv; | ||
suv.y = 0.5 + 0.5 * n.y; | ||
suv.x = atan(n.z, n.x) / (2.0 * PI) + 0.5; | ||
newTexColor = texture(uSampler, suv.xy); |
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!
You're right about that, your logic seems correct. However, despite following this approach, we aren't achieving the desired outcome. Take a look at my code for reference. #define PI 3.141592
precision highp float;
uniform sampler2D uSampler;
uniform mat3 uNewNormalMatrix;
uniform mat3 uCameraRotation;
uniform mat4 uNewModelViewMatrix;
uniform mat4 uModelViewMatrix;
varying vec2 vTexCoord;
varying vec3 faNormal;
varying vec3 faPosition;
void main() {
// vec4 viewModelPosition = uModelViewMatrix * vec4(faPosition, 1.0);
vec3 vViewPosition = vec3(0.0);
vec4 newTexColor = texture2D(uSampler, vTexCoord);
vec3 vGlobalNormal = uNewNormalMatrix * faNormal ;
vec3 n = normalize(vGlobalNormal.xyz);
vec2 suv;
suv.y = 0.5 + 0.5 * n.y;
suv.x = atan(n.z, n.x) / (2.0 * PI) + 0.5;
newTexColor = texture2D(uSampler, suv.xy);
vec4 baseColor = newTexColor;
gl_FragColor = baseColor;
} we are getting something like:- for.review.mp4 |
Good point, if we take out that matrix, we need to account for the different perspective from each pixel in a different way. Using
|
Thanks...Previously, I was confused about the rotation matrix. I believe I understand your point now and have made the necessary updates. The code is still functioning as it was before. Let me know if any thing still needs to be changed. I'll look into the minor cleanups, indentations, examples once the code looks good 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.
Thanks for making those updates! I was testing it out here: https://editor.p5js.org/davepagurek/sketches/iOfzR8r7g
It looks like the filter shader is clipping the box in the sketch at some angles. Maybe we need a clearDepth()
in there? (Should that be happening for all filter shaders?)
I also notice what seems to be a tilt happening sometimes you rotate the view. Do you know if this happens because of the new way we're calculating the initial normal, or did this happen before too? It could be that it's something about how we're applying the normal matrix, not sure yet.
Screen.Recording.2024-02-15.at.8.01.37.PM.mov
Ah, I just overlooked this one. I see where the issue lies. It's not with the new Normal or old Normal matrix, but rather with how we're calculating it. Specifically, we're performing an inverse transpose of the The result after fixing this have a look:- |
The rotation of the background looks good now! Although I noticed in the sketch that the tilt behaviour we saw in the background before now seems to be present in the reflections in Screen.Recording.2024-02-20.at.12.22.25.PM.movThis doesn't seem to be the case using the 1.9.1 beta build though: https://editor.p5js.org/davepagurek/sketches/edkYxkJAa I'm not sure what might have changed to cause that yet, do you have any ideas? |
src/webgl/light.js
Outdated
*/ | ||
p5.prototype.panorama = function (img) { | ||
this.filter(this._renderer._getSphereMapping(img)); | ||
this.clearDepth(); |
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.
Do you think we should do this for all filter shaders instead of just this one?
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're right. I was thinking we had addressed all the issues stemming from the filterShaders. However, I just noticed that the box clipping persists when we use other shaders too. Thanks for catching that.
Oh, no, this isn't a bug. I apologize. I was actually working on fixing the code above and testing it with my current camera matrix. So, I just reverted those changes. In this pull request, it's functioning somewhat like this: |
Ah ok makes sense, thanks for clarifying! |
src/webgl/light.js
Outdated
* this method, users can obtain a complete 360-degree view | ||
* of a scene. | ||
* | ||
* Using Panorama is straightforward. Similar to calling a |
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.
Rather than describing what users can do, we can address this directly to users like instructions, e.g. "Similar to calling background(color)
, call panorama(img)
before drawing your scene to create a 360-degree background out of your image." I think the paragraph above could be rephrased similarly.
Also, we generally stay away from calling anything simple or straightforward so that people don't feel bad if they struggle using something.
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 section where we describe it as being similar to background is actually maybe a good thing to start this documentation with instead of ending it with it, what do you think?
src/webgl/light.js
Outdated
* `background(color)`, users only need to call the `panorama(img)`, and | ||
* beneath it, anything created will form a 360-degree scene. | ||
* To enable 360-degree viewing, it is essential to invoke | ||
* `orbitControl()`; otherwise, the method will not function as intended. |
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.
Maybe instead of saying it's essential, since it still works if you programmatically animate a camera, we can say to either use orbitControl
or try changing the orientation of the camera to see different parts of the background.
src/webgl/light.js
Outdated
// activeImageLight property is checked by _setFillUniforms | ||
// for sending uniforms to the fillshader | ||
this._renderer.activeImageLight = img; | ||
this._renderer._enableLighting = true; | ||
}; | ||
|
||
/** | ||
* Creates a Panorama with given image. |
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 can probably take this line out if we move that last paragraph up to the top, it serves as a pretty good intro.
src/webgl/light.js
Outdated
* Creates a Panorama with given image. | ||
* | ||
* | ||
* `panorama(img)` is a method designed to transform a standard |
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.
Maybe we could try to explain what kind of images this is intended to be used with? Possibly mentioning things like maps as a common example of a 360 degree view mapped to a rectangular image, and maybe how HDRIs are also in this format
Please let me know if we can enhance the readability and user-friendliness of this document. |
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 a few more comments on the documentation and then I think we're good to go!
src/webgl/light.js
Outdated
// activeImageLight property is checked by _setFillUniforms | ||
// for sending uniforms to the fillshader | ||
this._renderer.activeImageLight = img; | ||
this._renderer._enableLighting = true; | ||
}; | ||
|
||
/** | ||
* The `panorama(img)` method adeptly transforms images such as |
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.
How do you feel about this as an update to the wording for this paragraph? I tried to simplify the language a little, and describe more what the content of the image should be.
* The `panorama(img)` method transforms images containing
* 360-degree content, such as maps or HDRIs, into immersive
* 3D backgrounds that surround your scene. This is similar to calling
* `background(color)`; call `panorama(img)` before drawing your
* scene to create a 360-degree background from your image. It
* operates on the concept of sphere mapping, where the image is
* mapped onto an infinitely large sphere based on the angles of the
* camera.
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.
Sounds good!
Co-authored-by: Dave Pagurek <[email protected]>
Co-authored-by: Dave Pagurek <[email protected]>
Sorry for the delay working on this. I have made the changes. |
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 your continued work on this, I think this is good to go now!
Thanks. Excited to see this feature how it goes.....and really thanks for your constant review on this. Love to contribute more like this❤ |
Resolves #6752
Changes:
PR-360.1.mp4
Screenshots of the change:
PR Checklist
npm run lint
passes