All integrators: issues with random_t pseudo-random number generator

Project:YafaRay
Component:YafaRay Core
Category:bug report
Priority:critical
Assigned:David Bluecame
Status:closed
Description

Hello,

I've been implementing russian roulette in path tracing partially based on the code in the bidirectional integrator and usage of the pseudo-random number generation random_t

Unfortunately I've just discovered there is something wrong, either in the implementation of random_t class or in Bidirectional usage of it.

Some example images in the first comment below.

 

Comments

#1

Priority:normal» critical

Example with a simple glossy material and bidirectional, rendered with 32 pixel tile size. It apparently looks normal but you can see some patterns in the lower part of the render

Same example rendered with 8 pixel tile size. Now the patterns are obvious

Same example rendered with 4 pixel tile size. Now the patterns are even worse

AttachmentSize
pseudo_random1 - tiles 32pixel.png 250.71 KB
pseudo_random1 - tiles 8pixel.png 204.37 KB
pseudo_random1 - tiles 4pixel.png 143.85 KB

#2

Title:Bidirectional integrator: issues with random_t pseudo-random number generator» All integrators: issues with random_t pseudo-random number generator
Status:active» ready to commit

Hello,

Without evaluating whether the YafaRay implementation of random_t is correct or not (which is something else we probably should investigate just in case), for now I've found out that one possible problem was insufficiently radom seeding of the random_t prng in all integrators, including SPPM.

For example, during the first AA pass, offset=0 and therefore the pnrg is seeded always with 123 for each tile, creating non-random patterns. For offsets > 0, for some weird reason patterning also appears. Therefore I've added a pseudo-random number to the prng seeding to try to add more randomness to the process.

This change is here: https://github.com/YafaRay/Core/commit/60452c5586772bcf2eccc47ed5fa9b293... and it affects *all* integrators, not only bidirectional. It also affects other secondary processes derived from this seeding such as the new path tracer russian roulette and others.

I believe this change should be beneficial in general, but it's difficult to know for sure. We need to keep an eye on this from now on, just in case new artifacts appear now.

As an example, this is a bidir scene with tile size 4, before the fix:

 

And this is the same scene with same parameters and tile size, after the fix:

AttachmentSize
pseudo_random1 - tiles 4pixel b4 fix.png 143.85 KB
pseudo_random1 - tiles 4pixel after fix.png 251.55 KB

#3

For your information, I've just found out an improvement in Bidir thanks to the fix above (this was probably one of the several reasons why Bidir does not work correctly)

Before the "random" fix, bidir 10 samples, 9.6s render time:

 

After the "random" fix, same scene with same parameters, 9.5s render time:

 

 

:-)

AttachmentSize
test_mat1 - bidir before random fix.png 567.98 KB
test_mat1 - bidir after random fix.png 673.38 KB

#4

As I understand it, in the first example issues were not produced by the tiles algorithm but by the multithreading, as every thread is using its own pseudo random number generator, right ? BTW a new random number generator is in the Rodrigo's radar since long time ago, particularly about the issues in SPPM.

My work · Grey18 workflow · Sampling strategy · [url=http://www.yafaray.org/node/816]SPPM

#5

By the way, PBRT uses and implementation of the PCG pseudo random number generator by Melissa E. O'Neill. I think this is the generator Rodrigo is talking about.

http://www.pcg-random.org/

https://books.google.es/books?id=iNMVBQAAQBAJ&lpg=PA1065&ots=iuBhGk3oSJ&...

https://www.cs.hmc.edu/~oneill/

My work · Grey18 workflow · Sampling strategy · [url=http://www.yafaray.org/node/816]SPPM

#6

Hello,

I don't think this is related to multithreading. In fact, if you render the scene with one single thread the same effect happens. I've attached the scene so you can try.

The problem is in the tiling algorithm, better said in the part where we seeded the random_t pnrg object at the beginning of each tile. The way it was seeded was incorrect (it was using offset * coordinates + constant) but offset is 0 the first AA pass, for example so it was seending the pnrg object in each tile with the same value, causing this pattern.

Moreover, maybe there is something wrong with the random_t algorithm as you say. I could spot several weird things, for example even with different seeds the results still show patterns. Without replacing the entire random_t algorithm the only "quick fix" I could come up with was to add extra randomness to pnrg seeding itself. It seems it's working, although I agree with you that a better algorithm for random_t would be a good idea.

For now I will keep it as it is now, as it's improved, and in the medium term we can discuss replacing the random_t algorithm...

AttachmentSize
pseudo_random1.blend.zip 72.73 KB

#7

Hi David

Thanks for the explanation. What would it be the impact of the new seed in adaptive sampling regarding time and quality?

My work · Grey18 workflow · Sampling strategy · [url=http://www.yafaray.org/node/816]SPPM

#8

Hello,

I don't think there will be any impact on speed, because the seeding happens only once per tile. About quality, I expect it should be the same or a bit better (as shown above), but we will need testing to make sure no new issues happen now.

#9

Version:» <none>
Component:Code» YafaRay Core
Status:ready to commit» fixed

Solved in the new version v3.2.0. See: http://www.yafaray.org/community/forum/viewtopic.php?f=15&t=5233

#10

Status:fixed» closed

Automatically closed -- issue fixed for 2 weeks with no activity.