ERPT fix

[Lord Crc]

Hi,

I'm about to push a crash fix for ERPT which was due to how ERPT had a base sampler, and both tried to use Sample::samplerData for their own data structure.

I don't think my fix is very elegant, but was the least invasive way I could see. Feel free to revert it or otherwise.

edit: I also moved the AddSampleCount(), it didn't seem right the place it was... is it wrong? Before it could render for several minutes and only reach 0.03S/px, now it's more in line with metrosampler.

[jeanphi]

Hi,

The AddSampleCount move is wrong. It will lead to badly normalized pictures.
And I don't like your change, sorry, it imposes a modification to all sampler while it is only required for ERPT and ERPT could as well be removed. It might be best to do the following:
- in ERPTSampler::InitSample, first call baseSampler->InitSample, create the ERPTSampler data chunk, save the baseSampler data chunk pointer in the ERPTSampler data and replace the sample data pointer with the ERPTSampler data chunk pointer
- when calling a baseSampler member, first replace the sample data pointer by the baseSampler data pointer, do the call, put back the ERPTSampler data pointer

The change is then localized to ERPT, it's ugly but ERPT IS ugly.

Jeanphi

[binarycortex]

...and ERPT could as well be removed...

Please don't remove ERPT. It is a valid unbiased sampler, and imho has potential. If I understand the concept correctly, it can combine the speed of the lowdiscrepancy sampler with the path mutations of MLT.

[jeanphi]

Hi,

It has never been able to outperform MLT with the latest MLT tweaks and produces inaccurate results in its current form. My improvements to the MLT algorithm already give us much smoother results than the plain algorithm usually presented so I'm sure there's anything to gain from ERPT. At least I'm not interested in it anymore.

Jeanphi

[Lord Crc]

- when calling a baseSampler member, first replace the sample data pointer by the baseSampler data pointer, do the call, put back the ERPTSampler data pointer

We then need to make the samplerData mutable or make parameter for the GetLazySample thingy a pointer as with the other calls.

Again, I didn't say it was pretty...

[Lord Crc]

Pushed the fugly hack.

I agree that we should seriously consider retiring the erpt. Since AddSampleCount can't be moved it's not at all compatible in terms of haltspp compared to the other samplers, it has issues with fireflies (due to random/ld base sampler), and there's no significant advantage over mlt as mentioned. It also doesn't fit very well into the current framework (as witnessed just now).

[SATtva]

I agree that we should seriously consider retiring the erpt.

Is there any need to drag it into 0.8 then? I haven't seen it used for anything besides mere tests for a very long time now.

[jeanphi]

Hi,

I think we should not change the feature set now. I agree this might not make a difference, but who knows, better wait v0.9 for that.

Jeanphi

[binarycortex]

Pushed the fugly hack.

I agree that we should seriously consider retiring the erpt. Since AddSampleCount can't be moved it's not at all compatible in terms of haltspp compared to the other samplers, it has issues with fireflies (due to random/ld base sampler), and there's no significant advantage over mlt as mentioned. It also doesn't fit very well into the current framework (as witnessed just now).

Why not use metropolis as the basesampler then. Or is that a bit redundantly redundant? As a matter of opinion, I like the way it spreads the samples across the whole image.

[binarycortex]

Hi,

I think we should not change the feature set now. I agree this might not make a difference, but who knows, better wait v0.9 for that.

Jeanphi

At least now we are not shipping a broken feature set.