-
-
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
Adding metallic feature in p5.js for both IBL and non-IBL codes. #6618
Conversation
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.
Sorry for the delay taking a look at this, thanks for working on it! I've left a few comments on bits that might take a bit more thought.
src/webgl/shaders/lighting.glsl
Outdated
@@ -130,7 +141,14 @@ vec3 calculateImageSpecular( vec3 vNormal, vec3 vViewPosition ){ | |||
#endif | |||
// this is to make the darker sections more dark | |||
// png and jpg usually flatten the brightness so it is to reverse that | |||
return pow(outColor.xyz, vec3(10.0)); | |||
if(metallic != 0.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.
Right now there's a bit of a jump between the two branches, since when metallic
is 0.00001, we return pow(outColor.xyz, vec3(8.0)
, but when metallic
is sliiightly smaller at 0.0
exactly, we return pow(outColor.xyz, vec3(10.0)
. Maybe we can remove the if/else and use 10.0
instead of 8.0
?
src/webgl/shaders/lighting.glsl
Outdated
@@ -73,9 +74,18 @@ LightResult _light(vec3 viewDirection, vec3 normal, vec3 lightVector) { | |||
|
|||
//compute our diffuse & specular terms | |||
LightResult lr; | |||
float invertValue = (1.0 - (metallic / 100.0)); | |||
float specularIntensity = mix(0.5 , 1.0, invertValue); | |||
float diffuseIntensity = mix(0.2, 1.0, invertValue); |
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 a test, I used fill('red')
and specularMaterial(100)
on a torus.
With imageLight()
, when you bring metalness
up to the maximum value, there's no remnant of the fill color.
When using lights()
instead, it's still very prominent:
I think to add metalness as a feature, we should figure out a way to deal with these two cases more consistently. There's two bits involved I think, the color, and the balance of diffuse/specular components.
For color, Threejs had this to say when metalness was still part of their Phong material:
If set to true the shader multiplies the specular highlight by the underlying color of the object, making it appear to be more metal-like and darker. If set to false the specular highlight is added ontop of the underlying colors.
Maybe that's what we should do too to have consistent color?
I sincerely apologize you @davepagurek for the delay in completing this task. My time was consumed by exams, demanding my full attention on academic commitments. I would like to provide a concise overview of the changes incorporated in this commit.
Changes: (NEW-IBL)(OLD-IBL)Changes: (NEW) NON-IBL(OLD) NON-IBLPlease have a look and let me know if there are still something missing with this. |
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 problem at all! It works out because I've also been away from Github the past few days 😅
This is looking good, mapping the specular color based on the metalness works well! I left a few suggestions on some details.
For non-image lights, I guess if we let the diffuse part go to 0, then I guess the problem is that our shape lacks much definition outside the specular highlight, and is just a flat color:
Maybe we can experiment with, as metalness goes higher, increasing the reflectiveness of the edges using the fresnel factor? A quick test without using anything like shininess or the ambient color looks a bit like this:
That's just one idea though, I mentioned earlier that it seems like an earlier version of Threejs included metalness in their Phong material. It could be helpful to see if you can find a demo of materials in that version to see what metals look like without image light as a point of reference for us.
src/webgl/p5.RendererGL.js
Outdated
const metalnessFactor = Math.min(this._useMetalness / 100, 0.6); | ||
const nonMetalnessFactor = Math.max(1 - metalnessFactor, 0.4); | ||
|
||
this.curSpecularColor = this.curSpecularColor.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.
By modifying its current value, I think that means the color will be slightly different for every successive shape we render. Instead maybe we can do something like let specularColor = [...this.curSpecularColor]
outside of the if statement and modify that, and pass that in to the uniform on line 2066 below? That way we aren't storing the new color, so it gets recalculated fresh for the next object.
src/webgl/p5.RendererGL.js
Outdated
@@ -2041,6 +2041,17 @@ p5.RendererGL = class RendererGL extends p5.Renderer { | |||
_setFillUniforms(fillShader) { | |||
fillShader.bindShader(); | |||
|
|||
if (this._useMetalness > 0 && !this.curFillColor.every((value, index) => | |||
value === 1)) { | |||
const metalnessFactor = Math.min(this._useMetalness / 100, 0.6); |
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.
Did you have some visual examples that lead you to arrive at 0.6 as the max metalness and 0.4 as the min non-metalness instead of letting them both go from 0 to 1?
src/webgl/shaders/lighting.glsl
Outdated
} | ||
float invertedMetallic = 1.0 - (metallic / 100.0); | ||
float mappedValue = mix(1.2, 10.0, invertedMetallic); | ||
return pow(outColor.xyz, vec3(mappedValue)); |
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 notice that as I bright the metalness slider up, in the middle, the shadows dip darker before they finally brighten:
I wonder if this is because we change the specular/diffuse balance linearly, but the power exponentially (since we linearly interpolate the exponent.) Maybe we'll avoid this dip if, instead of linearly interpolating the exponent, we linearly mix between to fixed exponents, like this?
return mix(
pow(outColor.xyz, vec3(1.2)),
pow(outColor.xyz, vec3(10)),
invertedMetallic
);
Thanks, @davepagurek , for suggesting I refer to three.js. I've noticed a couple of issues in p5.js:
Here's what's working for metalness in p5.js:
three.jsWhatsApp.Video.2023-12-23.at.01.04.47_5e6ddb42.mp4p5.js(middle silider is of ambientLights value)comparision.with.three.mp4Sorry for bothering you again on this ;) |
visual examples that lead me to arrive at 0.6 as the max metalness and 0.4 for min etc. was something like When using 0.6 and 0.4 values.When not using 0.6 and 0.4Initially, with metalness values of 0.6 and 0.4, the material appeared a bit dark and lacked the desired metallic look. However, this setup worked well for non-IBL (Image-Based Lighting), where a fully metallic appearance was necessary. After experimenting, I decided to remove the specific metalness values, and the material now looks suitable for non-IBL scenarios. I'm still exploring the ideal approach for IBL, as it requires a different treatment. |
I's pretty interesting that threejs doesn't use ambientLight on metals. One way of interpreting The threejs interpretation seems different. Maybe they aren't trying to use a consistent metaphor, and just think that ambient light looks too bright and uniforms for metals, and diminishing it makes the metals look more believably like metal? Anyway, if threejs does it, and if we think it looks better too, then I think following suit is justifiable.
Thanks for adding this context! I see where you're coming from now. I agree that it looks more natural in your example with those mins and maxes in place. I also think that for artistic purposes, one may want to push the values further, so we might not want a hard limit. Here's an idea for a way to maybe do both: you know how metalness(val) {
const metalMix = 1 - Math.exp(-val * 0.1); // We can tinker with the 0.1 multiplier
// ...
} ...then what happens is that |
@davepagurek Here's the demo for ambientLights() and for exponential metalness() value I have done the required changes. Please let me know what you think about it's output. metalballwith.mp4 |
Little bit of screenshots for you @davepagurek which can help us to identify the mistakes if any. |
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, I think the behaviour feels good to me! I think next we can look at the code itself, so I've left some comments to clean up the implementation a bit.
It would also be nice to have a test or two about this so that we can know when things change. We still don't yet have the visual test system merged in, do you think there's anything we can test without that that would still be useful?
src/webgl/material.js
Outdated
p5.prototype.metalness = function (metallic) { | ||
this._assert3d('metalness'); | ||
const metalMix = 1 - Math.exp(-metallic * 0.01); | ||
this._renderer._useMetalness = metalMix * 100; |
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 divide by 100 later on, maybe instead we can omit this factor here and elsewhere to simplify?
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 btw I'm referring to the metalMix * 100;
thing. If we just set it equal to metalMix
, then we don't have to do const metalnessFactor = this._useMetalness / 100;
later (and similarly in the shader), we could just use _useMetalness
directly.
src/webgl/p5.RendererGL.js
Outdated
let SpecularColor = [...this.curSpecularColor]; | ||
|
||
if (this._useMetalness > 0 && !this.curFillColor.every((value, index) => | ||
value === 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.
Do you mind explaining a bit what the scenario is that we're checking for here with the .every(...)
? Is it necessary, since we don't do it for ambient colors?
src/webgl/p5.RendererGL.js
Outdated
@@ -2039,6 +2041,20 @@ p5.RendererGL = class RendererGL extends p5.Renderer { | |||
_setFillUniforms(fillShader) { | |||
fillShader.bindShader(); | |||
|
|||
let SpecularColor = [...this.curSpecularColor]; |
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.
Generally speaking we use CapitalCamelCase
for classes and lowerCamelCase
for variables, so we should probably start this with a lowercase letter to match that convention
src/webgl/p5.RendererGL.js
Outdated
mixingArray.forEach((ambientColors, index) => { | ||
let mixing = ambientColors - this._useMetalness / 100; | ||
mixing = Math.max(0, mixing); | ||
mixingArray[index] = mixing; |
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 might be a tad cleaner, instead of setting each index here, to instead do mixingArray = mixingArray.map((color) => ... )
src/webgl/p5.RendererGL.js
Outdated
@@ -2086,8 +2103,17 @@ p5.RendererGL = class RendererGL extends p5.Renderer { | |||
|
|||
// TODO: sum these here... | |||
const ambientLightCount = this.ambientLightColors.length / 3; | |||
const mixingArray = [...this.ambientLightColors]; |
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 we do this for both specular and ambient colors, maybe we could call this variable something that describes how it will be used by the shader? e.g. mixedAmbientLight
? (Also whatever naming convention we use here, it would be good to name the array of specular light components similarly.)
src/webgl/shaders/lighting.glsl
Outdated
lr.diffuse = _lambertDiffuse(lightDir, normal); | ||
|
||
float invertValue = 1.0 - (metallic / 100.0); | ||
float specularIntensity = mix(0.4, 1.0, invertValue); |
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 is equivalent to this:
float specularIntensity = mix(0.4, 1.0, invertValue); | |
float specularIntensity = mix(1.0, 0.4, metallic / 100.0); |
Might be a bit easier to reason about without adding the extra in-between variable with the flipped value.
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, it looks like these changes have a different indentation level compared to the surroinding code, we should probably make it match
@@ -130,7 +137,12 @@ vec3 calculateImageSpecular( vec3 vNormal, vec3 vViewPosition ){ | |||
#endif | |||
// this is to make the darker sections more dark | |||
// png and jpg usually flatten the brightness so it is to reverse that | |||
return pow(outColor.xyz, vec3(10.0)); | |||
float invertedMetallic = 1.0 - (metallic / 100.0); | |||
return mix( |
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 line also looks like the indentation is a tad off
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 arguments to this function still need to be indented once more
src/webgl/shaders/lighting.glsl
Outdated
@@ -164,7 +175,7 @@ void totalLight( | |||
if (j < uPointLightCount) { | |||
vec3 lightPosition = (uViewMatrix * vec4(uPointLightLocation[j], 1.0)).xyz; | |||
vec3 lightVector = modelPosition - lightPosition; | |||
|
|||
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 like some extra spaces got added here
Hi @davepagurek , I've made the necessary changes. Regarding the test confusion, I believe that after merging the visual tests, they will become more efficient and accurate. In the meantime, we can perform tests on specific values. For instance: Regarding the code snippet if |
Those tests sound good! I'm not sure I fully understand yet the situation with reflections and white. Maybe some screenshots would help to explain what a white metal would look like without the |
@davepagurek I have written the tests and they are totally working ( I have checked myself). I am not sure about any simplification to it. Please suggest if you have any. Also simplified without .every() in IBLHere are my opinions with .every() in IBLWithout .every() in non-IBL.with .every() in non-IBLPlease leave a review or changes if any. If .every() does not looks good to you feel free to suggest. I will remove it. |
It sounds like it's trying to address the fact that if you only set a specular material, since metals uses the fill color for all its materials, it unexpectedly shows a white metal instead of the specular color? I think that would actually be the expected result, as long as we document that metalness uses the fill color for specular colors too. I'm open to a suggestion to improve it, but currently the issue is that checking if the fill is completely white isn't a reliable check to see if only a specular material has been set. One can manually set a fill of 255 and then it gets ignored, and if you set the fill to something just under 255 (e.g. 258) then it behaves differently: Screen.Recording.2024-01-05.at.8.36.45.AM.mov |
test/unit/webgl/p5.RendererGL.js
Outdated
myp5.noStroke(); | ||
myp5.metalness(100000); | ||
myp5.sphere(50); | ||
expect(myp5._renderer.mixedSpecularColor).to.have.same.members( |
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 adding the test! For this and the above test, I think order matters, so we probably want either .to.have.same.ordered.members
(or maybe just .to.deep.equal(...)
?)
You are correct, @davepagurek. I realize now that I had a different understanding of the idea. Thank you for the clarifications. I have committed the changes. Do you find the code satisfactory? Can I proceed with updating the documentation? Or do you have any other suggestions regarding the code? |
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 code is looking good, I left a few little comments in the shader with some little whitespace cleanups, but otherwise good to go!
For documentation, I think we should probably mention that metals use only have reflections (so only specular lights, is there a way we can avoid defining technical terms like "specular" while doing this for simplicity?), and their fill color is used to tint all reflections. We maybe also should use an example where we set a fill color, or maybe set the color of the light rather than setting the specular material color.
src/webgl/shaders/lighting.glsl
Outdated
@@ -141,7 +148,7 @@ void totalLight( | |||
) { | |||
|
|||
totalSpecular = vec3(0.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.
tiny change, but we can take out these extra spaces
@@ -130,7 +137,12 @@ vec3 calculateImageSpecular( vec3 vNormal, vec3 vViewPosition ){ | |||
#endif | |||
// this is to make the darker sections more dark | |||
// png and jpg usually flatten the brightness so it is to reverse that | |||
return pow(outColor.xyz, vec3(10.0)); | |||
float invertedMetallic = 1.0 - (metallic / 100.0); | |||
return mix( |
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 arguments to this function still need to be indented once more
ooh..I think you are checking the outdated file. I think i have fixed some indentation and the extraspaces. Also updated the docs. For the docs do you have any further suggestions @davepagurek ? |
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.
The text is looking great! I think the last thing to iterate on is maybe the examples to do our best to demonstrate how the feature works with as simple code as we can.
src/webgl/material.js
Outdated
* diffuse or ambient lighting. Metals use a fill color to influence the overall | ||
* color of their reflections. Pick a fill color, and you can easily change the | ||
* appearance of the metal surfaces. When no fill color is provided, it defaults | ||
* to using white color. |
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.
Tiny update, I think we can just say "white" here without also saying "color":
* to using white color. | |
* to using white. |
src/webgl/material.js
Outdated
* specularMaterial('gray'); | ||
* ambientLight(255); | ||
* shininess(2); | ||
* metalness(100); |
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 would be great for one of the examples to have a slider that controls metalness so people can see the effect it has. Maybe this one would be good for that? A quick update to this that I was trying out:
let slider
function setup() {
createCanvas(100, 100, WEBGL);
slider = createSlider(0, 200, 100);
}
function draw() {
noStroke();
background(100);
fill(255, 215, 0);
pointLight(255, 255, 255, 5000, 5000, 75);
specularMaterial('gray');
ambientLight(100);
shininess(2);
metalness(slider.value());
sphere(45);
}
The things I was hoping to improve upon were:
- added a slider to control metalness
- a different background color so you can see the outline of the shape better
- lights that show the form of the shape a bit more clearly (it's definitely still not perfect, any ideas on how to improve this? maybe making the light orbit the sphere over time? maybe using a torus instead of a sphere?)
- less ambient light so the non-metal version is less blown out and you can see the highlights better
src/webgl/material.js
Outdated
* } | ||
* function setup() { | ||
* createCanvas(100, 100, WEBGL); | ||
* slider = createSlider(0, 400, 100, 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.
Maybe for this slider (and in the next example if we add a slider) we can add a label next to it to say what parameter it's controlling?
src/webgl/material.js
Outdated
* background(220); | ||
* imageMode(CENTER); | ||
* push(); | ||
* translate(0, 0, -200); |
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 now have a clearDepth
method, maybe this could be simplified to just image(img, 0, 0, width, height)
followed by clearDepth()
without translating backwards and scaling?
src/webgl/material.js
Outdated
* metalness(100); | ||
* noStroke(); | ||
* scale(2); | ||
* sphere(15); |
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.
Instead of scaling before this, could we just do sphere(30)
?
Commited the suggested changes @davepagurek
I opted for a Also I attempted to label next to the slider, but it looked better at the top left corner. Let me know if you have any suggestions. |
I don't know why I always have an oversight @davepagurek . I am extremely sorry. I guess now we are ready to merge hahaha |
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 problem, thanks for your patience in working through everything! I think it's good to go now!
Thanks for merging it.❤ |
Resolves #6609
Changes:
CHANGES FOR IBL:-
imgeligh.mp4
CHANGES FOR NON-IBL:-
metalBallnon-ibl.mp4
Screenshots of the change:
PR Checklist
npm run lint
passes