Skip to content

refactor: reduce reliance on shader-name lookup when converting legacy toon shaders #789

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

Merged
sindharta merged 13 commits into from
Apr 13, 2026
Merged

refactor: reduce reliance on shader-name lookup when converting legacy toon shaders #789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
eb9f9c8
remove unused constants
sindharta Apr 8, 2026
3665a47
add constants
sindharta Apr 8, 2026
545c6d2
update assembly
sindharta Apr 8, 2026
bde1cfe
update test code
sindharta Apr 8, 2026
1ba97e7
make sure the path is linked
sindharta Apr 8, 2026
c57482b
Fix
sindharta Apr 8, 2026
832536b
MaterialForConversion
sindharta Apr 8, 2026
dc5c39f
rename
sindharta Apr 8, 2026
ed6b43b
update assembly
sindharta Apr 8, 2026
c4d8d46
change guids
sindharta Apr 8, 2026
c0e832d
Revert "change guids"
sindharta Apr 8, 2026
f9d2ee8
Revert "rename"
sindharta Apr 8, 2026
20c44f3
Revert "MaterialForConversion"
sindharta Apr 8, 2026
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
2 changes: 2 additions & 0 deletions com.unity.toonshader/Editor/AssemblyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,6 @@
[assembly: InternalsVisibleTo("Unity.VisualCompositor.Editor")]
[assembly: InternalsVisibleTo("Unity.VisualCompositor.EditorTests")]

[assembly: InternalsVisibleTo("Unity.ToonShader.EditorTests")]
Comment thread
sindharta marked this conversation as resolved.


3 changes: 1 addition & 2 deletions com.unity.toonshader/Editor/Converter/FromUTS2/BuiltInUTS2toIntegratedConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,7 @@ void ConvertBuiltInUTS2Materials( string[] guids)
string content = File.ReadAllText(path);
string[] lines = content.Split(lineSeparators, StringSplitOptions.None);
int renderQueueInMaterial = GetRenderQueue(path, lines);
var shaderID = GetShaderIDinMaterial(path);
material.shader = Shader.Find(IsTesselationShader(path) ? kIntegratedTessllationUTS3Name : kIntegratedUTS3Name);
material.shader = GetOrLoadToonShader(IsTesselationShader(path));
var shaderGUID = m_Material2GUID_Dictionary[material];
var UTS2Info = m_GuidToUTSID_Dictionary[shaderGUID] as UTS2INFO;

Expand Down
34 changes: 27 additions & 7 deletions com.unity.toonshader/Editor/Converter/RenderPipelineConverterContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,7 @@ public enum InstalledStatus {
protected List<string> m_ConvertingMaterialGuids = new List<string>();
protected Dictionary<Material, string> m_Material2GUID_Dictionary = new Dictionary<Material, string>();
protected Dictionary<string, UTSGUID> m_GuidToUTSID_Dictionary = new Dictionary<string, UTSGUID>();
protected const string kIntegratedUTS3Name = "Toon/Toon";
protected const string kIntegratedUTS3GUID = "be891319084e9d147b09d89e80ce60e0";

protected const string kIntegratedTessllationUTS3Name = "Toon/Toon (Tessellation)";
protected const string kIntegratedTessllationUTS3GUID = "e4468eb8a8320f7488ddbb0e591f9fbc";

protected const string kShaderKeywordInMatrial = " m_Shader:";

protected static string packageFullPath { get; set; }
Expand Down Expand Up @@ -166,8 +162,7 @@ protected void CommonConvert() {
renderQueue = GetRenderQueue(path, lines);
// deal with RenderType
string renderType = GetRenderType(path, lines);
material.shader = Shader.Find(IsTesselationShader(path) ? kIntegratedTessllationUTS3Name : kIntegratedUTS3Name);

material.shader = GetOrLoadToonShader(IsTesselationShader(path));
material.renderQueue = renderQueue;
if (renderType != null) {
material.SetOverrideTag("RenderType", renderType);
Expand Down Expand Up @@ -243,5 +238,30 @@ public void AddMaterialToScrollview(Material material) {
protected void SendAnalyticsEvent() {
AnalyticsSender.SendEventInEditor(new ToonShaderAnalytics.ConvertEvent(GetType().Name));
}

//----------------------------------------------------------------------------------------------------------------------

protected Shader GetOrLoadToonShader(bool tess) {
if (tess) {
return GetOrLoadToonShader(ref m_toonTessShader, ToonEditorConstants.TOON_TESS_SHADER_PATH);
}

return GetOrLoadToonShader(ref m_toonShader, ToonEditorConstants.TOON_SHADER_PATH);

}

Shader GetOrLoadToonShader(ref Shader shader, string path) {
if (null != shader)
return shader;

shader = AssetDatabase.LoadAssetAtPath<Shader>(path);
return shader;
}
Comment on lines +244 to +259
Copy link

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

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

GetOrLoadToonShader can return null when the shader asset can't be loaded at the expected package path; the caller then assigns material.shader = null, which can break subsequent conversion steps. Add an explicit null check after LoadAssetAtPath (log an actionable error including the path/material), and consider a fallback to Shader.Find using the known shader names to preserve behavior if assets are relocated/missing.

Suggested change
protected Shader GetOrLoadToonShader(bool tess) {
if (tess) {
return GetOrLoadToonShader(ref m_toonTessShader, ToonEditorConstants.TOON_TESS_SHADER_PATH);
}
return GetOrLoadToonShader(ref m_toonShader, ToonEditorConstants.TOON_SHADER_PATH);
}
Shader GetOrLoadToonShader(ref Shader shader, string path) {
if (null != shader)
return shader;
shader = AssetDatabase.LoadAssetAtPath<Shader>(path);
return shader;
}
protected Shader GetOrLoadToonShader(bool tess, Material material = null) {
if (tess) {
return GetOrLoadToonShader(ref m_toonTessShader, ToonEditorConstants.TOON_TESS_SHADER_PATH, material);
}
return GetOrLoadToonShader(ref m_toonShader, ToonEditorConstants.TOON_SHADER_PATH, material);
}
Shader GetOrLoadToonShader(ref Shader shader, string path, Material material = null) {
if (null != shader)
return shader;
shader = AssetDatabase.LoadAssetAtPath<Shader>(path);
if (null != shader)
return shader;
string shaderFileName = Path.GetFileNameWithoutExtension(path);
shader = FindShaderAssetByFileName(shaderFileName);
if (null != shader) {
Debug.LogWarning($"Toon shader could not be loaded from expected path '{path}' for material '{(material != null ? material.name : "<unknown>")}'. Using relocated shader asset '{shader.name}' found by filename lookup instead.");
return shader;
}
shader = Shader.Find(shaderFileName);
if (null != shader) {
Debug.LogWarning($"Toon shader could not be loaded from expected path '{path}' for material '{(material != null ? material.name : "<unknown>")}'. Using Shader.Find('{shaderFileName}') as a fallback.");
return shader;
}
Debug.LogError($"Failed to load Toon shader from expected path '{path}' for material '{(material != null ? material.name : "<unknown>")}'. Ensure the shader asset exists at that path or pass the correct shader reference to the converter.");
return null;
}
private Shader FindShaderAssetByFileName(string shaderFileName) {
string[] guids = AssetDatabase.FindAssets($"{shaderFileName} t:Shader");
foreach (string guid in guids) {
string candidatePath = AssetDatabase.GUIDToAssetPath(guid);
if (!string.Equals(Path.GetFileNameWithoutExtension(candidatePath), shaderFileName, StringComparison.OrdinalIgnoreCase))
continue;
Shader candidateShader = AssetDatabase.LoadAssetAtPath<Shader>(candidatePath);
if (null != candidateShader)
return candidateShader;
}
return null;
}

Copilot uses AI. Check for mistakes.

//----------------------------------------------------------------------------------------------------------------------

private Shader m_toonShader;
private Shader m_toonTessShader;

}
}
7 changes: 7 additions & 0 deletions com.unity.toonshader/Editor/Scripts/ToonEditorConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ internal static class ToonEditorConstants {
internal const int CUR_MATERIAL_VERSION = (int) ToonMaterialVersion.Initial;

internal static readonly string PACKAGE_PATH = Path.Combine("Packages", ToonConstants.PACKAGE_NAME).Replace('\\','/');


internal static readonly string TOON_SHADER_PATH =
Path.Combine(PACKAGE_PATH,"Runtime/Shaders/UnityToon.shader").Replace('\\','/');
internal static readonly string TOON_TESS_SHADER_PATH =
Path.Combine(PACKAGE_PATH,"Runtime/Shaders/UnityToonTessellation.shader").Replace('\\','/');


}

Expand Down
21 changes: 9 additions & 12 deletions com.unity.toonshader/Tests/Editor/ShaderCompileTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,27 @@
using System.IO;
using NUnit.Framework;
using UnityEditor;
using UnityEngine;
using Unity.Rendering.Toon;
using UnityEditor.Rendering.Toon;

namespace Unity.ToonShader.EditorTests {
internal class ShaderCompileTest
{
[Test]
public void CompileToonShaders() {
string[] guids = AssetDatabase.FindAssets("t:Shader", new[] { SHADERS_PATH});
int numShaders = guids.Length;
string[] shaderPaths = {
ToonEditorConstants.TOON_SHADER_PATH,
ToonEditorConstants.TOON_TESS_SHADER_PATH,
};
int numShaders = shaderPaths.Length;
Assert.Greater(numShaders,0);


for (int i=0;i<numShaders;++i) {
string curAssetPath = AssetDatabase.GUIDToAssetPath(guids[i]);
string directory = Path.GetDirectoryName(curAssetPath).Replace('\\','/');
string curAssetPath = shaderPaths[i];

// Exclude shaders in subfolders
if (directory != SHADERS_PATH) {
continue;
}
Shader shader = AssetDatabase.LoadAssetAtPath<Shader>(curAssetPath);
Assert.IsNotNull(shader);

AssetDatabase.ImportAsset(curAssetPath); //Recompile the shader to make sure there are no compile errors

Assert.True(shader.isSupported);
Expand All @@ -33,8 +32,6 @@ public void CompileToonShaders() {

//----------------------------------------------------------------------------------------------------------------------

private static readonly string SHADERS_PATH =
Path.Combine("Packages", ToonConstants.PACKAGE_NAME,"Runtime/Shaders").Replace('\\','/');

}

Expand Down
3 changes: 2 additions & 1 deletion com.unity.toonshader/Tests/Editor/Unity.ToonShader.EditorTests.asmdef
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
"UnityEngine.TestRunner",
"UnityEditor.TestRunner",
"Unity.Toonshader",
"Unity.FilmInternalUtilities.Editor"
"Unity.FilmInternalUtilities.Editor",
"Unity.Toonshader.Editor"
],
"includePlatforms": [
"Editor"
Expand Down