Skip to content

Conversation

gmitrano-unity
Copy link
Contributor

@gmitrano-unity gmitrano-unity commented Nov 4, 2021

Purpose of this PR

This change merges the FSR RCAS logic into the FinalPost shader to avoid
an unnecessary round trip through memory.

This saves about 200us of frame time (compared to the prior implementation) at 4k on PS5 (Spaceship Demo) with scaling forced to 77%.


Testing status

Manual testing locally on PC DX12 & DX11, HDRP DRS tests on DX12 & DX11, PS5 Spaceship Demo with scaling

@github-actions
Copy link

github-actions bot commented Nov 4, 2021

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@gmitrano-unity gmitrano-unity changed the title Merge FSR RCAS Logic into FinalPass [HDRP] Merge FSR RCAS Logic into FinalPass Nov 4, 2021
@github-actions github-actions bot added the HDRP label Nov 4, 2021
This change merges the FSR RCAS logic into the FinalPost shader to avoid
an unnecessary round trip through memory.
@gmitrano-unity gmitrano-unity marked this pull request as ready for review November 4, 2021 16:09
@gmitrano-unity gmitrano-unity requested a review from kecho November 4, 2021 16:09
Copy link
Contributor

@sebastienlagarde sebastienlagarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, please add a changelog

@sebastienlagarde
Copy link
Contributor

@kecho btw we haven't re-enable the dynamic res graphic test right? so we don't have test allowing to check the change?

@kecho
Copy link
Contributor

kecho commented Nov 5, 2021

@sebastienlagarde we have only disabled the DRS vulkan tests from what I see
image

We were waiting on trunk to fix this. I think the fix landed. Otherwise, dx12, dx11 and metal should be tested properly.

#ifdef CATMULL_ROM_4
CTYPE outColor = UpscaledResult(positionNDC.xy);
#elif defined(RCAS)
CTYPE outColor = ApplyRCAS(scaledPositionSS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ApplyRCAS returns a float3, yet you have a CTYPE. This means that you could potentially be setting alpha 0 and causing a shader compiler warning (float3 casted into float4).

Notice how the other passes use the CTYPE_SWIZZE to address this. Can you dig in a bit into support for alpha? I think this might break this pass.

Copy link
Contributor Author

@gmitrano-unity gmitrano-unity Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a new commit that adds some comments explaining how this is handled:

#define FSR_INPUT_TEXTURE _InputTexture
#define FSR_INPUT_SAMPLER s_linear_clamp_sampler
#if ENABLE_ALPHA
// When alpha is in use, activate the alpha-passthrough mode in the RCAS implementation.
// When this mode is active, ApplyRCAS returns a four component vector (rgba) instead of a three component vector (rgb).
#define FSR_ENABLE_ALPHA 1
#endif
#include "Packages/com.unity.render-pipelines.core/Runtime/PostProcessing/Shaders/FSRCommon.hlsl"

Any idea if this functionality is tested in the DRS tests?

@sebastienlagarde sebastienlagarde merged commit 8b5ec1d into master Nov 7, 2021
@sebastienlagarde sebastienlagarde deleted the hdrp/merged-rcas branch November 7, 2021 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants