[URP] Lit Shader Detail Fixes #5432
Conversation
|
Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed. URP Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure. |
|
It appears that you made a non-draft PR! |
…universal/detail-fixes # Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md
| - Fixed artifacts in Speed Tree 8 billboard LODs due to SpeedTree LOD smoothing/crossfading [case 1348407] | ||
| - Support undo of URP Global Settings asset assignation (case 1342987). | ||
| - Removed unsupported fields from Presets of Light and Camera [case 1335979]. | ||
| - Fixed an issue where Unlit and ParticlesUnlit shaders did not have HDR color selection for albedo [case 1283767](https://issuetracker.unity3d.com/issues/built-in-unlit-particle-shader-has-hdr-color-selection-for-albedo-urp-unlit-particles-do-not) |
| BaseShaderGUI.SetupMaterialBlendMode(material); | ||
| } | ||
|
|
||
| static void UpdateDetailScaleOffset(Material material) |
There was a problem hiding this comment.
minor: it would be good to explain the upgrade behavior in comment.
| "Designates a Normal Map to create the illusion of bumps and dents in the details of this Material's surface."); | ||
|
|
||
| public static readonly GUIContent detailAlbedoMapScaleInfo = EditorGUIUtility.TrTextContent("Setting the scaling factor to a value other than 1 results in a less performant shader variant."); | ||
| public static readonly GUIContent detailAlbedoMapFormatError = EditorGUIUtility.TrTextContent("This texture is not in linear space."); |
There was a problem hiding this comment.
I wonder if this is clear enough for our users. I think this might be confusing for some users, perhaps expand it a bit.
I think a good error message (is hard :) ) gives you the error and context (runtime parameters) and then points/guides you towards a likely fix.
- What is wrong
- What is the context (how we got there)
- What is the effect/end result (for example, detail texturing is skipped until fixed).
- What is the correct/expected state
- Suggestion how to fix it.
Can we give the current format of the texture in the message? so that you know what the format is when it's wrong and what it should be.
These were just my thoughts on this. No specific implementation requirements.
There was a problem hiding this comment.
This is infobox, so usually we do not go so deep in details for the sake of UX. I kinda copied this from normal texture info box. Do you have particular suggestion into what it should change for better readability?
There was a problem hiding this comment.
I also think some users will struggle with connecting linear space to the sRGB tick box. Maybe "This texture is marked as sRGB (Gamma space)"?
I think its ok to be a bit more clear since the "fix now" button is not available.
| static void UpdateDetailScaleOffset(Material material) | ||
| { | ||
| // In URP details tile/offset is tied to base map tile offset | ||
| var baseScale = material.GetTextureScale("_BaseMap"); |
There was a problem hiding this comment.
Is this specific to our shaders? _BaseMap is attributed as [MainTexture] for us and default is of course _MainTexture.
There was a problem hiding this comment.
It is specific from builtin->urp shader where tile/offset needs to be recalculated to retain same visual
…universal/detail-fixes
…universal/detail-fixes # Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md
|
Smoke test fails on master https://unity-ci.cds.internal.unity3d.com/job/8721614. Terrain issues instabilities URP_Foundation on Android_OpenGLES3_Standalone_il2cpp_Linear on version trunk - instability |
|
All failing issues happens on master too, merging. |
5 participants
Purpose of this PR
Testing status
Locally tested cases