[FIX] Temp Update on Material Assignment #420
Conversation
Previously it was shader assignment by request or iterative order from URP to Unlit, but LLM usually assign Standard in a URP setting. This small fix introduce the helper to resolve some of the conflicts, while give warnings in the log to show if the user is creating a shader in a non-compatible setting.
|
Caution Review failedThe pull request is closed. WalkthroughAdds an internal RenderPipelineUtility to detect the active Unity render pipeline and resolve shaders (by name or alias) with pipeline-aware fallbacks and warnings; updates ManageAsset to use this utility and adjusts material property application accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Scriptwonder
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs (1)
151-193: Consider using McpLog.Warn for consistency.The warning logic is helpful, but lines 169, 173, 179, 183, and 189 use
Debug.LogWarningdirectly whileResolveShader(line 69) usesMcpLog.Warn. For consistency with the rest of the codebase (see MCPForUnity/Editor/Helpers/McpLog.cs), consider usingMcpLog.Warnthroughout this class.Apply this diff to use consistent logging:
case PipelineKind.HighDefinition: if (shaderLooksUrp) { - Debug.LogWarning($"[RenderPipelineUtility] Active pipeline is HDRP but shader '{shaderName}' looks URP-based. Asset may appear incorrect."); + McpLog.Warn($"Active pipeline is HDRP but shader '{shaderName}' looks URP-based. Asset may appear incorrect."); } else if (shaderLooksBuiltin && !shaderLooksHdrp) { - Debug.LogWarning($"[RenderPipelineUtility] Active pipeline is HDRP but shader '{shaderName}' looks Built-in. Consider using an HDRP shader for correct results."); + McpLog.Warn($"Active pipeline is HDRP but shader '{shaderName}' looks Built-in. Consider using an HDRP shader for correct results."); } break; case PipelineKind.Universal: if (shaderLooksHdrp) { - Debug.LogWarning($"[RenderPipelineUtility] Active pipeline is URP but shader '{shaderName}' looks HDRP-based. Asset may appear incorrect."); + McpLog.Warn($"Active pipeline is URP but shader '{shaderName}' looks HDRP-based. Asset may appear incorrect."); } else if (shaderLooksBuiltin && !shaderLooksUrp) { - Debug.LogWarning($"[RenderPipelineUtility] Active pipeline is URP but shader '{shaderName}' looks Built-in. Consider using a URP shader for correct results."); + McpLog.Warn($"Active pipeline is URP but shader '{shaderName}' looks Built-in. Consider using a URP shader for correct results."); } break; case PipelineKind.BuiltIn: if (shaderLooksSrp) { - Debug.LogWarning($"[RenderPipelineUtility] Active pipeline is Built-in but shader '{shaderName}' targets URP/HDRP. Asset may not render as expected."); + McpLog.Warn($"Active pipeline is Built-in but shader '{shaderName}' targets URP/HDRP. Asset may not render as expected."); } break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs(1 hunks)MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs.meta(1 hunks)MCPForUnity/Editor/Tools/ManageAsset.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Applied to files:
MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs.metaMCPForUnity/Editor/Helpers/RenderPipelineUtility.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Helpers/RenderPipelineUtility.csMCPForUnity/Editor/Tools/ManageAsset.cs
🧬 Code graph analysis (2)
MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Warn(43-46)Debug(31-35)
MCPForUnity/Editor/Tools/ManageAsset.cs (1)
MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs (6)
Shader(48-82)Shader(84-93)Shader(95-104)Shader(106-136)Shader(138-149)RenderPipelineUtility(7-194)
🔇 Additional comments (11)
MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs.meta (1)
1-11: LGTM!Standard Unity metadata file with correct format and structure.
MCPForUnity/Editor/Tools/ManageAsset.cs (3)
199-202: LGTM! Good improvement to shader resolution.The switch to
RenderPipelineUtility.ResolveShadercentralizes shader resolution logic and the improved error message provides helpful guidance about URP/HDRP installation or explicit shader paths.
206-218: LGTM! Well-structured property handling.The logic correctly clones and strips the shader field to prevent duplicate shader assignment, and only applies remaining properties when present. This is good defensive programming.
908-917: LGTM! Consistent shader resolution in modify path.The material modification flow now consistently uses
RenderPipelineUtility.ResolveShader, aligning with the creation flow for centralized shader resolution.MCPForUnity/Editor/Helpers/RenderPipelineUtility.cs (7)
7-15: LGTM! Clean utility class structure.The internal static class with a clear PipelineKind enum provides good encapsulation for render pipeline detection and shader resolution.
17-22: LGTM! Comprehensive shader name coverage.The shader name arrays provide good coverage of built-in, URP, and HDRP shaders with appropriate fallbacks for both lit and unlit variants.
24-46: LGTM! Solid pipeline detection logic.The implementation correctly detects the active render pipeline using case-insensitive type name matching, with appropriate fallbacks for built-in and custom pipelines.
48-82: LGTM! Robust shader resolution with excellent fallback strategy.The method implements a clear resolution precedence (alias → direct lookup → pipeline-specific defaults → built-in defaults) with appropriate warnings. The multi-level fallback ensures a shader is always returned.
84-104: LGTM! Pipeline-aware shader resolution with cross-pipeline fallbacks.Both methods provide appropriate fallback chains, ensuring shaders can be found even when the expected pipeline shaders aren't available.
106-136: LGTM! User-friendly alias resolution.The case-insensitive alias mapping provides intuitive names for common shader types, improving the developer experience when working with the API.
138-149: LGTM! Clean helper method.The
TryFindShaderhelper provides a convenient way to attempt multiple shader lookups with fallback, enhancing code readability throughout the utility.
|
@Scriptwonder this is great and important. I was just thinking about tackling this yesterday. Thanks! |
2 participants
Previously it was shader assignment by request or iterative order from URP to Unlit, but LLM usually assign Standard in a URP setting.
This small fix introduce the helper to resolve some of the conflicts, while give warnings in the log to show if the user is creating a shader in a non-compatible setting.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.