perf(dynamic cubemaps): encode to b6h#2162
Conversation
📝 WalkthroughWalkthroughThis pull request introduces real-time BC6H GPU encoding for dynamic cubemaps. It adds a new HLSL compute shader implementing fast P1-mode BC6H block encoding, integrates it into the cubemap pipeline via a compression state machine, and allocates supporting GPU resources including BC6H textures, shader resource views, and constant buffers. Changes
Sequence DiagramsequenceDiagram
participant CubemapTask as Cubemap Task
participant Irradiance as Irradiance Stage
participant BC6H as BC6H Compression
participant GPU as GPU Compute
participant OutputTex as Output Textures
CubemapTask->>Irradiance: Execute irradiance capture
Irradiance->>Irradiance: Generate cubemap face
Irradiance->>CubemapTask: Return to task
CubemapTask->>BC6H: Transition to kBC6HCompress
BC6H->>BC6H: Bind BC6H encode shader
BC6H->>BC6H: Iterate mip levels
BC6H->>BC6H: Update encode constant buffer
BC6H->>GPU: Dispatch compute shader<br/>(6 faces per iteration)
GPU->>GPU: Encode 4x4 blocks to BC6H
GPU->>OutputTex: Write to scratch UAV
BC6H->>OutputTex: Copy scratch to destination
BC6H->>CubemapTask: Return to task
CubemapTask->>CubemapTask: Continue pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/BC6HEncodeCS.hlsl (1)
69-103: Optional:Pattern/PatternFixupIDare unused in P1-only mode.With
ENCODE_P2 = 0, these helpers (andPATTERN_NUM) are dead code. The compiler will strip them, but since the adaptation notes already document P2 removal, you could gate these behind#if ENCODE_P2for clarity with the rest of the file (e.g. the#if INSET_COLOR_BBOX/#if OPTIMIZE_ENDPOINTSconventions used elsewhere).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Dynamic` Cubemaps/Shaders/DynamicCubemaps/BC6HEncodeCS.hlsl around lines 69 - 103, Pattern and PatternFixupID (and the related PATTERN_NUM helper) are only needed when P2 encoding is enabled, so wrap their definitions with a preprocessor guard to avoid dead code when ENCODE_P2 == 0; locate the functions Pattern(uint p, uint i) and PatternFixupID(uint i) and surround their full definitions (and any associated constants like PATTERN_NUM) with `#if` ENCODE_P2 ... `#endif` following the project's existing convention (e.g., similar to the INSET_COLOR_BBOX / OPTIMIZE_ENDPOINTS guards).src/Features/DynamicCubemaps.cpp (1)
485-539: LGTM — BC6H dispatch pipeline.The per-mip loop correctly updates
bc6hEncodeCB(blocks-X/Y + mip level), binds the per-mip scratch UAV at slot 0, and dispatchesceil(blocks/8)groups × 6 faces. TheCopyResourcebetweenR32G32B32A32_UINT(W/4 × H/4) andBC6H_UF16(W × H) is valid because each 16-byte UINT4 texel aligns 1:1 with a 16-byte BC6H block, and both resources have identical mip counts and array size. Cleanup of SRV/UAV/CB/CS slots at the end is also correct.Minor nit:
mipDim = max(Width, Height)on line 503 differs fromscratchBase = max(1u, Width/4)used inSetupResources(line 729). Cubemap faces are always square so this is benign today, but for consistency consider usingtexDesc.Widthin both places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/DynamicCubemaps.cpp` around lines 485 - 539, Summary: Use the texture width instead of max(width,height) for mipDim to match SetupResources and maintain consistency. Fix: In DynamicCubemaps::CompressToBC6H replace the mipDim initialization (currently std::max(envTexture->desc.Width, envTexture->desc.Height)) with the texture width used in SetupResources (use envTexture->desc.Width); update the variable at its declaration so the loop and block calculations use that width. Reference symbols: DynamicCubemaps::CompressToBC6H, mipDim, envTexture->desc.Width, and SetupResources's scratchBase usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/DynamicCubemaps.cpp`:
- Around line 617-621: The shader is sampling mip level 15 from BC6H SRVs
(EnvReflectionsTexture, EnvTexture) but those textures only have 8 mips (0–7);
update the code so the sampled mip is clamped to the texture's actual mip count.
Change the hard-coded SampleLevel(…, 15) uses in DynamicCubemaps.hlsli to use a
clamped value (e.g., min(15, MaxMip)) or replace the literal with a shader
constant/uniform supplied from the C++ side that is computed from the SRV
texture description (mipLevels - 1), and ensure the C++ code that binds
EnvReflectionsTexture/EnvTexture (the views array and caller that sets shader
constants) sets that MaxMip value accordingly so SampleLevel never exceeds 7 for
BC6H textures.
- Around line 725-784: The BC6H path reduces bc6hMipLevels (see variable
bc6hMipLevels and the scratch/BC6H texture creation blocks) which makes
hardcoded SampleLevel(..., R, 15) calls in DeferredCompositePS.hlsl
out-of-range; replace those hardcoded 15s with a dynamic value (e.g., MIPLEVELS
- 1 or computed LOD from roughness) so sampling clamps to the actual mip count,
and ensure any code that uses envTexture/envTextureBC6H or
envReflectionsTextureBC6H queries/assumes MIPLEVELS or bc6hMipLevels rather than
a magic 15; update the five SampleLevel occurrences (previously at lines ~244,
272, 279, 286, 294) to use the new constant or computed LOD and validate
specular IBL behavior for reduced BC6H mip counts.
---
Nitpick comments:
In `@features/Dynamic` Cubemaps/Shaders/DynamicCubemaps/BC6HEncodeCS.hlsl:
- Around line 69-103: Pattern and PatternFixupID (and the related PATTERN_NUM
helper) are only needed when P2 encoding is enabled, so wrap their definitions
with a preprocessor guard to avoid dead code when ENCODE_P2 == 0; locate the
functions Pattern(uint p, uint i) and PatternFixupID(uint i) and surround their
full definitions (and any associated constants like PATTERN_NUM) with `#if`
ENCODE_P2 ... `#endif` following the project's existing convention (e.g., similar
to the INSET_COLOR_BBOX / OPTIMIZE_ENDPOINTS guards).
In `@src/Features/DynamicCubemaps.cpp`:
- Around line 485-539: Summary: Use the texture width instead of
max(width,height) for mipDim to match SetupResources and maintain consistency.
Fix: In DynamicCubemaps::CompressToBC6H replace the mipDim initialization
(currently std::max(envTexture->desc.Width, envTexture->desc.Height)) with the
texture width used in SetupResources (use envTexture->desc.Width); update the
variable at its declaration so the loop and block calculations use that width.
Reference symbols: DynamicCubemaps::CompressToBC6H, mipDim,
envTexture->desc.Width, and SetupResources's scratchBase usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4456f0a0-17fb-49ff-8e25-04259d22fa13
📒 Files selected for processing (3)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/BC6HEncodeCS.hlslsrc/Features/DynamicCubemaps.cppsrc/Features/DynamicCubemaps.h
|
✅ A pre-release build is available for this PR: |
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> (cherry picked from commit 28ee046)
2 participants
Summary by CodeRabbit