DoF bokeh shaping: polygonal aperture, anamorphic, rotation, and intensity#346
DoF bokeh shaping: polygonal aperture, anamorphic, rotation, and intensity#346taylnos wants to merge 5 commits into
Conversation
Give the DoF bokeh real lens character by reshaping the gather kernel into an
aperture-blade polygon and/or an anamorphic oval. Both are pure sample-
placement reshapes -- the tap count is unchanged, so the O(CoC^2) gather cost
is unaffected. All controls default to neutral values, so the existing
circular bokeh is preserved bit-for-bit until a user opts in.
- postDeferredF.glsl: new bokehOffset() reshapes tap placement at both the
near (FRONT_BLUR) and far gather sites, with a coherent early-out that
returns the identical circular offset when inactive. Two CPU-baked vec4
uniforms (bokeh_shape, bokeh_lens) carry the kernel.
- ALDoFBokeh::bakeKernel (new aldofbokeh.{h,cpp}): pure helper that clamps the
settings and pre-bakes the N-gon trig and area-preserving anamorphic scale
on the CPU (mirroring the chromatic-aberration path). Covered by a TUT unit
test (aldofbokeh_test.cpp).
- pipeline.cpp renderDoF(): reads the settings, bakes once per pass, pushes
the two uniforms onto the active post program.
- settings_alchemy.xml: 4 opt-in settings (RenderDoFBokehBlades / Roundness /
Rotation, RenderDoFAnamorphicRatio), all neutral by default.
- llshadermgr.{h,cpp}: register the two reserved DoF bokeh uniforms.
Lens/chromatic aberration is intentionally out of scope (a screen-space CA
already exists) and left for a follow-up.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two refinements to the DoF bokeh shaping: - Rotation now rigidly rotates the entire aperture. Previously rotation was folded into the sampling angle while the anamorphic scale was applied on the fixed screen axes, so the oval never tilted (only the polygon rotated). Build the aperture in its own frame, then rotate the whole offset -- polygon AND anamorphic oval spin together. cos/sin(rotation) is loop-invariant so it is hoisted out of the gather loop. - Add RenderDoFBokehIntensity (default 1.0): scales the gather's existing highlight weighting (wg = 0.25 + intensity*(r+g+b)) so bright out-of-focus points form brighter, more defined bokeh discs -- which makes the aperture shape and its rotation legible. Independent of the offset reshape, so it also intensifies a plain round bokeh. 1.0 reproduces the classic weighting exactly; 0 gives a flat soft blur. Baked/clamped in ALDoFBokeh::bakeKernel (unit test extended to 7 cases) and carried by a new bokeh_intensity reserved uniform. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The intensity control was a linear weight multiplier, but the gather is a normalized weighted average -- a global weight scale cancels in the ratio, so past ~1.0 the control saturated (0->1 was a strong change, 1->16 barely moved). Reparameterize intensity as an exponent on the highlight weight (wg = 0.25 + pow(lum, intensity)): changing the exponent changes which samples dominate rather than applying a scale that cancels, so every step keeps having an effect. 1.0 stays a bit-exact linear fast path (existing DoF unchanged); the base is clamped for overflow safety and the range tightened to [0, 8] (an exponent of 8 is already extreme). Settings help text and the unit test updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds CPU-side depth-of-field bokeh kernel baking, new shader uniforms and settings, pipeline wiring to upload the baked kernel, and build/test integration for the new module. ChangesDoF Bokeh Kernel Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Pipeline as pipeline.cpp
participant Bokeh as ALDoFBokeh::bakeKernel
participant Settings as gSavedSettings
participant Shader as DOF shader uniforms
Pipeline->>Settings: read blades, roundness, rotation, anamorphic ratio, intensity
Pipeline->>Bokeh: bakeKernel(blades, roundness, rotationDeg, anamorphicRatio, intensity)
Bokeh->>Bokeh: clamp inputs and compute Kernel fields
Bokeh-->>Pipeline: return Kernel
Pipeline->>Shader: upload DOF_BOKEH_SHAPE, DOF_BOKEH_LENS, DOF_BOKEH_INTENSITY
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
indra/llrender/llshadermgr.h (1)
223-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent terminology: "scale" vs. "exponent" for
bokeh_intensity.This comment describes
bokeh_intensityas a "highlight-weight scale", butALDoFBokeh::Kernel::intensity(aldofbokeh.h) and the referenced GLSL comment both document it as an exponent (per the PR's commit history: "changing intensity from a linear multiplier to an exponent"). Align the wording here to avoid confusing future readers of the reserved-uniform table about how the value is applied in the shader.📝 Suggested comment fix
- DOF_BOKEH_INTENSITY, // "bokeh_intensity" (float: highlight-weight scale) + DOF_BOKEH_INTENSITY, // "bokeh_intensity" (float: highlight-weight exponent)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/llrender/llshadermgr.h` around lines 223 - 225, The reserved-uniform table entry for DOF_BOKEH_INTENSITY uses wording that conflicts with the shader and ALDoFBokeh::Kernel::intensity documentation. Update the inline comment for bokeh_intensity in llshadermgr.h to match the exponent-based terminology used elsewhere, so the description is consistent with the GLSL comment and the kernel field name.indra/newview/aldofbokeh.cpp (2)
66-66: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueExact float equality for the
activeflag.
anamorphicRatio != 1.fuses exact float comparison. SinceanamorphicRatiocomes from a user-editable debug setting parsed as text, values extremely close to but not exactly 1.0 (e.g. rounding artifacts from a UI slider) would flipactivetrue and take the reshaping path unnecessarily, even though visually indistinguishable from circular. Low risk given the default is likely stored as exact 1.0, but worth a small epsilon tolerance for robustness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/aldofbokeh.cpp` at line 66, The `active` flag in `aldofbokeh.cpp` is using an exact float comparison on `anamorphicRatio`, which can incorrectly enable the reshaping path for values that are visually indistinguishable from 1.0 due to parsing or slider rounding. Update the logic in the code that sets `k.active` to use a small epsilon/tolerance around the `anamorphicRatio` check instead of `!= 1.f`, while keeping the existing `clampedBlades` condition intact.
58-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "scale" wording; field is now an exponent.
The
Kernel::intensityfield doc in aldofbokeh.h explicitly calls this a "highlight-weight EXPONENT", matching the PR's move from a linear multiplier to an exponent, but this comment still says "Highlight-weight scale". Likely left over from before the exponent change (per commit history). Update for consistency to avoid confusing future maintainers about how the shader consumes this value.📝 Suggested comment fix
- // Highlight-weight scale for the gather. Independent of the offset reshape + // Highlight-weight exponent for the gather. Independent of the offset reshape // (it also intensifies a plain round bokeh); 1.0 reproduces the classic // weighting exactly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/aldofbokeh.cpp` around lines 58 - 61, Update the stale comment in the bokeh kernel setup so it matches the current meaning of Kernel::intensity as an exponent rather than a linear scale. In aldofbokeh.cpp around the code that assigns k.intensity, revise the wording to describe a highlight-weight exponent and keep it consistent with the documentation in aldofbokeh.h and the shader usage in the bokeh kernel path.indra/newview/aldofbokeh.h (1)
52-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the blade upper-bound clamp.
The docstring lists clamped ranges for roundness, anamorphicRatio, and intensity, but omits that blade count is also capped at 12 (
llmin(blades, 12)in aldofbokeh.cpp). Worth adding for completeness since this comment is the documented contract callers rely on.📝 Suggested doc tweak
- // Bake the shader kernel from raw setting values. Inputs are clamped to their - // documented, sane ranges here so an out-of-range debug setting can never feed - // garbage into the gather: blades below 3 disable the polygon (circular); - // roundness clamps to [0, 1]; anamorphicRatio clamps to [0.25, 4] (1 = off); - // intensity clamps to [0, 8] (1 = off; the shader applies it as an exponent). + // Bake the shader kernel from raw setting values. Inputs are clamped to their + // documented, sane ranges here so an out-of-range debug setting can never feed + // garbage into the gather: blades below 3 disable the polygon (circular), + // blades above 12 are clamped to 12; roundness clamps to [0, 1]; + // anamorphicRatio clamps to [0.25, 4] (1 = off); intensity clamps to [0, 8] + // (1 = off; the shader applies it as an exponent).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/aldofbokeh.h` around lines 52 - 57, The Kernel::bakeKernel docstring currently describes the lower-bound behavior for blades but omits the upper-bound clamp applied in the implementation. Update the comment near bakeKernel in aldofbokeh.h to state that blades are also capped at 12, matching the clamping logic in Kernel::bakeKernel so the documented contract is complete and accurate.indra/newview/tests/aldofbokeh_test.cpp (1)
50-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a boundary test for
blades == 3.
test<2>covers blades 0-2 (inactive) andtest<3>jumps straight to blades=6. The exact activation threshold (blades == 3) is never directly exercised, which is the most off-by-one-prone value.✅ Suggested additional boundary test
// A hexagonal aperture bakes exactly the documented N-gon constants. template<> template<> void dofbokeh_object::test<3>() { Kernel k = bakeKernel(6, 0.f, 0.f, 1.f, 1.f); ensure("hexagon is active", k.active); ensure_distance("seg = 2pi/6", k.seg, F_PI / 3.f, 1e-5f); ensure_distance("halfSeg = pi/6", k.halfSeg, F_PI / 6.f, 1e-5f); ensure_distance("edge = cos(pi/6)", k.edge, cosf(F_PI / 6.f), 1e-5f); } + + // A triangle (minimum blade count) is the exact activation boundary. + template<> template<> + void dofbokeh_object::test<8>() + { + Kernel k = bakeKernel(3, 0.f, 0.f, 1.f, 1.f); + ensure("blades==3 is active", k.active); + ensure_distance("seg = 2pi/3", k.seg, 2.f * F_PI / 3.f, 1e-5f); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indra/newview/tests/aldofbokeh_test.cpp` around lines 50 - 71, Add a boundary case test for the exact activation threshold in dofbokeh_object::test by exercising bakeKernel with blades == 3, since test<2> only covers 0-2 and test<3> skips directly to 6. Extend the existing aldofbokeh test coverage with a focused assertion that blades=3 produces an active Kernel and nonzero polygon segment values, keeping the style consistent with the current ensure/ensure_distance checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@indra/llrender/llshadermgr.h`:
- Around line 223-225: The reserved-uniform table entry for DOF_BOKEH_INTENSITY
uses wording that conflicts with the shader and ALDoFBokeh::Kernel::intensity
documentation. Update the inline comment for bokeh_intensity in llshadermgr.h to
match the exponent-based terminology used elsewhere, so the description is
consistent with the GLSL comment and the kernel field name.
In `@indra/newview/aldofbokeh.cpp`:
- Line 66: The `active` flag in `aldofbokeh.cpp` is using an exact float
comparison on `anamorphicRatio`, which can incorrectly enable the reshaping path
for values that are visually indistinguishable from 1.0 due to parsing or slider
rounding. Update the logic in the code that sets `k.active` to use a small
epsilon/tolerance around the `anamorphicRatio` check instead of `!= 1.f`, while
keeping the existing `clampedBlades` condition intact.
- Around line 58-61: Update the stale comment in the bokeh kernel setup so it
matches the current meaning of Kernel::intensity as an exponent rather than a
linear scale. In aldofbokeh.cpp around the code that assigns k.intensity, revise
the wording to describe a highlight-weight exponent and keep it consistent with
the documentation in aldofbokeh.h and the shader usage in the bokeh kernel path.
In `@indra/newview/aldofbokeh.h`:
- Around line 52-57: The Kernel::bakeKernel docstring currently describes the
lower-bound behavior for blades but omits the upper-bound clamp applied in the
implementation. Update the comment near bakeKernel in aldofbokeh.h to state that
blades are also capped at 12, matching the clamping logic in Kernel::bakeKernel
so the documented contract is complete and accurate.
In `@indra/newview/tests/aldofbokeh_test.cpp`:
- Around line 50-71: Add a boundary case test for the exact activation threshold
in dofbokeh_object::test by exercising bakeKernel with blades == 3, since
test<2> only covers 0-2 and test<3> skips directly to 6. Extend the existing
aldofbokeh test coverage with a focused assertion that blades=3 produces an
active Kernel and nonzero polygon segment values, keeping the style consistent
with the current ensure/ensure_distance checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ba45b8f9-b774-4aae-8371-497909e92be5
⛔ Files ignored due to path filters (1)
indra/newview/app_settings/shaders/class1/deferred/postDeferredF.glslis excluded by!**/*.glsl
📒 Files selected for processing (8)
indra/llrender/llshadermgr.cppindra/llrender/llshadermgr.hindra/newview/CMakeLists.txtindra/newview/aldofbokeh.cppindra/newview/aldofbokeh.hindra/newview/app_settings/settings_alchemy.xmlindra/newview/pipeline.cppindra/newview/tests/aldofbokeh_test.cpp
…ilon, docs, test) - llshadermgr.h / aldofbokeh.cpp: describe bokeh_intensity as a highlight-weight EXPONENT (not "scale"), matching aldofbokeh.h and the shader, which apply it via pow(). - aldofbokeh.cpp: decide `active` with a small epsilon (fabsf(anamorphicRatio - 1) > 1e-3) instead of an exact float inequality, so a ratio that rounds to ~1.0 (UI-slider noise) stays on the circular path. - aldofbokeh.h: document that blades are also capped at 12 (upper bound). - aldofbokeh_test.cpp: add a blades==3 boundary case (the exact activation threshold) asserting it is active and bakes the triangle N-gon constants. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the review! Addressed the five nitpicks in 8074587 (these came through as a single consolidated review rather than inline threads, so replying here):
🤖 Addressed by Claude Code |
pow(lum, bokeh_intensity) with bokeh_intensity == 0 and a pure-black tap
(lum == 0) is pow(0,0) -- undefined in GLSL, and NaN on common drivers.
Intensity 0 is a legal, documented value ("0 = fully flat" in the
RenderDoFBokehIntensity setting text), and one NaN-weighted tap poisons the
normalized average for the whole disc, smearing a NaN blotch through the
blur. Floor the clamped luma to 1e-5: pow(x>0, 0) == 1 gives exactly the
intended flat weight, and at every other intensity the floor is
imperceptible under the 0.25 weight base. Both gather sites fixed.
Found by an adversarial shader-math review on the follow-on compute-DoF
branch, which shares this expression; fixed there in 8f86115.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed one post-review commit: f68a193. What: the pop weight's Fix: floor the already-clamped luma to 1e-5 at both gather sites ( Found by a shader-math audit on the follow-on compute-DoF branch, which shares this expression (fixed there too); verified in-world at intensity 0. |
1 participant
Summary
Adds camera-lens character to the Depth-of-Field bokeh — an aperture-blade polygon, an anamorphic (oval) squeeze, aperture rotation, and a highlight intensity control — all opt-in. Every control defaults to a neutral value, so the current circular bokeh is preserved exactly until a user opts in.
New settings (
settings_alchemy.xml, editable via Debug Settings)RenderDoFBokehBlades(S32)0<3= round (off). 5–9 emulates a partially-closed iris (pentagon…enneagon).RenderDoFBokehRoundness(F32)00= crisp polygon …1= circle (only when blades ≥ 3).RenderDoFBokehRotation(F32°)0RenderDoFAnamorphicRatio(F32)1.01= circular;>1= vertical-oval anamorphic. Area-preserving.RenderDoFBokehIntensity(F32)1.00= flat.1.0is the classic look.How it works
The DoF gather (
postDeferredF.glsl) placed its blur taps on a circle. This reshapes where each tap lands — into a rounded N-gon and/or an anamorphic oval, rigidly rotated — via a newbokehOffset()helper called at both gather sites (near + far). The tap count is unchanged, so the O(CoC²) gather cost is unaffected; the reshape adds only a few ALU ops, and a coherent early-out returns the identical circular offset when everything is neutral.The N-gon trig and anamorphic scale are baked once per frame on the CPU (
ALDoFBokeh::bakeKernel, mirroring the existing chromatic-aberration path) and handed to the shader as twovec4uniforms plus one scalar, so the shader avoids per-tap trig.Intensity is an exponent on the existing highlight weight (
wg = 0.25 + pow(lum, intensity)), not a multiplier. The gather is a normalized weighted average, so a global scale cancels in the ratio and saturates past ~1; an exponent instead changes which samples dominate and stays useful across its whole range.1.0reproduces the classic weighting via a bit-exact fast path.Performance
No new texture taps — the reshape is a handful of ALU ops per existing tap, and feature-off is bit-for-bit the current bokeh (a coherent, uniform-driven branch skips the reshape entirely). Intensity swaps a multiply for a
powper tap only when it is ≠ 1 (ALU on an otherwise texture-fetch-bound loop).Tests
The CPU-side kernel baking is factored into a pure, dependency-light helper (
ALDoFBokeh::bakeKernelinindra/newview/aldofbokeh.{h,cpp}) with a TUT unit test (indra/newview/tests/aldofbokeh_test.cpp, 7 cases): neutral→inactive, blade-count threshold, N-gon constants, deg→rad conversion, area-preserving anamorphic squeeze, input clamping, and intensity clamp/passthrough.ctest -R aldofbokehpasses. The shader and wiring were validated by a full Release build plus in-viewer runtime shader-compile smoke tests.Notes
🤖 Generated with Claude Code