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

Completely wrong #1

Open
davidmanzanares opened this issue Aug 16, 2016 · 4 comments
Open

Completely wrong #1

davidmanzanares opened this issue Aug 16, 2016 · 4 comments

Comments

@davidmanzanares
Copy link

I'm sorry but the code has multiple bugs, it doesn't match the Oren Nayar formula

  • Variable A , is completely wrong calculated, your code even uses variables that are not needed to calculate A.
  • Variables s and t are probably wrong too. I'm not sure about this one, since your formulas can be equivalent... But I don't think so... A prove would be appreciated if I'm wrong.

I guess that this is un-maintained code, but the fact that this is the first Google result of "oren nayar glsl" makes me want to warn everyone from using this.

The original Oren-Nayar paper can be found here: http://www1.cs.columbia.edu/CAVE/publications/pdfs/Oren_SIGGRAPH94.pdf

@mikolalysenko
Copy link
Contributor

If you want to send a PR, go for it.

@davidmanzanares
Copy link
Author

I'm currently writing my own Oren-Nayar code. I'll probably upload it to another repository since I don't like/need all the JS around this shader. However, I searched around this topic (Oren-Nayar) and I only found:

I can comment here my current version, which works. However, I'm not sure about its correctness.

@mikolalysenko
Copy link
Contributor

There isn't any JS code in the repo, just GLSL. If you want to to fix it up just go for it.

@ranjak
Copy link

ranjak commented Jan 13, 2017

@dv343 Apparently the shader was implemented according to the instructions found on this post.

The A and B first mentioned in this post are not the same as the ones used in the "qualitative" model, as the author has apparently calculated them from the full model instead. Unfortunately, the author doesn't give the detail of his calculations.
As for the rest of the formula, I'm quite confident it is correct, as I did the math myself on paper starting from the original qualitative formula, and reached the same result.
If you're interested, I implemented it here using the original A and B from the qualitative formula (so my implementation should be 100% correct wrt the original qualitative model).

The only incorrect thing I can see here is in the documentation: sigma (the roughness parameter) is an angle in radians, AFAICT between 0 and PI/2, not a ratio.
However, it would be a ratio if using the A and B constants created by Yasuhiro Fujii in the aforementioned post (the one he uses to fix the energy conservation law violation). In this case, it is called sigma' instead of just sigma. For reference, this method is implemented in Blender's Cycles renderer (here).

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

No branches or pull requests

3 participants