Well, I guess I would be the author of that ugly code.  ;-)

Looking at the code, it looks like I first wrote a special case for anything
with the sample count less than two (the second "if").  Then later I decided
to consolidate the code, so I wrote the first "if", which in effect converts
the special case into a normal case.  This makes code maintenance easier,
because it generalizes all cases into one and removes a whole block of code.
However, then I never removed that block of code.  At least that's my guess
as to what I was thinking... that was a long time ago and I think I did most
of that coding at 2:00am.

Regarding the attenuation... I, unfortunately, didn't know much about the
attenuation algorithm when I wrote that code.  And, to be honest, I still
don't know much about it.  My goal was to try to attenuate the light on a
per-sample basis, with each sample using the optical depth accumulated from
earlier samples (which we should be able to do because we're gathering the
samples in sequential order).  I think your last comment, about attenuating
by ODResult may be correct.  Another possibility is that we should be
dividing by "j", which is the number of samples gathered so far in the loop.

Looking at it all these years later, the only think I know for sure is that
this is ugly code in high need of refactoring and better commenting!

Maybe it would be worthwhile to create some POV scenes that would test this.
You can try different algorithms to see which produces the correct result.
If/when you manage to find the correct algorithm, please post your
recommendations here so that we can incorporate the fix into the official
version of POV.  Thanks!

-Nathan Kopp



"Daniel Hulme" <pho### [at] isticorg> wrote...
> Looking through the code further, I see the following piece of fun in
> media.cpp around line 580:
> >>>
>       if (sampleCount < 2)
>         sampleCount = 2;
>
>       /* set static sample count */
>       sampCount_s = sampleCount - 1;
>
>       if (sampleCount < 2)
>       {
> <<<
>
> Note that the second sampleCount<2 test can never succeed. I assume this
> second test and the associated block of code are optimised away by the
> compiler, but does this not give a warning? Clearly one of the tests
> should not be there: either the first because it stops the
> three-samples-only special case code running, or the second because it
> is superfluous. Can some explain a bit further please?
>
> A bit lower down, line 635, I see:
> >>>
> /* do some attenuation, too, since we are doing samples in order */
> Media_Interval[i].te[0]+=Result[0] *
> exp(-Media_Interval[i].od[0]/sampleCount);
> /* should this be sampleCount+1? */
> Media_Interval[i].te[1]+=Result[1] *
> exp(-Media_Interval[i].od[1]/sampleCount);
> Media_Interval[i].te[2]+=Result[2] *
> exp(-Media_Interval[i].od[2]/sampleCount);
> <<<
> Note particularly the comment. Though I haven't finished studying the
> media code yet, I strongly suspect that it should not be sampleCount+1,
> because although we are doing sampleCount+1 samples in the interval,
> there are only sampleCount gaps. What I don't yet get, though, is that
> Media_Interval[i].od[] holds the cumulative optical density - it is
> added to on each sample by the ODResult variable coming from the
> sample_media_rec call. If Media_Interval[i].od[] holds the entire
> optical density so far along this interval, why are we dividing it by
> the number of gaps? Surely that would just attenuate by the average
> optical density? Would it not be better to not divide it and attenuate
> by the whole optical density? If it needs to be a per-sample thing, why
> not use the optical density from this sample, held in ODResult?
>
> Perhaps my further exploration will help, but if anyone who knows about
> this is out there perhaps they would consider tossing a hint my way.
>
> Thanks,
> Daniel
>
> --
> A most peculiar man    With the windows closed      And Mrs Reardon says
> He died last Saturday  So he'd never wake up  He has a brother somewhere
> He turned on the gas   To his silent world   Who should be notified soon
> And he went to sleep   And his tiny room    .oO( http://sad.istic.org/ )

Post a reply to this message