#8833 fix: ensure dof effect is resolution independent#8834
Conversation
|
@mvaligursky What do you suggest we do with "high quality" switch. From what I can tell it was a workaround for this issue and as such no longer makes sense. Remove it? Deprecate it? |
There was a problem hiding this comment.
Pull request overview
This PR fixes the Depth of Field (DoF) blur varying with render resolution / pixel ratio by redefining blur radius in the DoF blur shader as a normalized value (calibrated against a reference height) and adding aspect correction so the blur kernel remains circular in screen pixels.
Changes:
- Update GLSL/WGSL DoF blur shaders to use an aspect-corrected, resolution-independent blur step.
- Normalize
blurRadiusNear/Faragainst a newreferenceHeight(default 540) when uploading uniforms. - Remove the previous per-quality compensation for far blur radius since scaling is now handled consistently.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/scene/shader-lib/wgsl/chunks/render-pass/frag/dofBlur.js | Makes WGSL DoF blur step resolution-independent and aspect-corrected. |
| src/scene/shader-lib/glsl/chunks/render-pass/frag/dofBlur.js | Makes GLSL DoF blur step resolution-independent and aspect-corrected. |
| src/extras/render-passes/render-pass-dof-blur.js | Adds reference-height normalization when setting blur radius uniforms. |
| src/extras/render-passes/frame-pass-dof.js | Removes quality-based far blur compensation and updates blur radius wiring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mvaligursky
left a comment
There was a problem hiding this comment.
Looks good — the approach is correct. Expressing the blur as a fraction of a reference height (rather than in source-texture texels) is the right way to make this resolution- and pixel-ratio-independent, and the height/width factor on the X step correctly keeps the kernel circular in screen pixels. The math checks out: in pixels both axes resolve to kernel * blurRadius/540 * height. Picking referenceHeight = 540 to reproduce the old high-quality blur at 1080p is a sensible calibration that preserves previously authored blurRadius values. Perf impact is effectively nil (one scalar divide swapped for a vec2 divide; no change to tap count or texture sizes).
On the high quality flag question: I'd keep it. It's not the workaround for this issue — it controls the internal resolution of the DoF textures (half-of-full-res far texture + full-res composite vs. quarter-res), which is a genuine quality/perf trade-off independent of the resolution-dependence bug. The actual workaround was the * (highQuality ? 1 : 0.5) factor on blurRadiusFar, and you've already correctly removed that — now that blur is reference-height-relative, the far-texture resolution no longer changes blur strength, so the compensation is obsolete. So nothing further to deprecate.
Two small suggestions inline (both optional, ready to accept).
A suggestion was applied one line off, leaving the original invReferenceHeight declaration in place alongside the guarded one, breaking the build. Remove the duplicate and keep the reference-height guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* #8833 fix: ensure dof effect is resolution independent * Update src/extras/render-passes/frame-pass-dof.js * Update src/extras/render-passes/render-pass-dof-blur.js * fix: remove duplicate invReferenceHeight declaration A suggestion was applied one line off, leaving the original invReferenceHeight declaration in place alongside the guarded one, breaking the build. Remove the duplicate and keep the reference-height guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Martin Valigursky <59932779+mvaligursky@users.noreply.github.com> Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Description
Fixes #8833
High resolution
Low resolution
Checklist