Conversation
chore: update lut build with dev
WalkthroughSupport for loading and binding a DDS LUT texture was added to the Deferred rendering system. A new method binds the LUT to the pixel shader. Two hooks were introduced to ensure the LUT is bound during specific image-space shader technique setups by intercepting virtual function calls. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Deferred
participant DirectXTex
participant Shader
App->>Deferred: SetupResources()
Deferred->>DirectXTex: CreateDDSTextureFromFile(LUT.dds)
DirectXTex-->>Deferred: Returns lutTexture
Deferred->>Deferred: Store lutTexture
App->>Shader: SetupTechnique()
Shader->>Deferred: BindLUT()
Deferred->>Shader: Bind lutTexture to pixel shader slot 100
sequenceDiagram
participant App
participant Shader
participant Deferred
App->>Shader: Call SetupTechnique()
Shader->>Deferred: Hooks call BindLUT()
Deferred->>Shader: Bind LUT texture if valid
Shader->>App: Continue original SetupTechnique()
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Why in deferred? |
No reason. I am only making this PR so that when me and Bottle test things we can test on any CS build. If it's effectively test code I don't know that it matters where it goes. I could just add comments to mark that it's all test code? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Deferred.cpp (2)
181-181: Consider making the LUT file path configurable.The hardcoded path
"Data\\Shaders\\LUT.dds"limits flexibility for testing different LUT files. Since this is marked as testing code, consider making the path configurable through a setting or command line parameter.
728-728: Clarify the lifecycle of this testing code.Since this is explicitly marked as "Testing code for imagespace shaders", consider adding documentation about when this code should be removed or converted to production code.
Add a comment clarifying the testing nature:
-// Testing code for imagespace shaders +// Testing code for imagespace shaders - TODO: Remove or convert to configurable featuresrc/Deferred.h (1)
133-151: Consider the scope limitation of targeting specific shaders.The hooks only target
BSImagespaceShaderHDRTonemapBlendCinematicandBSImagespaceShaderHDRTonemapBlendCinematicFade. This limits LUT usage to these specific shaders. If broader LUT support is desired in the future, you might need to hook additional shader types or use a more general approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Deferred.cpp(3 hunks)src/Deferred.h(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (5)
src/Deferred.cpp (2)
3-3: Header inclusion looks good.The
DDSTextureLoader.hinclude is appropriate for the DDS texture loading functionality being added.
728-734: LUT binding implementation looks correct.The null check and binding to slot 100 is appropriate. Slot 100 is high enough to avoid conflicts with typical shader resource bindings.
src/Deferred.h (3)
80-81: Member declarations look appropriate.The
winrt::com_ptr<ID3D11ShaderResourceView>type is correct for managing D3D11 shader resource views, and theBindLUT()method declaration is properly placed.
133-151: Hook implementations follow established patterns.The hook structures correctly call
BindLUT()before invoking the original function, which ensures the LUT is bound when these specific imagespace shaders are set up.
168-169:Details
✅ Verification successful
Verify the vtable function index.
The hooks are installed at vtable index 0x2. Please verify that this corresponds to the
SetupTechniquemethod in bothBSImagespaceShaderHDRTonemapBlendCinematicandBSImagespaceShaderHDRTonemapBlendCinematicFadevtables.
🏁 Script executed:
#!/bin/bash # Description: Search for SetupTechnique method usage and vtable references # Expected: Find references to SetupTechnique method and verify vtable index usage echo "Searching for SetupTechnique method references..." rg -A 3 -B 3 "SetupTechnique" echo -e "\nSearching for other vtable hooks with index 0x2..." rg -A 2 -B 2 "write_vfunc.*0x2"Length of output: 5666
Vtable index 0x2 correctly corresponds to SetupTechnique
Both BSImagespaceShaderHDRTonemapBlendCinematic and its Fade variant inherit from RE::BSShader, where
• ~BSShader() … // 00
• virtual bool SetupTechnique(…) = 0; // 02The hooks in Deferred.h use
stl::write_vfunc<0x2, …SetupTechnique>—exactly the same index used elsewhere (e.g. BSGrassShader_SetupTechnique). No adjustment is needed.
|
|
||
| // Testing code for imagespace shaders | ||
| { | ||
| auto device = globals::d3d::device; | ||
| auto context = globals::d3d::context; | ||
| DirectX::CreateDDSTextureFromFile(device, context, L"Data\\Shaders\\LUT.dds", nullptr, lutTexture.put()); | ||
| } |
There was a problem hiding this comment.
Add error handling for LUT file loading.
The hardcoded path and lack of error handling could cause issues if the LUT file doesn't exist or fails to load.
Consider adding proper error handling:
// Testing code for imagespace shaders
{
auto device = globals::d3d::device;
auto context = globals::d3d::context;
- DirectX::CreateDDSTextureFromFile(device, context, L"Data\\Shaders\\LUT.dds", nullptr, lutTexture.put());
+ auto hr = DirectX::CreateDDSTextureFromFile(device, context, L"Data\\Shaders\\LUT.dds", nullptr, lutTexture.put());
+ if (FAILED(hr)) {
+ logger::warn("Failed to load LUT texture: Data\\Shaders\\LUT.dds (HRESULT: 0x{:08X})", static_cast<uint32_t>(hr));
+ lutTexture = nullptr;
+ }
}🤖 Prompt for AI Agents
In src/Deferred.cpp around lines 180 to 186, the code loads a LUT texture from a
hardcoded path without checking for errors, which can cause issues if the file
is missing or fails to load. Modify the code to capture the HRESULT returned by
CreateDDSTextureFromFile and check if the operation succeeded. If it fails, log
an appropriate error message or handle the failure gracefully to prevent silent
errors.
doodlum
changed the title
* chore: lut * fix: do not bind if invalid texture * chore: add testing code comments
* chore: lut * fix: do not bind if invalid texture * chore: add testing code comments
* chore: lut * fix: do not bind if invalid texture * chore: add testing code comments
2 participants
Summary by CodeRabbit