Fix CAS sharpening clamping scene colors to [0-1] range - #76
Open
filipppavlov wants to merge 2 commits into
Open
Fix CAS sharpening clamping scene colors to [0-1] range#76filipppavlov wants to merge 2 commits into
filipppavlov wants to merge 2 commits into
Conversation
…ering and change tonemapping result texture to FP16 if CAS is required; expose sharpening strength
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cpp-linter Review
Used clang-format v21.1.8
Click here for the full clang-format patch
diff --git a/trinity/PostProcess/Tr2PostProcessRenderer.cpp b/trinity/PostProcess/Tr2PostProcessRenderer.cpp
index ea8af58..d2030bc 100644
--- a/trinity/PostProcess/Tr2PostProcessRenderer.cpp
+++ b/trinity/PostProcess/Tr2PostProcessRenderer.cpp
@@ -687,4 +687,4 @@ void Tr2PostProcessRenderer::Execute(
- Tr2GpuResourcePool::Texture output = gpuResourcePool.GetTempTexture(
- "Final Result",
- displaySize,
- sharpeningRequired ? GetUavCompatibleFormat( destination.GetFormat() ) : destination.GetFormat(),
+ Tr2GpuResourcePool::Texture output = gpuResourcePool.GetTempTexture(
+ "Final Result",
+ displaySize,
+ sharpeningRequired ? GetUavCompatibleFormat( destination.GetFormat() ) : destination.GetFormat(),
@@ -821 +821 @@ void Tr2PostProcessRenderer::Execute(
- else if ( sharpeningRequired )
+ else if( sharpeningRequired )
@@ -841 +841 @@ void Tr2PostProcessRenderer::Execute(
- else if ( sharpeningRequired )
+ else if( sharpeningRequired )
Have any feedback or feature suggestions? Share it here.
Comment on lines
+687
to
+690
| Tr2GpuResourcePool::Texture output = gpuResourcePool.GetTempTexture( | ||
| "Final Result", | ||
| displaySize, | ||
| sharpeningRequired ? GetUavCompatibleFormat( destination.GetFormat() ) : destination.GetFormat(), |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| Tr2GpuResourcePool::Texture output = gpuResourcePool.GetTempTexture( | |
| "Final Result", | |
| displaySize, | |
| sharpeningRequired ? GetUavCompatibleFormat( destination.GetFormat() ) : destination.GetFormat(), | |
| Tr2GpuResourcePool::Texture output = gpuResourcePool.GetTempTexture( | |
| "Final Result", | |
| displaySize, | |
| sharpeningRequired ? GetUavCompatibleFormat( destination.GetFormat() ) : destination.GetFormat(), |
| output = upscaled; | ||
| } | ||
| } | ||
| else if ( sharpeningRequired ) |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| else if ( sharpeningRequired ) | |
| else if( sharpeningRequired ) |
| Tr2Renderer::DrawTexture( renderContext, output ); | ||
| } | ||
| } | ||
| else if ( sharpeningRequired ) |
There was a problem hiding this comment.
clang-format suggestion
Suggested change
| else if ( sharpeningRequired ) | |
| else if( sharpeningRequired ) |
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adjusts Trinity’s post-processing pipeline to avoid CAS sharpening clamping HDR scene colors by moving sharpening to run after tonemapping, and introduces a project-wide sharpening strength control.
Changes:
- Move CAS sharpening to run after tonemapping and gate tonemapping dithering via a new
DitherStrengthparameter. - Allocate post-process outputs with UAV-compatible formats when CAS sharpening is required, to support higher-precision tonemap output and compute-based sharpening.
- Expose
Tr2PostProcess2::m_sharpeningStrength(persisted Blue attribute) and propagate it into the combined post-process object.
File summaries
| File | Description |
|---|---|
| trinity/PostProcess/Tr2PostProcessRenderer.h | Updates sharpening API to take strength and explicit input/output textures. |
| trinity/PostProcess/Tr2PostProcessRenderer.cpp | Reorders sharpening relative to tonemapping/upscaling, adds dithering control, and adjusts output texture allocation/format usage. |
| trinity/PostProcess/Tr2PostProcess2.h | Adds global sharpening strength field. |
| trinity/PostProcess/Tr2PostProcess2_Blue.cpp | Exposes sharpening strength as a persisted Blue attribute. |
| trinity/Eve/EveSpaceScene.cpp | Propagates scene default sharpening strength into the combined post-process instance. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Move CAS sharpening pass in the post-process pipeline to be executed after tonemapping.
Details
The issue observed is that scene colors are clamped to [0-1] range before exposure and tonemapping due to CAS sharpening pass: CAS expects linear colors in [0-1] range for both input and output.
The issue was introduced in v3.6.4 when sharpening was moved to be before tonemapping, and the reason for the move was that sharpening is amplifying dithering added to tonemapping.
This PR moves sharpening back to its original place. Dithering performed by the tonemapping shader is now supposed to happen in CAS shader (after sharpening). Tonemapping may still need to perform dithering if CAS sharpening is disabled. Trinity now passes
DitherStrengthparameter to the tonemapping shader (either 0 or 1), and the shader is supposed to disable dithering if the parameter is zero.To perform dithering correctly, the output of the tonemapping shader (if CAS is enabled) needs to be of higher precision, so post-processing uses the source texture format for it.
On top of the fix, the PR exposes a sharpening strength attribute on the
Tr2PostProcess2object, so that projects can tweak this value. It was decided that this value only needs to be tweakable globally per-project, and there is no need to have it in post-process volumes.Note on implementation. There are more changes to Tr2PostProcessRenderer.cpp than I would like. The reason for it is that the
outputtexture needs to be set as the "hudless texture" for upscaling (for frame generation), and the code needs to make sure that throughout all the branches, the result of post-processing ends up in that texture.Shader changes
This PR needs corresponding shader changes to tonemapping and CAS sharpening
Task link
https://fenriscreations.atlassian.net/browse/PLAT-11873