Permalink
Browse files

IMPORTANT: Path/Photon OneDirectLight - attempt to sample lights more…

… uniformly

As reported in http://www.yafaray.org/node/803 there are artifacts in the path tracing and photon mapping algorithms when there is more than one light.

I found that the allegedly "random" sampling of lights in the mcintegrator.cc "estimateOneDirectLight" was not sampling all lights in the scene with the same probability. It was (incorrectly I think) sampling more often the first light, and less often the last light in the lights vector. As the result of each sample is multiplied by the number of lights, it caused the lighting to be non-uniform.

First I attempted to use a "randomized" approach to solve this, but it increased the noise (not too much, but it was noticeable). So I decided to try to improve the sampling using the current Halton sampling algorithm. I found that the input to the algorithm was not correlative enough, having many "jumps" that probably caused the wrong Halton outputs. Therefore I decided to change the input data and use an almost purely correlative numbering for each sample (having separate correlative numbers, one per thread). This seems to improve the noise and still give similar results no matter how many lights are used, and a more uniform light sampling.

This is a very significant change. I would expect scenes to be a little bit noisier now, but having a much more correct lighting when there is more than one light in the scene.

In any case we need to keep an eye on this and perhaps fine tune it more or even revert it back completely and look for another solution. For now I will leave it this way and see what happens...
  • Loading branch information...
DavidBluecame committed Mar 11, 2017
1 parent 4e1252f commit 11551b779ce9234a348d6aefa56af2289bd76d97
Showing with 12 additions and 3 deletions.
  1. +1 −1 include/core_api/tiledintegrator.h
  2. +7 −0 src/yafraycore/integrator.cc
  3. +4 −2 src/yafraycore/mcintegrator.cc
@@ -53,7 +53,7 @@ class YAFRAYCORE_EXPORT tiledIntegrator_t: public surfaceIntegrator_t
float maxDepth; //!< Inverse of max depth from camera within the scene boundaries
float minDepth; //!< Distance between camera and the closest object on the scene
bool diffRaysEnabled; //!< Differential rays enabled/disabled - for future motion blur / interference features
-
+ static std::vector<int> correlativeSampleNumber; //!< Used to sample lights more uniformly when using estimateOneDirectLight
};
struct threadControl_t
@@ -42,6 +42,9 @@
__BEGIN_YAFRAY
+
+std::vector<int> tiledIntegrator_t::correlativeSampleNumber(0);
+
void tiledIntegrator_t::renderWorker(int mNumView, tiledIntegrator_t *integrator, scene_t *scene, imageFilm_t *imageFilm, threadControl_t *control, int threadID, int samples, int offset, bool adaptive, int AA_pass)
{
renderArea_t a;
@@ -186,6 +189,10 @@ bool tiledIntegrator_t::render(int numView, imageFilm_t *image)
preRender();
+ correlativeSampleNumber.clear();
+ correlativeSampleNumber.resize(scene->getNumThreads());
+ std::fill(correlativeSampleNumber.begin(), correlativeSampleNumber.end(), 0);
+
int acumAASamples = AA_samples;
if(session.renderResumed())
@@ -29,6 +29,7 @@
#include <utilities/mcqmc.h>
#include <core_api/renderpasses.h>
#include <core_api/camera.h>
+#include <core_api/imagefilm.h>
#ifdef __clang__
#define inline // aka inline removal
@@ -61,12 +62,13 @@ inline color_t mcIntegrator_t::estimateOneDirectLight(renderState_t &state, cons
if(lightNum == 0) return color_t(0.f); //??? if you get this far the lights must be >= 1 but, what the hell... :)
Halton hal2(2);
- hal2.setStart(n-1);
+ hal2.setStart(imageFilm->getBaseSamplingOffset() + correlativeSampleNumber[state.threadID]-1); //Probably with this change the parameter "n" is no longer necessary, but I will keep it just in case I have to revert back this change!
int lnum = std::min((int)(hal2.getNext() * (float)lightNum), lightNum - 1);
+ ++correlativeSampleNumber[state.threadID];
+
return doLightEstimation(state, lights[lnum], sp, wo, lnum, colorPasses) * lightNum;
- //return col * nLights;
}
inline color_t mcIntegrator_t::doLightEstimation(renderState_t &state, light_t *light, const surfacePoint_t &sp, const vector3d_t &wo, const unsigned int &loffs, colorPasses_t &colorPasses) const

1 comment on commit 11551b7

@DavidBluecame

This comment has been minimized.

Show comment
Hide comment
@DavidBluecame

DavidBluecame Mar 11, 2017

Member

Just a comment: I know that this "correlativeSampleNumber" vector is actually not thread safe. Therefore it's quite probable that the values there are not purely correlative and there could be some repeated and missing values.

However, for speed and simplicity reasons I decided to avoid using mutexes, etc, as atomics were not working well for me.

Despite this fact, I believe that the fix still works well enough. However maybe in the future we should make this a thread safe variable with proper mutexes or atomic operations.

Member

DavidBluecame commented on 11551b7 Mar 11, 2017

Just a comment: I know that this "correlativeSampleNumber" vector is actually not thread safe. Therefore it's quite probable that the values there are not purely correlative and there could be some repeated and missing values.

However, for speed and simplicity reasons I decided to avoid using mutexes, etc, as atomics were not working well for me.

Despite this fact, I believe that the fix still works well enough. However maybe in the future we should make this a thread safe variable with proper mutexes or atomic operations.

Please sign in to comment.