opt: reduce duplicate albedo calculations in URP#718
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors shader code to eliminate redundant calculations by introducing intermediate variables for commonly reused color computations. The changes improve code readability and maintainability by extracting repeated albedo calculations (_BaseColor * mainTex, _1st_ShadeColor * firstShadeTex, _2nd_ShadeColor * secondShadeTex) into named variables.
Changes:
- Introduced
baseAlbedo,firstShadeAlbedo, andsecondShadeAlbedovariables to store pre-calculated color values - Replaced all instances of redundant inline calculations with references to these new variables
- Removed an outdated comment
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| UniversalToonBodyShadingGradeMap.hlsl | Introduces albedo variables at the top of the fragment shader and refactors color calculations across forward base and additional light passes to use these variables |
| UniversalToonBodyDoubleShadeWithFeather.hlsl | Introduces albedo variables within the forward base pass and refactors color calculations across all lighting passes to use these variables |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
com.unity.toonshader/Runtime/UniversalRP/Shaders/UniversalToonBodyShadingGradeMap.hlsl
Outdated
Show resolved
Hide resolved
| const float3 baseAlbedo = _BaseColor.rgb * mainTex.rgb; | ||
| const float4 firstShadeTex = lerp(SAMPLE_TEXTURE2D(_1st_ShadeMap, sampler_MainTex, mainTexUV),mainTex, _Use_BaseAs1st); | ||
| float3 Set_1st_ShadeColor = lerp((_1st_ShadeColor.rgb * firstShadeTex.rgb),((_1st_ShadeColor.rgb * firstShadeTex.rgb) * Set_LightColor), _Is_LightColor_1st_Shade); | ||
| const float3 firstShadeAlbedo = _1st_ShadeColor.rgb * firstShadeTex.rgb; |
There was a problem hiding this comment.
Trailing whitespace should be removed from the end of this line.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| const float3 baseAlbedo = _BaseColor.rgb * mainTex.rgb; | ||
| const float4 firstShadeTex = lerp(SAMPLE_TEXTURE2D(_1st_ShadeMap, sampler_MainTex, mainTexUV),mainTex, _Use_BaseAs1st); | ||
| float3 Set_1st_ShadeColor = lerp((_1st_ShadeColor.rgb * firstShadeTex.rgb),((_1st_ShadeColor.rgb * firstShadeTex.rgb) * Set_LightColor), _Is_LightColor_1st_Shade); | ||
| const float3 firstShadeAlbedo = _1st_ShadeColor.rgb * firstShadeTex.rgb; | ||
| const float4 secondShadeTex = lerp(SAMPLE_TEXTURE2D(_2nd_ShadeMap, sampler_MainTex, mainTexUV),firstShadeTex, _Use_1stAs2nd); | ||
| float3 Set_2nd_ShadeColor = lerp((_2nd_ShadeColor.rgb * secondShadeTex.rgb),((_2nd_ShadeColor.rgb * secondShadeTex.rgb) * Set_LightColor), _Is_LightColor_2nd_Shade); | ||
| const float3 secondShadeAlbedo = _2nd_ShadeColor.rgb * secondShadeTex.rgb; |
There was a problem hiding this comment.
The albedo variables are declared within the _IS_PASS_FWDBASE block but are used in other code paths (lines 363, 365-366, 455, 457-458). These variables should be moved outside the #ifdef _IS_PASS_FWDBASE block to be accessible in all rendering passes, similar to how they are declared at the top of the function in UniversalToonBodyShadingGradeMap.hlsl.
…BodyShadingGradeMap.hlsl Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@sindharta-tanuwijaya I've opened a new pull request, #719, to work on those changes. Once the pull request is ready, I'll request review from you. |
sindharta
changed the title
No description provided.