-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Direct light sampling #37
Comments
- Random number generator now produces enough different sequences to cover 500 frames of reference mode accumulation; - Hemisphere sampling for indirect diffuse tuned to make the results better match the cosine-weighted sampling in reference mode; - Direct diffuse lighting is now correctly divided by PI and matches same lighting computed by random sampling of emissive surfaces; - Spotlight term removed from sky polygons and added to other non-analytic emissive surfaces; - Suspicious 0.5 term removed from sky polygons. Also the `pt_direct_polygon_lights` cvar has a new meaning when set to -1: all polygon lights are sampled through the indirect lighting shader, for comparison purposes. Inspired by #37
Thank you for reporting this issue! |
On line 180 of indirect_lighting.rgen, is_analytic_light shouldn't have the With the asvgf_seed_rng.comp change, frame x checkerboard 1 will have the same rng as frame (x+1) checkerboard 0, which I think will produce duplicate samples when the scene is stationary. To avoid this issue, it should be |
…on-default settings for `pt_direct_polygon_lights` and `pt_indirect_polygon_lights`. #37
Makes sense - fixed but with slightly different interpretation of The RNG thing is not really a problem because although one checkerboard field will use the same sequence as the other on the previous frame, they process different sets of pixels, so overall there is no undersampling (I think). I don't see any image quality difference in real-time rendering mode, and this new behavior is definitely better for the reference accumulation mode because it generates 512 unique sequences over time instead of just 256. |
I'm not 100% sure here but i've been implementing recent changes in my fork and i think i see a definitive increase in temporal ghosting in indoor areas: Of course i might've screwed something on my part so it's worth checking out on your end. |
Yes, with the new changes materials behave slightly differently, and you see more specular reflections. And the specular denoiser is not great here - just a temporal filter. The ghosting was not so visible before because the signal was just too dim compared to diffuse lighting. I'll try to come up with a solution for the ghosting / noise, and some materials need roughness tweaking too. You should probably not merge these changes until it's resolved. |
Yeah that seems to be the case. |
Are you sure? The two checkerboard fields alternate between odd and even pixels on a frame by frame basis, so I would think there would be overlap. For instance, if my understanding is correct frame 0 checkerboard 0 position (0, 0) would correspond to pixel (0, 0) in the full frame while frame 0 checkerboard 1 position (0, 0) would correspond to pixel (1, 0), but frame 1 checkerboard 0 position (0, 0) would correspond to pixel (1, 0) and frame 1 checkerboard 1 position (0, 0) would correspond to pixel (0, 0). EDIT: In my local fork, I fixed the issue of getting unique rng by keeping lines 55 and 56 unchanged but changing lines 53 and 54 to:
For a while now I've actually had the reference mode accumulating 8192 samples, and it seems to work fine. |
- Added cvar `pt_accumulation_rendering_framenum` to control how many frames to accumulate; - Added offsets to the RNG seed X and Y components to avoid repeating the random sequence after 512 frames. Inspired by SkacikPL@63bcfe2 and #37
Yes they alternate, but the RNG seed was computed without the alternation... For accumulation rendering, I implemented a version of your coordinate adjustment but with a change to remove the obvious diagonal noise patterns that appeared after a couple thousand frames. Thanks again! |
Yeah, i just checked it out and it is better. Not exactly "perfectly playable" but also not entirely unplayable like it used to be. I'm also glad some of my ideas managed to get on board too. On a semi related note, i was experimenting with automating accumulation rendering for demos to achieve a prerendering functionality. I'm not sure how useful it would be to a general user base but the concept isn't hard to implement.
Then in
And set client time to tick only when unpaused
Lastly in
This ensures fixed time step if demo is played with in vkpt\main.c i added
And my entire
About half of that are dirty hacks which can probably be done much better but it gets the job done. Aside that, it's pretty straightforward - just record any demo and play it back while |
That's true, but it only matters for resolutions for which (resX / 2) % 256 != 0, due to the % BLUE_NOISE_RES. Consider again the case of position (0, 0) checkerboard 0 vs position (0, 0) checkerboard 1. In the rng seed texture, the former maps to position (0, 0) while the latter maps to position (resX / 2, 0). Say the resolution is 2560x1440. Then (ipos.x % BLUE_NOISE_RES) will be 0 for the former case, but also 0 for the latter case (2560/2 % 256 == 0). |
A couple of things with the direct light calculations in get_direct_illumination and get_sunlight in path_tracer_rgen.h:
In both functions, the diffuse term is (correctly) multiplied by NdotL. However, since this is done before multiplication by the specular term, the latter ends up multiplied by that cosine as well, which I don't think is correct. I realized this after noticing that specular highlights on surfaces with roughness <= pt_direct_roughness_threshold - 0.02 were disproportionately brighter than those on surfaces with roughness >= pt_direct_roughness_threshold + 0.02. Changing the code so NdotL only affects diffuse but not specular corrects this inconsistency.
I'm less sure about this, but shouldn't the diffuse (but not the specular of course) term in each function be divided by pi (lambertian scale factor)?
The text was updated successfully, but these errors were encountered: