-
-
Notifications
You must be signed in to change notification settings - Fork 35.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
Subsurface scattering support in Blinn-Phong material #13511
Conversation
Thanks for this! I have a few issues, though...
I feel fairly strongly about both of these points, but I am glad you did this, and would like to see your example included. |
@WestLangley Thanks for your comment. I was considering it should not extend As the
In
for diffuse scattering. If it is workable for this approach, do we still need |
Correct. It should not. It should not rely on the Physical shader, either.
No, you do not have to use |
Agreed! Better to start with this by having the code in |
8ce44cc
to
1cac5c9
Compare
@WestLangley I have followed your suggestion to move the subsurface scattering shader to be Please give me some feedback to confirm if it can be merged. Thanks! |
@daoshengmu Thanks for this! As I said, since this technique is not physically based, I do not think you should be using the
With those two simplifications, this should be ready to merge. Your results should look similar the excellent example here: #9249. |
After do some adjustment of the material parameters, it looks much better. @WestLangley IIUC, the difference between PBR and Blinn-Phong is the part of specular lighting. Despite the PBR defines metalness value in its formula, we still can give it zero to make it be non-metalness. PBR's specular lighting is more closer to the cases in the real world. Therefore, some materials like skin, wax, and jade still need it although they are not metal as well. Besides, I am curious why you mentioned |
Your shader is not PBR, and should not be presented as such. I stand by my recommendations in my previous comment.
I meant remove |
@WestLangley, Interesting. Maybe I misunderstand the PBR in three.js. If I would like to use PBR, which shader I should refer? the |
PBR is based on the physics of light. But the technique you are implementing is not based on the physics of light (subsurface scattering, in this case) -- it is a "Cheap and Convincing Subsurface Scattering Look". If you want to mix PBR and this technique in your own projects, that is OK. But here, I do not want to give the impression that a shader is PBR when it is not. Both The light should be behind the object (backlit) to demonstrate the effect you are trying to incorporate into your shader. |
1cac5c9
to
7cf8cf2
Compare
It makes sense. I have followed your comments to fix my commits. Thanks for the suggestion.
I have one directional light and two point lights in my demo. The 1st point light (white color) who has a small radius and put in front of the model is for adding brightness to this model, it will not have scattering on the model's back side because of the point light's small radius. The 2nd point light (yellow color) I put behind the model with larger radius will generate scattering and be presented on the model's front side. |
I don't think the following location for the FBX file is correct: Please use |
7cf8cf2
to
1dfebc1
Compare
@Mugen87 Thanks for the correction. I have moved the fbx model to |
// LOADER | ||
|
||
var loader = new THREE.FBXLoader(); | ||
loader.load("models/fbx/stanford-bunny.fbx", function(geometry) { |
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.
FBXLoader.load()
does not return a geometry.
}); | ||
} | ||
|
||
function loadedModel(mesh, scale, material) { |
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 is not a mesh
either.
Thanks @daoshengmu. I think this can be merged, but the code is going to need cleanup, which can be done later. /ping @Mugen87 |
1dfebc1
to
0b26080
Compare
I have replaced the |
@Mugen87 It seems to have been a couple of days. I am curious when is the possible timeline to merge my patch? Thanks! |
@mrdoob I am interested if it is available to be merged. Thanks! |
@daoshengmu This PR has an r92 milestone, which means if @mrdoob approves it, it will be merged before the next release. Thank you for being patient. |
Thanks! |
Per #9249, according to the approach from GDC 2011 – Approximating Translucency for a Fast, Cheap and Convincing Subsurface Scattering Look, implementing the subsurface scattering shader for PBR material.