-
-
Notifications
You must be signed in to change notification settings - Fork 133
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
base: main
Are you sure you want to change the base?
Conversation
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 |
Agreed! This is really great. For formatting if you run |
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. |
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:
🎉 😁 I'm excited to see how this works with animated geometry, too. |
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. I'm not really sure what would be the best solution here. |
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. |
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. |
Alright, I've updated TR:
|
I literally shouted with joy when I first read this PR. Insane work. |
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? |
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 |
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.movI'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! |
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). 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 |
I think that's okay. That's just a consequence of using tiling - you're trading performance and quality.
No... I would really like one too but it doesn't look like anyones made one yet |
Temporal resolve update
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
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 |
Thanks! I just pushed a fix. But I think this all looks pretty awesome! It feels pretty good with the incrementing tiles, too.
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! |
|
||
### .temporalResolveMix | ||
|
||
```javascript |
There was a problem hiding this comment.
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
perspectiveCamera.position.set( - 4, 2, 3 ); | ||
perspectiveCamera.position.set( - 4, 2, 7 ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
temporalResolve.temporalResolveMix = 0.9; | ||
temporalResolve.clampRadius = 1; | ||
temporalResolve.newSamplesSmoothing = 0.5; | ||
temporalResolve.newSamplesCorrection = 0.75; | ||
temporalResolve.weightTransform = 0; |
There was a problem hiding this comment.
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.
temporalResolve.temporalResolveMix = 0.9; | ||
temporalResolve.clampRadius = 2; | ||
temporalResolve.newSamplesSmoothing = 0.675; | ||
temporalResolve.newSamplesCorrection = 1; | ||
temporalResolve.weightTransform = 0; |
There was a problem hiding this comment.
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.
typeof window !== 'undefined' ? window.innerWidth : 2000, | ||
typeof window !== 'undefined' ? window.innerHeight : 1000, |
There was a problem hiding this comment.
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
#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 |
There was a problem hiding this comment.
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.
typeof window !== 'undefined' ? window.innerWidth : 2000, | ||
typeof window !== 'undefined' ? window.innerHeight : 1000, |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Also not sure if you have any thoughts on these issues - maybe there's a quick fix? I noticed them in the main 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.movAnd 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.movAnd 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. |
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. |
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 🤔 |
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.
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. |
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. |
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:
The parameters to tweak the
TemporalResolve
class are documented in the readme.Demos
Online Demo
Currently, the two demos
example/index.js
andexample/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)