Add support for multiple custom post-processing effects - #92
Open
filipppavlov wants to merge 5 commits into
Open
filipppavlov wants to merge 5 commits into
filipppavlov wants to merge 5 commits into
Conversation
- add generic effects to post-processing volumes - mix multiple effects together - expose two places in post-processing pipeline to execute these effects
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🟡 Changes recommended
A potential null dereference and unresolved ordering, blending, compatibility, performance, and configuration issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds support for multiple custom post-processing effects configured through post-process volumes, including blending, ordering, execution slots, and pipeline integration.
Changes:
- Adds effect accumulation, grouping, hashing, and parameter blending.
- Executes effects before upscaling and after tonemapping.
- Exposes configuration through Blue and documents setup.
File summaries
| File | Reviewed changes and findings |
|---|---|
trinity/Shader/Tr2Effect.h |
Declares parameter-aware hashing support. |
trinity/Shader/Tr2Effect.cpp |
Implements effect hashing. moderate (1 vote): unsupported blendable parameter kinds are excluded from the hash. |
trinity/PostProcess/Tr2PostProcessRenderer.h |
Declares multi-effect rendering. |
trinity/PostProcess/Tr2PostProcessRenderer.cpp |
Executes effects in pipeline slots. critical (3 votes): guard null GetPostProcess() results at lines 821 and 842. |
trinity/PostProcess/Tr2PostProcessAttributes.h |
Adds generic effect attributes. |
trinity/PostProcess/Tr2PostProcessAttributes.cpp |
Accumulates generic effects. moderate (1 vote): legacy effect updates can lag by one frame. |
trinity/PostProcess/Tr2PostProcessAttributes_Blue.cpp |
Exposes generic effect collections. moderate (1 vote): make genericEffects editable and persistent. |
trinity/PostProcess/Tr2PostProcess2.h |
Stores accumulated effects. |
trinity/PostProcess/Effects/Tr2PPGenericEffect.h |
Defines effect metadata and instances. moderate (2 votes): preserve the previous MEDIUM quality default. |
trinity/PostProcess/Effects/Tr2PPGenericEffect.cpp |
Groups and blends effects. moderate: deduplicate stage constants (3 votes), include m_order in merge checks (2 votes), avoid full per-frame hashing (1 vote), and fix legacy accumulation timing (2 votes). |
trinity/PostProcess/Effects/Tr2PPGenericEffect_Blue.cpp |
Exposes execution configuration. |
doc/source/eve/generic-pp-effects.rst |
Documents custom effects. nit: correct execututionSlot (3 votes) and “effect instanced” grammar (1 vote). |
Review details
Suppressed comments (7)
doc/source/eve/generic-pp-effects.rst:32
- This sentence contains a duplicated word,
the the, which makes the shader input description unpolished.
The effect may use the the output of the previous post-processing step as the input texture. For that, the shader needs to define a texture 2D object
doc/source/eve/generic-pp-effects.rst:42
effect instancedis grammatically incorrect here; the sentence should refer to each effect instance.
Trinity will blend these parameters based on the volume intensity/relative weight. For each effect instanced it will interpolate such parameters between their
trinity/PostProcess/Effects/Tr2PPGenericEffect.cpp:155
Tr2Effect::GetHashValue()is explicitly documented as non-lightweight and unsuitable for per-frame calls (Shader/Tr2Effect.cpp:1516-1519), butUpdatePostProcessAttributes()invokesUpdateEffectParameters()for every active volume every frame and this line recomputes the full effect hash before every cache check. With multiple effects this adds a full parameter/resource traversal per effect per frame; use mutation/dirty tracking or another invalidation mechanism instead of polling the full hash.
auto hash = m_effect->GetHashValue();
if( hash == m_lastHashValue && m_lastShader == shader )
trinity/PostProcess/Tr2PostProcessAttributes.cpp:150
AccumulateGenericEffectsnow snapshots the deprecatedm_genericeffect beforeEveSpaceSceneassigns the current scene-default effect afterMergeInto(Eve/EveSpaceScene.cpp:391-399). Since the renderer consumesm_genericEffectsinstead of readingm_genericdirectly, the first frame misses a newly assigned legacy effect and subsequent replacements/removals render the previous effect for one frame. Update the legacy effect before accumulation or pass the current legacy effect into the accumulator.
AccumulateGenericEffects( postprocess, sources );
trinity/PostProcess/Tr2PostProcessAttributes_Blue.cpp:117
genericEffectsis the user-authored list that the documentation says configures effects on post-process volumes, but it is exposed withBe::READ. Unlike generated lists elsewhere, this attribute has no add/modify API, so Blue cannot configure the new effects through the volume. Expose it asBe::READWRITE | Be::PERSIST, matching the editablepostProcessAttributesonEveChildPostProcessVolume.
MAP_ATTRIBUTE( "genericEffects", genericEffects, "List of generic post-processing effects", Be::READ | Be::PERSIST )
trinity/PostProcess/Tr2PostProcessRenderer.cpp:842
- This branch has the same null dereference as the upscaling branch:
postProcesscan be null, but the new after-tonemap call dereferences it before rendering the final output. Use an empty effect list/output when no post-process object exists.
auto newOutput = RenderGenericEffects( postProcess->m_genericEffects.effects[Tr2PPGenericEffect::AFTER_TONEMAP], output, gpuResourcePool, renderContext );
trinity/Shader/Tr2Effect.cpp:1574
- This excludes every parameter carrying
IsBlendable, butUpdateEffectParametersonly recognizes supported scalar/vector float constants (one element, dimensions 1–4). An annotated array, matrix, or non-float parameter is therefore neither accumulated nor included in this hash; effects with different values can merge and the first effect's value wins. Restrict hash exclusion to the supported blendable kinds or include unsupported annotated values.
if ( GetBool( m_shader, ( *it )->GetParameterName(), "IsBlendable", false ) )
{
continue;
- Files reviewed: 12/12 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| { | ||
| auto newOutput = RenderGenericEffects( postProcess->m_genericEffects.effects[Tr2PPGenericEffect::AFTER_TONEMAP], output, gpuResourcePool, renderContext ); |
Comment on lines
+74
to
+81
| if( auto deprecated = postprocess.GetGenericEffectIfAvailable() ) | ||
| { | ||
| if( deprecated->IsActive() ) | ||
| { | ||
| deprecated->UpdateEffectParameters(); | ||
| if( deprecated->IsValid() ) | ||
| { | ||
| buckets.emplace_back( deprecated, PostProcessEnums::SCENE_DEFAULT_PRIORITY, 1.0f ); |
Comment on lines
+168
to
+182
| auto& desc = shader->GetEffectDescription(); | ||
| for( auto& technique : desc.techniques ) | ||
| { | ||
| for( auto& pass : technique.passes ) | ||
| { | ||
| for( auto& stage : pass.stageInputs ) | ||
| { | ||
| for( auto& constant : stage.constants ) | ||
| { | ||
| if( !IsBlendableParameter( constant, *shader ) ) | ||
| { | ||
| continue; | ||
| } | ||
| AddBlendableParameter( constant, reinterpret_cast<const uint8_t*>( stage.constantValues ) ); | ||
| } |
Comment on lines
+203
to
+204
| if( m_executionSlot != other.m_executionSlot || m_quality != other.m_quality ) | ||
| { |
|
|
||
| - `effect`: The shader effect used for this post-processing pass. | ||
| - `quality`: Minimal post-processing quality setting when this effect is visible. | ||
| - `execututionSlot`: Place in the post-processing pipeline where this effect should be executed. |
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
Add support for multiple custom post-processing effects. This feature expands on the previous implementation
of custom post-process effects that was limited to a single effect.
Custom post-process effects are now specified in post-process volumes. See documentation included in this PR
for more information.
https://fenriscreations.atlassian.net/browse/PLAT-11862