Critical: Incorrect sampling / randomness!

Project:YafaRay
Component:YafaRay Core
Category:bug report
Priority:critical
Assigned:David Bluecame
Status:ready to commit
Description

Hello,

While studying integrators and materials I saw something in the mcintegrator.cc code that didn't make much sense to me.

https://github.com/YafaRay/Core/blob/master/src/yafraycore/mcintegrator.cc#L68

This is supposedly (as far as I understand) to be used in photon and path tracing for indirect illumination, and it gets the illumination that a randomly chosen light gives to a surface.

What does not make sense to me is that the result is multiplied by the total number of lights. I suppose this has something to do with a subsequent division by the total number of lights later on in the integration.

However, it looks "fishy" to me and the first thing I thought was: what happens if I make a scene with only one light, render it, and compare it with the same scene with that light plus other 5 lights that are in the scene but pointing outwards so they don't illuminate the scene at all?

The results are disheartening

With Photon mapping, only one light:

test_mat1c - Photon map - original MC without extra lights.pngtest_mat1c - Photon map - original MC without extra lights.png

 

Now, same scene with the same light plus other 5 spot lights pointing upwards (so they don't illuminate the scene). As I was afraid of, due to that "number of lights" multiplication effect now the indirect light is incorrectly calculated:

test_mat1c - Photon map - original MC plus 5 lights not affecting scene.pngtest_mat1c - Photon map - original MC plus 5 lights not affecting scene.png

 

Same problem happens with path tracing. With only one light we get this render:

test_mat1c - Path tracing - original MC without extra lights.pngtest_mat1c - Path tracing - original MC without extra lights.png

 

If we add 5 more spot lights pointing upwards, not illuminating the scene, we get this incorrect result:

test_mat1c - Path tracing - original MC plus 5 lights not affecting scene.pngtest_mat1c - Path tracing - original MC plus 5 lights not affecting scene.png

 

I don't know... I'm still trying to understand how Yafaray integration and materials work and I could be wrong, but this all look very bad in my opinion. What do you think?

I will attach the scene itself to the first comment for your evaluation.

Comments

#1

This is the scene I used for the tests. See attached file.

AttachmentSize
test_mat1c.blend.zip 129.93 KB

#2

Yes, it sounds really weird. But as you're the only integrator coder here...

Maybe fixing this solves some annoying too much brightness in corners that I always get.

Can you test what happens without this multiplier?

I've seen some radiosity intensity multiplicator in other softwares, like Lightwave, but as an option only for tweaking results.

Let's see if any yafaray guru helps. :)

#3

Yes, it must be wrong because with 40 lights the result gets totally broken. And this does not happen with cycles, for example.

#4

Hello,

Thanks for your comments. Yes, I tested without the multiplier but the indirect light gets too dark, darker when you increase the total lights. I guess that makes sense because you are randomly sampling 1 of N lights, and therefore the contribution of that light is 1/N without the multiplier.

There must be something wrong somewhere (I know I'm not being very specific but I don't know yet). I will keep investigating!

#5

Title:Path Tracing and Photon mapping incorrectly handling multiple lights» Critical: Incorrectly sampling randomness!
Status:needs review» needs work

Hello,

After investigating this a lot, I believe that the Total Ligths multiplier in mcintegrator.cc is correct, as it has to compensate for the 1/Nth sampling when there are N lights to be randomly chosen from.

What I believe now is that there is not enough randomness in the main sampling processes. For example in the second example (path tracing) with 1 light + 5 non-contributing lights, the problem is that when sampling the material in the cube, at some places it *never* generates secondary rays that land on the sphere!!  No matter how many samples you use, it just never happens because the sampling is only generating rays with a certain range for the angles for each light.

That's *really bad* and unless I'm mistaken it means that our current random number generators are broken, or the samplers are broken, or we need to introduce additional randomness in a similar way as in the case: http://yafaray.org/node/792

In fact, if I introduce additional radomness in the creation of the s1, s2 "random" components of the sample, the problem gets solved (apparently). However, I'm not sure what the consequences of this would be or in how many algorithms should we introduce this additional randomness. I will keep investigating this...

For example, this is the previously (wrong) result in path tracing with 1 light + 5 non-contributing lights:

 

Now, after applying a possible fix with extra randomness for sampling in path tracing. With only one light illuminating the scene I get this:

 

And with the extra randomness, one light + 5 non-contributing lights I get this, which now looks correct (a bit noisier but that's normal as we have to divide the total samples by 6 lights and not only one). To me it looks so much better now!

AttachmentSize
test_mat1c - 001 original.png 117.42 KB
test_mat1c - 001 additional randomness one light.png 139.54 KB
test_mat1c - 001 additional randomness one light + 5 non contributing lights.png 171.5 KB

#6

Similarly, in the photon integrator I got previously this (wrong) result with 1 + 5 lights:

 

And with the added randomness I'm getting this result (which I believe is correct) with 1 + 5 lights:

AttachmentSize
test_mat1c - original photon map 1+5.png 119.8 KB
test_mat1c - additional randomness photon map 1+5.png 145.46 KB

#7

The problems now are:

* Determine where to add this extra randomness in the code. Only in the areas that caused the issues described in this bug tracker?  If so, I can do it quickly. However, perhaps this lack of randomness could be affecting other processes in YafaRay causing other subtle artifacts or difficulties to get a good render / convergence. So I need to think about where to add extra randomness and where not.

* Determine the potential negative consequences of adding this extra randomness. Could it affect negatively renders?

* Keep an eye on render speed. I will try to use fast random number generators (we have one already in the code I used for these "fixed" examples) to try to avoid increasing render times. However, if what I suspect is correct, even if the render time increases a little bit, perhaps better randomness will allow the render to converge with fewer samples, giving an overall speedup for the same quality?

I will keep investigating this as this is absolutely critical in my opinion. Please let me know if you have any suggestions or ideas about all this...

#8

Title change to: "Critical: Incorrect sampling / randomness!"

#9

Title:Critical: Incorrectly sampling randomness!» Critical: Incorrect sampling / randomness!

#10

What I whink is we will improve speed in the cost of noise convergence, but I'm not sure why the power has to be divided by the number of lights. Does it makes more sense to be divided by the contribution of each light?

Is this related?

http://www.luxrender.net/wiki/LuxRender_Render_settings#Light_Strategy

It seems strange but you ought ask some help to luxrender coders, if you want, maybe they can give you some insight about that. They are very helpful.

Regards.

#11

Hello,

Thank you for your comments. I've found that the problem is located in one sampling algorithm (which is *really bad*). There is a sample generator algorithms that is not generating properly random numbers. This is a fundamental problem I need to address.

The problem here is that I suppose whoever implemented that sampler algorithm had a reason for it. The algorithm itself follows a white paper about samplers, but I wonder if it's really correct/incorrect or if we are using it incorrectly in the code.

In any case, I need to do quite a fewtests to determine what's the best way forward. I'm more inclined to increase the randomness to reduce these weird issues, although I'm not sure if it will affect the speed of convergence. Anyway, we cannot just ignore this, this is introducing severe artifacts in all images.

#12

Status:needs work» ready to commit

Hello,

After a long and difficult investigation I found the problem in a combination of factors:

* First, the EstimateOneDirectLight is supposed to pick a sample randomly among the existing lights and multiply its radiance by the total number of lights. This approach would only work if the samples are taken randomly but uniformly from all lights.

* Second: unfortunately the sampler was not picking samples uniformly from all lights. In fact it was picking more samples from the first light and fewer samples from the last, in a decreasing number of samples for each light!!

* Third: due to a weird correlation between the sampling algorithm used for sampling materials and the sampling algorithm used for sampling lights (was giving the same sampling values for both!) the calculated lighting was not uniform in the materials as it was only generating a certain range of samples per light source, always in a similar range of vector angles, etc.

 

Therefore this was causing severe artifacts in the images in Photon Mapping and Path Tracing when more than one light was used. Probably this went unnoticed for a *very* long time because lightling in a scene from several lights is often more or less uniform. However for non-uniform lighting cases the artifacts become evident. I've added some examples below (see also attached files)

 

I've made an attempt to fix this problem by changing the EstimateOneDirectLight so it uses correlative numbers per thread. Strictly speaking they are not 100% pure correlative as the variable I created is not thread safe because making it thread safe would imply using mutexes and decrease speed. Fortunately despite this limitation, the fix seems to be working nicely, at least in simple scenes. However it would need much more testing with complex scenes to ensure the fix is good enough and no new artifacts are introduced now.

The fix is described here: https://github.com/YafaRay/Core/commit/11551b779ce9234a348d6aefa56af2289bd76d97

 

For example, taking the scene from the original bug report, this is how it looks like with 1 light + 5 contributing lights, with the fix included, in photon mapping:

 

And now in path tracing. Both look correct now, more noisy than the "1 light only" versions of course (more noise is to be expected) but without the weird artifacts.

 

Moreover, I created a new scene (see attached zip file) with a non-uniform example with white materials and colored spot lights. Without the fix, the results show a lot of artifacts both in photon mapping and path tracing (both show artifacts, but path tracing shows really horrible artifacts)

 

Now the same scene in path tracing and photon mapping with the fix applied. It looks much better to me...

I hope this solves the problem without introducing new artifacts. In any case the previous system was totally wrong anyway, but we should do many tests with different types of scenes to ensure this is really working well now.

I expect scenes to show a bit more noise than before, but now the lighting should be more correct and more realistic. Crossing fingers!

AttachmentSize
test_mat1d.zip 789.77 KB
test_mat1d - path after fix.png 176.08 KB
test_mat1d - path before fix.png 162.18 KB
test_mat1d - photon after fix.png 153.5 KB
test_mat1d - photon before fix.png 169.81 KB
test_mat1c - path tracing with fix correlative,halton - with 5 non-contrib lights.png 159.28 KB
test_mat1c - photon map with fix correlative,halton - with 5 non-contrib lights.png 122.33 KB

#13

This is great! When will we have a build with this fix?

Thanks!

#14

Hello,

My estimation would be to have the new v3.2.0-beta released in around 1-2 weeks, including all these fixes.

#15

In any case, an ALPHA preview has been published today, you can test it to see if it helps:

http://www.yafaray.org/community/forum/viewtopic.php?f=12&t=5232