Buffer API and numberOfSamples

[guibou]

Hi. I have an issue with SPPM I'd like to fix, it is the way buffer are normalized in Lux.

In SPPM I have two kind of contributions:

a) The contribution from the eyepass, it should be normalized per pixel
b) The contribution from the photonsampler. It should be normalized per number of samples (ie, photon)

My issue is that any contribution from eyepass and photonpass are taken into account for the normalization (because buffers share their normalization value. This is an issue because it means that the contribution from the photon pass is too much normalized)

It currently works on SPPM because of an ugly cheat and because We are storing in the SPPMRenderer some value which needs to be restored for film restart (or network rendering). Solving this issue will make the SPPM code cleaner and helps network rendering and film restart.

So I want to be able to normalize buffer only by the number of contribution associated to the buffer.

I have a working patch (I may commit it in a separate branch soon), but I have some question I wanted to discuss with you first :

- a) Is there a need for that sharing of number of sample ? There is only one other integrator which use more than one buffer, it is the bidirectional one. jeanphi, can you confirm me that this sharing of number of sample is used in the bidirectional, or this feature is not used ? (It will dramatically improve the code if not, but by the way, I currently have a code which work for both)
- b) API design. Currently we add a sample with AddSample. The default implementation is:

void Sampler::AddSample(const Sample &sample)
{
sample.contribBuffer->AddSampleCount(1.f);
for (u_int i = 0; i < sample.contributions.size(); ++i)
sample.contribBuffer->Add(sample.contributions[i], 1.f);
sample.contributions.clear();
}

I have currently change the AddSampleCount method to take a second argument, buffer_id. I'm planning on using -1 as a wilcard (ie, every buffer) and 0..N as a buffer number. And using AddSample(sample, buffer_id).

So for me in sppm it means that a sample from the eye should be added as AddSample(sample, eye_buffer_id), and a sample from the photon pass should be added as AddSample(sample, photon_buffer_id).

But I'm wondering, the contribution already stores the indice of the associated buffer, so two use cases:

a) We suppose that one day we'll need to add a sample with contribution to different buffer, but only changing the count of one of them ?
b) We suppose the same think, but we want to be able to change the count of just some buffer at once (so AddSampleCount should take a vector as parameter)
c) It will never happen and we can refactor Contribution to remove the buffer field which may earn some memory (not a lot I guess) at a cost of a really hard refactoring.

Ideas ?

[jeanphi]

Hi,

Actually bidir has the same requirements: per pixel normalisation of the eye buffer and per screen normalization of the light buffer.
When using the per pixel normalization, you don't use the number of samples at all, just the per pixel weight sum of the buffer.
The issue with SPPM is that you have 2 samplers, one for the eye buffer and one for the light buffer. So you can either use special samplers, that will call AddSampleCount appropriately (ie don't call it for the eye sampler, but call it with the proper weight value for the photon sampler), or add a weight value to the sample (initialized to a value of 1 by default) and use that value instead of the hard coded 1 when calling AddSampleCount in the samplers (you would then set it to 0 for the eye sample and an appropriate value for the photon sample).

That'd be much less intrusive and give a proper solution to your needs, what do you think?

Jeanphi

[guibou]

When using the per pixel normalization, you don't use the number of samples at all, just the per pixel weight sum of the buffer.
The issue with SPPM is that you have 2 samplers, one for the eye buffer and one for the light buffer. So you can either use special samplers, that will call AddSampleCount appropriately (ie don't call it for the eye sampler, but call it with the proper weight value for the photon sampler), or add a weight value to the sample (initialized to a value of 1 by default) and use that value instead of the hard coded 1 when calling AddSampleCount in the samplers (you would then set it to 0 for the eye sample and an appropriate value for the photon sample).

I agree with you and in fact it is what is actually done in SPPM.

Issue is haltspp does not work at all with this setup.

User want to set haltspp as the number of passes, but the number of passes is stored in SPPMRenderer so currently we have to use a custom code (or the ugly hack I wrote twos days ago) to get an unified halt spp between all integrators. I'd like to be able to set the api to check for eye buffer for haltSPP will keeping good normalization for photon buffer.

Also I'd like to remove the number of passes stored in SPPMRenderer and use in place the number of samples stored in the eye buffer, because it will allow network rendering and film merging/resuming.

The other solution is to allow storing custom attributes to the film, but may change the binary film format and breaks years of compatibility.

[Lord Crc]

User want to set haltspp as the number of passes, but the number of passes is stored in SPPMRenderer so currently we have to use a custom code (or the ugly hack I wrote twos days ago) to get an unified halt spp between all integrators.

I guess the core issue is that the "haltspp" condition is actually something that should be checked by the renderer and not the film. Perhaps we could modify the halt logic. After all the sampler renderer should know how many samples it's added to the film...

[guibou]

User want to set haltspp as the number of passes, but the number of passes is stored in SPPMRenderer so currently we have to use a custom code (or the ugly hack I wrote twos days ago) to get an unified halt spp between all integrators.

I guess the core issue is that the "haltspp" condition is actually something that should be checked by the renderer and not the film. Perhaps we could modify the halt logic. After all the sampler renderer should know how many samples it's added to the film...

Agree. But that does not solve the issue of "storing the number of passes and photon in the film".

[jeanphi]

Hi,

Checking for haltspp in the renderer will break network rendering or restoring behaviour.

Guibou, your current scheme is not that bad. You could optimize it a bit by having the special buffer type store a float pointer instead of a float and a scale factor accessor, and let the SPPM renderer hold the float and update it when needed. This could actually be useful for the ERPT sampler if it is kept.
If you want to clean up the interface, an optional (default value to NULL) parameter could be added to Film::RequestBuffer so that you don't need those ugly casts (it might then be easier to store the float in the SPPM surface integrator).
Another way would be to add that scale parameter to the BufferGroup itself (like the number of samples), and modify AddSampleCount behaviour so that it adds 1 to the sample count every time it is called and adds the provided value to the scale parameter, and have a new Film function member AddScale or something like that that would add its parameter to the scale but not update the number of samples. This would allow easy saving and restoring of those values with the film.

What do you think?

Jeanphi

[guibou]

Guibou, your current scheme is not that bad. You could optimize it a bit by having the special buffer type store a float pointer instead of a float and a scale factor accessor, and let the SPPM renderer hold the float and update it when needed. This could actually be useful for the ERPT sampler if it is kept.

I'm not too much in favor of a float pointer handled by the Renderer, because it means that we'll need a mean of storing this value in the film later for network/restore.

If you want to clean up the interface, an optional (default value to NULL) parameter could be added to Film::RequestBuffer so that you don't need those ugly casts (it might then be easier to store the float in the SPPM surface integrator).
Another way would be to add that scale parameter to the BufferGroup itself (like the number of samples), and modify AddSampleCount behaviour so that it adds 1 to the sample count every time it is called and adds the provided value to the scale parameter, and have a new Film function member AddScale or something like that that would add its parameter to the scale but not update the number of samples. This would allow easy saving and restoring of those values with the film.

I'm not sure I fully got your point.

The more I'm thinking of it and the more I'm pretty sure we'll have to modify the film binary file format to handle this, and I'm not happy with this.

[jeanphi]

If you want to clean up the interface, an optional (default value to NULL) parameter could be added to Film::RequestBuffer so that you don't need those ugly casts (it might then be easier to store the float in the SPPM surface integrator).
Another way would be to add that scale parameter to the BufferGroup itself (like the number of samples), and modify AddSampleCount behaviour so that it adds 1 to the sample count every time it is called and adds the provided value to the scale parameter, and have a new Film function member AddScale or something like that that would add its parameter to the scale but not update the number of samples. This would allow easy saving and restoring of those values with the film.

I'm not sure I fully got your point.

The more I'm thinking of it and the more I'm pretty sure we'll have to modify the film binary file format to handle this, and I'm not happy with this.

What you need is a weighting factor to normalize some buffers, so add it the the buffer group and provide the relevant accessors to update it. I was just suggesting a scheme that would allow some reuse of the existing interfaces.
Regarding the film binary format, that's life, and that's why we have a version numbering for the file format. I'd even say better do it before v1.0 final since that's a major version jump and users will more easily understand that there are some incompatibilities.

Jeanphi