Skip to content
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

Feature: Temporal Resolving #241

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Conversation

0beqz
Copy link

@0beqz 0beqz commented Jul 28, 2022

About

This PR implements temporal resolving to preserve samples and reduce noise on camera movement. It does so by calculating the velocity of each pixel each frame and stores it in a velocity buffer. In the TemporalResolvePass it's using the velocity to find out where a pixel was in the last frame and projects that pixel onto the new pixel if possible. It's using the velocity buffer, the current frame's depth buffer and the last frame's depth buffer to reduce artifacts by checking which pixels can't have the last frame reprojected onto them (e.g. because they were occluded in the last frame).

Related issue: #60

Comparisons

Temporal Resolving first enabled and then disabled:

vid1.webm

How good TR looks can depend on the scene, so scenes with complex geometry and intense specular reflections are more sophisticating for TR and won't usually look as smooth as scenes with simple geometry and rough materials.
So the scene from the video works really well for TR for example.

Usage

The API would look like this:

import { TemporalResolve } from 'three-gpu-pathtracer'

const temporalResolve = new TemporalResolve( ptRenderer, scene, camera );

// the in the render loop
renderer.autoClear = false;
temporalResolve.update();
fsQuad.material.map = temporalResolve.target.texture;
renderer.autoClear = true;

The parameters to tweak the TemporalResolve class are documented in the readme.

Demos

Online Demo

Currently, the two demos example/index.js and example/materialBall.js include Temporal Resolving.

Credits

The VelocityShader.js file is from threejs-sandbox: https://github.com/gkjohnson/threejs-sandbox/blob/master/shader-replacement/src/passes/VelocityShader.js

References

https://media.contentapi.ea.com/content/dam/ea/seed/presentations/dd18-seed-raytracing-in-hybrid-real-time-rendering.pdf (Raytracing in hybrid real-time rendering)
https://github.com/0beqz/screen-space-reflections#temporal-reprojection (mainly articles about TRAA)

@bhouston
Copy link
Contributor

This is amazing work @0beqz! So exciting to see it actually work!

BTW I have a similar bounty over at Three.js, which may be easier for you to do now - mrdoob/three.js#14050

@gkjohnson
Copy link
Owner

Agreed! This is really great.

For formatting if you run npm run lint -- --fix it should fix most of the linter issues automatically

@0beqz
Copy link
Author

0beqz commented Jul 29, 2022

Glad to hear!

Looks like I formatted all files with the wrong config by accident in a certain commit. The formatting should be correct now in the recent commit.

@gkjohnson
Copy link
Owner

I still need to dig into the code more but I have a couple thoughts on the tiling behavior. At the moment when there's more than one "tile" that renders the TRAA only applies to the first tile during camera changes. I think one of the really nice capabilities of this would be that even when moving the camera with tiled rendering that the rest of screen does not revert to the background or rasterized rendering.

And then the tiling could also continue to increment throughout the camera changes instead of just rerendering the top left corner.

What are your thoughts?

Also:

The VelocityShader.js file is from threejs-sandbox: https://github.com/gkjohnson/threejs-sandbox/blob/master/shader-replacement/src/passes/VelocityShader.js

🎉 😁

I'm excited to see how this works with animated geometry, too.

@0beqz
Copy link
Author

0beqz commented Aug 1, 2022

Well so one problem for TR with tiled rendering is that when you keep moving the camera constantly then it will happen that all the tiles (besides the top-left tile) won't be rendered for a lot of frames.
So let's say you have tiling enabled in the materialBall scene and then keep moving the camera constantly all around the ball in the scene for a span of multiple seconds (so that it always has to resample each frame). Since none of the tiles besides the top-left one will be rendered during that time we can't use TR anymore otherwise it will just smear a lot since the only info TR has is the frame before you started moving the camera around.

I'm not really sure what would be the best solution here.

@gkjohnson
Copy link
Owner

gkjohnson commented Aug 2, 2022

Well so one problem for TR with tiled rendering is that when you keep moving the camera constantly then it will happen that all the tiles (besides the top-left tile) won't be rendered for a lot of frames.

Right this is what I was alluding to. We can handle this bit in another PR but I think a good solution is to not just render the first tile when the path tracer has been reset. When TRAA is enabled we'd want to step through all the tiles to render so the reset of the scene doesn't just degrade into a blurry mess. Then you can render 1/4 of the screen per frame while sill having a coherent path traced view.

For this PR, though, I think it would be okay if the rest of the scene other than the top left corner became blurry.

@0beqz
Copy link
Author

0beqz commented Aug 3, 2022

Ah I see, yeah that sounds like the best solution. I will add another commit where it implements that so that it works better with tiling.

@0beqz
Copy link
Author

0beqz commented Aug 5, 2022

Alright, I've updated TR:

  • made it re-render the pathtracer till samples >= 1 (so that all tiles will be rendered first before we do TR), now it always displays the pathtraced output when using tiling
  • Added dilation when fetching velocity to get rid of aliasing and flickering at edges (idea from here)
  • re-worked the TR blending algorithm so that it's less smeary now (especially when there is a lot of movement)
  • added weightTransform property (based on "Intensity / Color Weighing" from here) to reduce noise on camera movement)

@N8python
Copy link

N8python commented Aug 5, 2022

I literally shouted with joy when I first read this PR. Insane work.

@gkjohnson
Copy link
Owner

I'm slowly taking a look at this - it's gonna take me a little bit to get my head around it. I'm making a PR to make into your branch with some code style changes to make it more consistent with the rest of the project as I go. One thing I noticed is that performance seems to be worse when using tiled rendering with temporal resolve than when rerendering the full frame. Is this something you've noticed, as well?

@0beqz
Copy link
Author

0beqz commented Aug 8, 2022

I see, well yeah makes sense. This line here will be the reason:

while ( this.ptRenderer.samples < 1 ) this.ptRenderer.update();

Right now TR will keep re-rendering the pathtracer till all tiles are rendered to prevent smearing. That's probably more expensive than just rendering a single full frame. I think the optimal solution would be that, if TR is enabled, we don't use tiling for the very first sample and then just re-enable tiling afterwards. To do that I'll have to change the structure of the TR pass so that it calls ptRenderer.update() now and changes the ptRenderer's tiling based on if it's the first sample (right now it doesn't change the ptRenderer and only uses its ouput textures). So then you'll no longer need to call both ptRenderer.update() and temporalResolve.update() when using TR.
So what do you think if I should change that?

@gkjohnson
Copy link
Owner

I see, well yeah makes sense. This line here will be the reason:

while ( this.ptRenderer.samples < 1 ) this.ptRenderer.update();

Got it thanks -- that seems to be it. I'm surprised rendering the tiles separately has such a significant performance impact but that seems to be the issue.

Why don't we remove the while loop understanding that it's not going to look right. I think it should be possible to tile rendering over multiple frames with this temporal resolve and have it look nice but I'm having trouble articulating the plan.

Here's what I'm seeing right now when removing that line - the other tiles render as black in the temporal resolve. Is it possible to have them retain the latest value in the TRAA texture? I know it'll be a blurry mess but it's a first step. Once I figure that out I'll toy around with the other options I'm imagining.

traa-material-ball.mov

I'm also seeing that the TRAA texture seems to have the wrong aspect ration once the screen has been resized. You can see some of the hotspots in the texture look stretched when making the window particularly narrow and it started off at a normal aspect ratio.

And lastly I've made 0beqz#1 with some style changes to match the project while I was taking a look at things!

@0beqz
Copy link
Author

0beqz commented Aug 10, 2022

Yeah that's possible, you can detect if a pixel of the PT's buffer was not rendered yet in the shader through the alpha value (it will be 0 until the tile of the current texel is rendered).
And I think your PR would be a great solution here, I'll check how it looks with TR. But it will definitely reduce smearing with that solution.
One issue would be that if you have a lot of tiles to be rendered e.g. 4x4 tiles then it will result in smearing again since it still takes many frames to have a specific tile rendered.

How's the performance difference between rendering a single tile and rendering the entire frame? I was thinking that we could always render the full frame on the first sample when TR is enabled (even when we use tiling). This should solve the smearing problem. Afterwards we'd resume with tiled rendering.

And thanks for making the PR. Are you using any formatter for the in-line glsl code btw? Looking for one currently

@gkjohnson
Copy link
Owner

gkjohnson commented Aug 10, 2022

One issue would be that if you have a lot of tiles to be rendered e.g. 4x4 tiles then it will result in smearing again since it still takes many frames to have a specific tile rendered.

I think that's okay. That's just a consequence of using tiling - you're trading performance and quality.

And thanks for making the PR. Are you using any formatter for the in-line glsl code btw? Looking for one currently

No... I would really like one too but it doesn't look like anyones made one yet

@0beqz
Copy link
Author

0beqz commented Aug 11, 2022

Alright, I've fixed the issue with resizing and improved tiling so that it's no longer re-rendering the pathtracer. Also improved a few other things like making box-blur depth-aware to reduce smearing.

I've noticed an issue when you have set stableTiles = false and either tiles.x or tiles.y is 1 (while the other isn't). It won't render a certain tile at all then.

No... I would really like one too but it doesn't look like anyones made one yet

I haven't checked yet but does the build tool support importing external .glsl / .vert / .frag files into JS files? If so you can use clang-format to format these (as C code), works pretty well for formatting

@gkjohnson
Copy link
Owner

I've noticed an issue when you have set stableTiles = false and either tiles.x or tiles.y is 1 (while the other isn't). It won't render a certain tile at all then.

Thanks! I just pushed a fix. But I think this all looks pretty awesome! It feels pretty good with the incrementing tiles, too.

I haven't checked yet but does the build tool support importing external .glsl / .vert / .frag files into JS files? If so you can use clang-format to format these (as C code), works pretty well for formatting

That's something to think about - I hadn't considered using clang.

I'll do one more pass at this in a bit and then I think we can merge it if it all looks good!

src/temporal-resolve/TemporalResolve.js Outdated Show resolved Hide resolved
src/temporal-resolve/TemporalResolve.js Outdated Show resolved Hide resolved

### .temporalResolveMix

```javascript
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we use js instead of javascript for the syntax keyword

Comment on lines -135 to +141
perspectiveCamera.position.set( - 4, 2, 3 );
perspectiveCamera.position.set( - 4, 2, 7 );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we revert this camera change unless there's a reason to move it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll change it back

Comment on lines +155 to +159
temporalResolve.temporalResolveMix = 0.9;
temporalResolve.clampRadius = 1;
temporalResolve.newSamplesSmoothing = 0.5;
temporalResolve.newSamplesCorrection = 0.75;
temporalResolve.weightTransform = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be best to initialize these parameters from the params object - otherwise if we change the demo defaults we have to remember to do it in two places.

Comment on lines +177 to +181
temporalResolve.temporalResolveMix = 0.9;
temporalResolve.clampRadius = 2;
temporalResolve.newSamplesSmoothing = 0.675;
temporalResolve.newSamplesCorrection = 1;
temporalResolve.weightTransform = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above - lets initialize from the params object fields.

Comment on lines +30 to +31
typeof window !== 'undefined' ? window.innerWidth : 2000,
typeof window !== 'undefined' ? window.innerHeight : 1000,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this to initialize to a width and height of 1, 1

Comment on lines +98 to +108
#ifdef DILATION
vec4 velocity = getDilatedTexture( velocityTexture, vUv, pxSize );

vec2 velUv = velocity.xy;
vec2 reprojectedUv = vUv - velUv;
#else
vec4 velocity = textureLod( velocityTexture, vUv, 0.0 );

vec2 velUv = velocity.xy;
vec2 reprojectedUv = vUv - velUv;
#endif
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: looks like there's some inconsistent indentation in this shader chunk.

Comment on lines +27 to +28
typeof window !== 'undefined' ? window.innerWidth : 2000,
typeof window !== 'undefined' ? window.innerHeight : 1000,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we initialize these values to 1, 1

clampRadius = 1 : Number
```

An integer to set the radius of pixels to be used for neighborhood clamping. Higher values will result in a less noisy look at the cost of more blurring.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "neighborhood clamping" mean here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You clamp the color of the reprojected pixel by the min and max color of the neighboring pixels from the raytracer's output at the current pixel. This is the most popular method to reduce ghosting for TRAA but since we have a noisy output in the first frames when raytracing, neighborhood clamping will lead to flickering there. So by increasing the number of pixel we use for neighborhood clamping, we reduce flickering but will have more smear.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! Maybe we can include more information on the docs about the role color plays in this process

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, yeah I'll add a more in-depth explanation for it

@gkjohnson
Copy link
Owner

Also not sure if you have any thoughts on these issues - maybe there's a quick fix? I noticed them in the main index.html demo.

There's some flickering in the background which I assume is because we can't get a "velocity" or before / after pixel for the background samples?

flashing.mov

And there's a lot more background ghosting than I expected on the trees in the Japanese Bridge model. Any thoughts on what's causing that?

ghosting.mov

And of course the transparent floor causes issues but that's expected. Maybe in the future we can use something like dithering in the velocity pass so it's not quite as much an issue.

@0beqz
Copy link
Author

0beqz commented Aug 12, 2022

I'm not sure what's causing the flickering of the background there. Is there anything different in this scene compared to the materialBall.html scene? I'm wondering as the background has no flickering there even with tiling enabled.

Yeah currently TR doesn't work well with objects that have thin geometry like also the Rover for example. You should play around with the settings (temporalResolveMix, clampRadius,...) to reduce smearing. They are somewhat dependant on the scene so in the materialBall you can use 'less aggressive' correcting since the geometry is less complex. But I'll look into it and see if I can improve how correction for disoccluded pixels is handled.

@gkjohnson
Copy link
Owner

I'll have to dig into the flickering more but my intuition without looking deeper is the the velocity / depth texture don't have values for the background image which means the path traced pixels don't get reprojected and the rasterized render shows through? The environment intensity is higher in the index.html file which is why the pathtraced background might look different from the rasterized one.

I would have imagined that it would be possible to improve the ghosting more but your head has been in this more than me. One thing to keep in mind is that the pathtraced renderer is using antialiasing whereas the TRAA depth and velocity passes do not so there will be at least a small amount of misalignment of color on reprojection. So I did expect some ghosting but maybe not as much as in that video 🤔

@0beqz
Copy link
Author

0beqz commented Aug 13, 2022

I'll have to dig into the flickering more but my intuition without looking deeper is the the velocity / depth texture don't have values for the background image which means the path traced pixels don't get reprojected and the rasterized render shows through? The environment intensity is higher in the index.html file which is why the pathtraced background might look different from the rasterized one.

Yeah so I detect if the current texel is from the background by checking if the alpha channel of the velocity texel is 1 (see here: https://github.com/0beqz/three-gpu-pathtracer/blob/temporal-resolve/src/temporal-resolve/materials/shaders/temporalResolveFragment.js#L113). In the VelocityPass, I set the scene's background alpha to 1 and set the alpha of the gl_FragColor to 0 (https://github.com/0beqz/three-gpu-pathtracer/blob/temporal-resolve/src/temporal-resolve/materials/VelocityShader.js#L111). This way I can detect if a texel wasn't rendered in the VelocityPass which should imply it's from the background. So I'm not sure why it isn't detecting the background in your demo. I'll have to look into it.

I would have imagined that it would be possible to improve the ghosting more but your head has been in this more than me. One thing to keep in mind is that the pathtraced renderer is using antialiasing whereas the TRAA depth and velocity passes do not so there will be at least a small amount of misalignment of color on reprojection. So I did expect some ghosting but maybe not as much as in that video 🤔

Well I I will re-work the algorithm and switch from detecting disocclusions through depth to detecting them through velocity since that seems to be more stable I noticed in other passes where I use TR.
To reduce smearing, just reduce temporalResolveMix for such scenes with a lot of smearing.
But yeah it looks like the blending part needs some re-work to deal more aggressively with disocclusions.
I'm not using AA through e.g. multisampling for the velocity and depth buffer but I use dilation in the shader to deal with flickering at edges and as a way to reduce aliasing for them.

@gkjohnson
Copy link
Owner

gkjohnson commented Aug 17, 2022

Well I I will re-work the algorithm and switch from detecting disocclusions through depth to detecting them through velocity since that seems to be more stable I noticed in other passes where I use TR.

Depth seems like a good way to handle this, as well, but it looks like its taking the foreground object color when an object is occluded instead marking it as black / no data like I'd expect. There will definitely wind up being some kind of discontinuity it's just a matter of how we handle it.

If you have some thoughts on or see how to immediately improve this that would be great otherwise we can also add a few issues to track these and improve the behavior in the future. The flashing background would be the bigger priority for me first.

@0beqz
Copy link
Author

0beqz commented Aug 21, 2022

Depth seems like a good way to handle this, as well, but it looks like its taking the foreground object color when an object is occluded instead marking it as black / no data like I'd expect. There will definitely wind up being some kind of discontinuity it's just a matter of how we handle it.

I think the main reason for that issue was that I hadn't found a proper way to detect disocclusions yet. I switched from comparing the current and the reprojected world positions to just comparing the two depth values to find disocclusions. This is more stable now and also removes the need for neighborhood clamping that doesn't work too well for this purpose. I'll add that soon to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants