Skip to content

[HOLD][case 1347209] Only process materials of the same SRP that the MaterialPostProcessor #5071

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

[HOLD][case 1347209] Only process materials of the same SRP that the MaterialPostProcessor #5071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e1dd52
Initial commit
jenniferd-unity Jul 6, 2021
2cd1ee3
Using glob search instead of type search
jenniferd-unity Jul 14, 2021
d5ece55
Merge branch 'master' into x-pipeline/1347209-infinite-import-loop-de…
jenniferd-unity Jul 19, 2021
ca0bac9
Fix for adding packages on empty project
jenniferd-unity Jul 21, 2021
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
9 changes: 4 additions & 5 deletions com.unity.render-pipelines.high-definition/Editor/AssetProcessors/MaterialPostProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class MaterialReimporter : Editor

static internal void ReimportAllMaterials()
{
string[] guids = AssetDatabase.FindAssets("t:material", null);
string[] guids = AssetDatabase.FindAssets($"glob:\"*.mat\"");
// There can be several materials subAssets per guid ( ie : FBX files ), remove duplicate guids.
var distinctGuids = guids.Distinct();

Expand Down Expand Up @@ -335,6 +335,9 @@ static void OnPostprocessAllAssets(string[] importedAssets, string[] deletedAsse

var material = (Material)AssetDatabase.LoadAssetAtPath(asset, typeof(Material));

if (!HDShaderUtils.IsHDRPShader(material.shader, upgradable: true))
continue;

if (MaterialReimporter.s_ReimportShaderGraphDependencyOnMaterialUpdate && GraphUtil.IsShaderGraphAsset(material.shader))
{
// Check first if the HDRP shadergraph assigned needs a migration:
Expand All @@ -359,10 +362,6 @@ static void OnPostprocessAllAssets(string[] importedAssets, string[] deletedAsse
}
}

if (!HDShaderUtils.IsHDRPShader(material.shader, upgradable: true))
continue;


void AddDiffusionProfileToSettings(string propName)
{
if (Application.isBatchMode || HDRenderPipelineGlobalSettings.instance == null
Expand Down
28 changes: 27 additions & 1 deletion com.unity.render-pipelines.universal/Editor/AssetPostProcessors/MaterialPostprocessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using UnityEngine;
using UnityEngine.Rendering.Universal;
using static Unity.Rendering.Universal.ShaderUtils;
using UnityEditor.Rendering.Universal.ShaderGraph;

namespace UnityEditor.Rendering.Universal
{
Expand All @@ -25,9 +26,27 @@ class MaterialReimporter : Editor
{
static bool s_NeedToCheckProjSettingExistence = true;


internal static bool IsURPShader(Shader shader)
Copy link
Contributor

@RSlysz RSlysz Jul 12, 2021

Choose a reason for hiding this comment

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

Should be part of ShaderUtils

Copy link
Contributor Author

@jenniferd-unity jenniferd-unity Jul 14, 2021

Choose a reason for hiding this comment

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

I would agree but in URP ShaderUtils is part of the Runtime portion and most of these function are Editor only (IsShaderGraphAsset for ex)

{
if (shader == null)
return false;

if (shader.IsShaderGraphAsset())
{
return shader.TryGetMetadataOfType<UniversalMetadata>(out _);
}
else if (ShaderUtils.IsLWShader(shader))
{
return true;
}

return shader.name.Contains("Universal Render Pipeline");
}

static void ReimportAllMaterials()
{
string[] guids = AssetDatabase.FindAssets("t:material", null);
string[] guids = AssetDatabase.FindAssets($"glob:\"*.mat\"");
// There can be several materials subAssets per guid ( ie : FBX files ), remove duplicate guids.
var distinctGuids = guids.Distinct();

Expand All @@ -38,6 +57,13 @@ static void ReimportAllMaterials()
materialIdx++;
var path = AssetDatabase.GUIDToAssetPath(asset);
EditorUtility.DisplayProgressBar("Material Upgrader re-import", string.Format("({0} of {1}) {2}", materialIdx, totalMaterials, path), (float)materialIdx / (float)totalMaterials);
var material = (Material)AssetDatabase.LoadAssetAtPath(asset, typeof(Material));
Copy link
Contributor

@RSlysz RSlysz Jul 12, 2021

Choose a reason for hiding this comment

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

Sadly this will slow up the import :/
Not sure how we can prevent it.

Copy link
Contributor Author

@jenniferd-unity jenniferd-unity Jul 12, 2021

Choose a reason for hiding this comment

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

@bastienunity do you have an idea on how i can properly enforce to process Materials from only the RenderPipeline I am interested in?

Copy link
Contributor

@bastienunity bastienunity Jul 12, 2021

Choose a reason for hiding this comment

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

I don't think you can as you have to load them in memory to figure out the shader.
However, there are probably some significant improvements that could be made in this method:

  • Don't use AssetDatabase.FindAssets("t:material", null); but AssetDatabase.FindAssets("glob:"*.mat""); instead. This will find guids only for .mat assets and will avoid loading in memory materials that you are not supposed to change.
  • What is AssetDatabase.ImportAsset doing? You probably want to do an AssetDatabase.SaveAssetIfDirty instead?
  • If Importers create Materials using a Shader that needs to be updated based on the SRP installed/active, then they need to declare a dependency upon it. We can discuss that separately on what is expected and how we can implement it. In any case, it's not viable to modify them after the import like that. They are never being saved back anywhere, and when unloaded they may be reverted to before your change (especially given the ModelImporter is always using a local caching now)

Copy link
Contributor Author

@jenniferd-unity jenniferd-unity Jul 14, 2021

Choose a reason for hiding this comment

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

Thanks!

  • I implemented the glob search suggestion,

  • i think we are retriggering import to ensure the Material Upgrader is run. I dont think SaveAsset will trigger this behavior. no?

  • i guess this point is out of scope for this bugfix but yes lets discuss it - do you agree?

Copy link
Contributor Author

@jenniferd-unity jenniferd-unity Jul 15, 2021

Choose a reason for hiding this comment

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

note: Bastien and I had a discussion there are some things I could implement for this PR.
cc @phi-lira

if (material == null || material.shader == null)
continue;
if (!IsURPShader(material.shader))
{
continue;
}
AssetDatabase.ImportAsset(path);
}
EditorUtility.ClearProgressBar();
Expand Down