-
Notifications
You must be signed in to change notification settings - Fork 855
Do shadow code cleanup + Fix Yamato after PR #5 Multi compile shadow #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,12 @@ | |
|
||
#define DWORD_PER_TILE 16 // See dwordsPerTile in LightLoop.cs, we have roomm for 31 lights and a number of light value all store on 16 bit (ushort) | ||
|
||
// Some file may not required HD shadow context at all. In this case provide an empty one | ||
// Note: if a double defintion error occur it is likely have include HDShadow.hlsl (and so HDShadowContext.hlsl) after lightloopdef.hlsl | ||
#ifndef HAVE_HD_SHADOW_CONTEXT | ||
struct HDShadowContext { float unused; }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the purpose of this? The definition of shadow context will mismatch whatever is going to be used by the functions, so it won't compile anyway? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HDShadowContext is only define in HDShadowContext. (where I now define HAVE_HD_SHADOW_CONTEXT). Several files required to include LightloopDefs that require a shadow context so all those files include hdshadow.hlsl implicitely via lighting.hlsl whereas they don't need all those files. To remove those around 8 files dependency just to declare a HDShadowContext that is not used, we do this. (See the example of ContactShadows.compute where we can removed BTW, ContactShadow.compute was suppose to define a shadow mode because it was including the lighting.hlsl. BUT by luck (or badluck), the volumetric and voxelize test with this was define. Because SHADERPASS wasn't define... i.e lot of random behavior that I wanted to clean as none of this is used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, though unless I am missing something this now creates a dependency on the order of include so I guess that would need to be made more explicit in comments. Say I include HDShadowContext.hlsl after I include LightloopDef , I will double define the struct which I assume will not compile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. problem is already there, you can't do it the other way :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but before it would complain about a struct not being defined (so it is obvious you are missing an include), now it will complain about it being defined twice which is a bit less obvious. I would add a small comment in here that if the double defintion error occur is likely because you need to include HDShadowContext.hlsl beforehand There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a comment.
|
||
#endif | ||
|
||
// LightLoopContext is not visible from Material (user should not use these properties in Material file) | ||
// It allow the lightloop to have transmit sampling information (do we use atlas, or texture array etc...) | ||
struct LightLoopContext | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,31 +1,10 @@ | ||
// Various shadow algorithms | ||
// There are two variants provided, one takes the texture and sampler explicitly so they can be statically passed in. | ||
// The variant without resource parameters dynamically accesses the texture when sampling. | ||
|
||
// WARNINGS: | ||
// Keep in sync with HDShadowManager::GetDirectionalShadowAlgorithm() | ||
// Be careful this require to update GetPunctualFilterWidthInTexels() in C# as well! | ||
// Configure which shadow algorithms to use per shadow level quality | ||
|
||
// Since we use slope-scale bias, the constant bias is for now set as a small fixed value | ||
#define FIXED_UNIFORM_BIAS (1.0f / 65536.0f) | ||
|
||
#if (defined(_ENABLE_SHADOW_MATTE) && SHADERPASS == SHADERPASS_FORWARD_UNLIT) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does that work for shadow matte now? Unlit does not go through deferred.compute and I don't see the multi compile for it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See change in shader graph. Shadow matte generated from shader graph correctly generate a multi compile for shadow. (there is no shadow matte from unlit.shader, only from SG unlit). |
||
|
||
#if SHADEROPTIONS_DEFERRED_SHADOW_FILTERING == HDSHADOWFILTERINGQUALITY_LOW | ||
#define SHADOW_LOW | ||
#elif SHADEROPTIONS_DEFERRED_SHADOW_FILTERING == HDSHADOWFILTERINGQUALITY_MEDIUM | ||
#define SHADOW_MEDIUM | ||
#elif SHADEROPTIONS_DEFERRED_SHADOW_FILTERING == HDSHADOWFILTERINGQUALITY_HIGH | ||
#define SHADOW_HIGH | ||
#else | ||
#define SHADOW_MEDIUM | ||
#endif | ||
|
||
#endif | ||
|
||
#if (SHADERPASS == SHADERPASS_VOLUMETRIC_LIGHTING || SHADERPASS == SHADERPASS_VOLUME_VOXELIZATION) | ||
#define SHADOW_LOW | ||
#endif | ||
// WARNINGS: | ||
// Keep in sync with both HDShadowManager::GetDirectionalShadowAlgorithm() and GetPunctualFilterWidthInTexels() in C# as well! | ||
|
||
#ifdef SHADOW_LOW | ||
#define PUNCTUAL_FILTER_ALGORITHM(sd, posSS, posTC, tex, samp, bias) SampleShadow_PCF_Tent_3x3(_ShadowAtlasSize.zwxy, posTC, tex, samp, bias) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -203,6 +203,22 @@ void InitializeScreenSpaceShadows() | |
// Directional shadow material | ||
s_ScreenSpaceShadowsMat = CoreUtils.CreateEngineMaterial(screenSpaceShadowsShader); | ||
|
||
switch (m_Asset.currentPlatformRenderPipelineSettings.hdShadowInitParams.shadowFilteringQuality) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this init function called upon changing the value in the UI? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. Same as in lightloop (where we call the code in InizializeLightloop) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, any change in something in the HDRP asset will recreate the pipeline from scratch. |
||
{ | ||
case HDShadowFilteringQuality.Low: | ||
s_ScreenSpaceShadowsMat.EnableKeyword("SHADOW_LOW"); | ||
break; | ||
case HDShadowFilteringQuality.Medium: | ||
s_ScreenSpaceShadowsMat.EnableKeyword("SHADOW_MEDIUM"); | ||
break; | ||
case HDShadowFilteringQuality.High: | ||
s_ScreenSpaceShadowsMat.EnableKeyword("SHADOW_HIGH"); | ||
break; | ||
default: | ||
s_ScreenSpaceShadowsMat.EnableKeyword("SHADOW_MEDIUM"); | ||
break; | ||
} | ||
|
||
// Allocate the final result texture | ||
int numShadowTextures = Math.Max((int)Math.Ceiling(m_Asset.currentPlatformRenderPipelineSettings.hdShadowInitParams.maxScreenSpaceShadowSlots / 4.0f), 1); | ||
GraphicsFormat graphicsFormat = (GraphicsFormat)m_Asset.currentPlatformRenderPipelineSettings.hdShadowInitParams.screenSpaceShadowBufferFormat; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to the question below, aren't shadow matte unlit with the shadow functions called? If so, why undefining SHADOW_OPTIMIZE_REGISTER_USAGE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double define, wasn't required here. part of clode cleanup