vkoskiv
commented
4 years ago I'm all for moving to an iterative path tracing loop. In fact, I've been intending on moving away from that recursive approach for a while now. The only benefit to the recursive approach IMO is that the code is "nifty", but that's not really a good design motivation for something like this. Agreed about it being more confusing than an iterative loop.
I don't think the existing path tracer is correct
This is very much possible. The only way I verified it when I implemented it is that I checked that both with and without RR converged to the same result. But it's been a while, so it's possible I had a rather lax definition of 'same result'.
The first commit of this PR keeps this behaviour unchanged.
This can and should be changed if you think it's incorrect. Comparing the results then determines which approach is better!
Anyway, thank you for taking the time to review this (and the previous) PR.
Thank you! I'm still struggling to put into words just how much your contributions here mean to me. It's really incredible to have someone else helping out like this after many years of mostly solo work.
madmann91
This PR implements an iterative path tracer version, which should boost performance a little. I don't think it should be merged as-is right now, because of the following reasons:
I don't think the existing path tracer is correct (but please correct me if I'm wrong): The emission is multiplied by the RR probability (line 76 of
path_trace.cin this PR, https://github.com/madmann91/c-ray/blob/2274db242b51091aab81cc7a864df42bdca11178/src/renderer/pathtrace.c#L76). This should not be the case, because the RR applies to bouncing only. Additionally, it also simplifies the code if you don't, as all control-flow paths that terminate the path can be merged. The first commit of this PR keeps this behaviour unchanged.The old function
pathTracehas been renamedpathTraceRecursive. I don't think it makes sense to keep it in the long run (because of its lower performance), unless you prefer the recursive formulation---which is IMO quite a bit more confusing than the iterative one, but I suppose it's a matter of taste really. That said, it makes sense to keep it for a bit, just to make sure the new version renders the same pictures.Anyway, thank you for taking the time to review this (and the previous) PR. I know it can take some time so there is no hurry, this can wait a bit. I'll make whatever change you need to push this forward.