Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ Shader "HDRP/TerrainLit"
#pragma multi_compile_instancing
#pragma instancing_options assumeuniformscaling nomatrices nolightprobe nolightmap

#pragma multi_compile _ _ALPHATEST_ON
#pragma multi_compile_local _ _ALPHATEST_ON

// All our shaders use same name for entry point
#pragma vertex Vert
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ Shader "Hidden/HDRP/TerrainLit_Basemap"
#pragma multi_compile_instancing
#pragma instancing_options assumeuniformscaling nomatrices nolightprobe nolightmap

#pragma multi_compile _ _ALPHATEST_ON
#pragma multi_compile_local _ _ALPHATEST_ON

#pragma vertex Vert
#pragma fragment Frag
Expand Down
2 changes: 2 additions & 0 deletions com.unity.shadergraph/Editor/Drawing/PreviewManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,8 @@ void BeginCompile(PreviewRenderData renderData, string shaderStr)
ShaderUtil.UpdateShaderAsset(shaderData.shader, shaderStr, false);
}

CoreUtils.Destroy(shaderData.mat);

Choose a reason for hiding this comment

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

@sebastienlagarde Why do we want to destroy the Material here?
Does this magically set shaderData.mat to null? Otherwise the material might get stuck in a "destroyed but not null" state...

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually got confused here and I thought it did (our HD util that destroys does set to null, while coreutils. seems not to).

I am curious on why it did not break though. Visually everything seems to work fine (and QA tested :/ )

On the why: it was a workaround that seemed to work as in the keywords were not queried wrongly but the preview material. This is something that was planned to be reverted as soon as the original bug gets fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will check tomorrow morning if adding the = null makes the workaround not work anymore :D

Choose a reason for hiding this comment

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

Make sure to test it with modifying the upstream graph of a preview node several times. That should cause this code to be called repeatedly.

Choose a reason for hiding this comment

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

I tested it locally, seems to work.
I think it's because of this:
https://forum.unity.com/threads/unitys-magic-null-check-not-properly-documented.408135/
"Destroyed" objects are "equal" to null.. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:D

Ok, to keep it clear I am gonna add the = null assignment anyway, but glad that nothing is broken.


if (shaderData.mat == null)
{
shaderData.mat = new Material(shaderData.shader) { hideFlags = HideFlags.HideAndDontSave };
Expand Down