Skip to content
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

Corrected Schlick specular term and added specular normalization constant #1518

Merged
merged 2 commits into from Mar 21, 2012
Merged

Corrected Schlick specular term and added specular normalization constant #1518

merged 2 commits into from Mar 21, 2012

Conversation

WestLangley
Copy link
Collaborator

No description provided.

@WestLangley
Copy link
Collaborator Author

@alteredq

Sidenote: it's better to create separate branches

Yes, {blush}, I know. I am just learning git, and quite frankly, I have no idea what I am doing... :-)

@zz85 has alreaady merged in all my changes for the arrow thingie into his version.

@alteredq
Copy link
Contributor

Don't worry, it's normal, you should see which mess we made sometimes with git with @mrdoob ;).

Cool that you took the plunge, once you learn the basics, workflow with git is pretty simple, just you need a good client (I use TortoiseGit on Windows).

About specular term changes - are you sure they are correct? I tried and it seems they completely mess several examples (though problem may be these may had parameters tuned to compensate for shader bugs?). It feels like if specular term was sucking out light.

@WestLangley
Copy link
Collaborator Author

I am certain as I'll ever be that it is correct. With proper parameters, the results are incredible.

Yes, the parameters in the examples will have to be changed.

As Hoffman points out, for most materials, spectral is 2% (0x050505) to 5% (0x0d0d0d) and shininess can vary from 0 to 1000+. Keep ambient the same as diffuse.

Metals now work well with no diffuse term, fairly high spectral reflectance, and shininess of 10 or so.

The glare at low angles off of flat surfaces looks great now, too.

BTW, thank you and @mrdoob for all your hard work on this project. I am learning a lot from you.

@alteredq
Copy link
Contributor

Thank you, you bring some more rigor to this operation ;)

About specular: what puzzles me is that it seems like sometimes specular has negative values - overall lighting got darker, not just specular term. This shouldn't happen even with wrong parameters. It should never be darker with specular than just with ambient and diffuse.

Edit: wow, I get completely different results on ANGLE and OpenGL. Things look fine in OpenGL but are really messed up in ANGLE. I guess you work on platform that uses OpenGL.

angle
opengl
Bah, this is tricky. It doesn't even happen every time, some examples are fine, some are messed up.

@alteredq
Copy link
Contributor

This seems worse than I thought - it looks related to #1292, something going wrong in HLSL triggered by particular GLSL code constellation around loops :S

Well, at least we know your changes were ok. Now we just need to figure out some workaround for ANGLE bug. The one I used before works (add extra light), but this is quite costly. This change was relatively small and different from what was triggering problems before, so maybe there is some other way.

@WestLangley
Copy link
Collaborator Author

I'm on Mac.

@alteredq
Copy link
Contributor

This is really really bizarre, even more so than this other issue.

This doesn't work:

float specularNormalization = ( shininess + 2.0 ) / 8.0;

This does:

float specularNormalization = ( shininess + 2.01 ) / 8.0;
float specularNormalization = ( shininess + 1.99 ) / 8.0;

Well, I guess I will just leave it at that and pray this doesn't break for some combination of other parameters (it's not about value of shininess + number, if I mess with shininess to compensate, it still works).

@youngershen
Copy link

WOw , that is really cooool

@mrdoob mrdoob merged commit 322839d into mrdoob:dev Mar 21, 2012
npmcomponent pushed a commit to npmcomponent/timoxley-threejs that referenced this pull request Jan 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants