Skip to content

[URP] Lit Shader Detail Fixes #5432

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
merged 9 commits into from
Sep 16, 2021
Merged

[URP] Lit Shader Detail Fixes #5432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ad11245
Fixed lit detail correctly upgraded from standard shader. [1323725]
lukaschod Aug 25, 2021
bdec597
Added warning for lit shader detailed abledo, if texture is not linea…
lukaschod Aug 25, 2021
95c3983
Updating changelog
lukaschod Aug 25, 2021
989685f
Adding missing namespace
lukaschod Aug 26, 2021
ff2cb4c
Merge branch 'master' of github.com:Unity-Technologies/Graphics into …
lukaschod Aug 27, 2021
e969dfe
Merge branch 'master' of github.com:Unity-Technologies/Graphics into …
lukaschod Sep 7, 2021
5b796c9
Updating changelog and extending comments
lukaschod Sep 7, 2021
41d3c06
Merge branch 'master' of github.com:Unity-Technologies/Graphics into …
lukaschod Sep 14, 2021
c962cd8
Merge branch 'master' into universal/detail-fixes
lukaschod Sep 16, 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
2 changes: 2 additions & 0 deletions com.unity.render-pipelines.universal/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Removed experimental tile deferred code.

### Fixed
- Added warning for lit shader detailed abledo, if texture is not linear. [1342011](https://issuetracker.unity3d.com/issues/detail-maps-packed-differently-in-built-in-vs-urp)
- Fixed lit detail correctly upgraded from standard shader. [1323725](https://issuetracker.unity3d.com/issues/urp-detail-map-tiling-is-tied-to-base-texture-tiling)
- URP asset can now use multi-edit. [case 1364966](https://issuetracker.unity3d.com/issues/urp-universalrenderpipelineasset-does-not-support-multi-edit)

## [12.0.0] - 2021-01-11
Expand Down
7 changes: 7 additions & 0 deletions com.unity.render-pipelines.universal/Editor/ShaderGUI/ShadingModels/LitDetailGUI.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using UnityEngine;
using UnityEngine.Rendering;
using UnityEngine.Experimental.Rendering;

namespace UnityEditor.Rendering.Universal.ShaderGUI
{
Expand All @@ -20,6 +21,7 @@ public static class Styles
"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.");
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

  1. What is wrong
  2. What is the context (how we got there)
  3. What is the effect/end result (for example, detail texturing is skipped until fixed).
  4. What is the correct/expected state
  5. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

public struct LitProperties
Expand Down Expand Up @@ -49,6 +51,11 @@ public static void DoDetailArea(LitProperties properties, MaterialEditor materia
{
EditorGUILayout.HelpBox(Styles.detailAlbedoMapScaleInfo.text, MessageType.Info, true);
}
var detailAlbedoTexture = properties.detailAlbedoMap.textureValue as Texture2D;
if (detailAlbedoTexture != null && GraphicsFormatUtility.IsSRGBFormat(detailAlbedoTexture.graphicsFormat))
{
EditorGUILayout.HelpBox(Styles.detailAlbedoMapFormatError.text, MessageType.Warning, true);
}
materialEditor.TexturePropertySingleLine(Styles.detailNormalMapText, properties.detailNormalMap,
properties.detailNormalMap.textureValue != null ? properties.detailNormalMapScale : null);
materialEditor.TextureScaleOffsetProperty(properties.detailAlbedoMap);
Expand Down
16 changes: 16 additions & 0 deletions com.unity.render-pipelines.universal/Editor/UniversalRenderPipelineMaterialUpgrader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ public static void UpdateStandardMaterialKeywords(Material material)
CoreUtils.SetKeyword(material, "_OCCLUSIONMAP", material.GetTexture("_OcclusionMap"));
CoreUtils.SetKeyword(material, "_METALLICSPECGLOSSMAP", material.GetTexture("_MetallicGlossMap"));
UpdateSurfaceTypeAndBlendMode(material);
UpdateDetailScaleOffset(material);
BaseShaderGUI.SetupMaterialBlendMode(material);
}

Expand All @@ -391,9 +392,24 @@ public static void UpdateStandardSpecularMaterialKeywords(Material material)
CoreUtils.SetKeyword(material, "_METALLICSPECGLOSSMAP", material.GetTexture("_SpecGlossMap"));
CoreUtils.SetKeyword(material, "_SPECULAR_SETUP", true);
UpdateSurfaceTypeAndBlendMode(material);
UpdateDetailScaleOffset(material);
BaseShaderGUI.SetupMaterialBlendMode(material);
}

static void UpdateDetailScaleOffset(Material material)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: it would be good to explain the upgrade behavior in comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added more info

{
// In URP details tile/offset is multipied with base tile/offset, where in builtin is not
// Basically we setup new tile/offset values that in shader they would result in same values as in builtin
// This archieved with inverted calculation where scale=detailScale/baseScale and tile=detailOffset-baseOffset*scale
var baseScale = material.GetTextureScale("_BaseMap");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this specific to our shaders? _BaseMap is attributed as [MainTexture] for us and default is of course _MainTexture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is specific from builtin->urp shader where tile/offset needs to be recalculated to retain same visual

var baseOffset = material.GetTextureOffset("_BaseMap");
var detailScale = material.GetTextureScale("_DetailAlbedoMap");
var detailOffset = material.GetTextureOffset("_DetailAlbedoMap");
var scale = new Vector2(baseScale.x == 0 ? 0 : detailScale.x / baseScale.x, baseScale.y == 0 ? 0 : detailScale.y / baseScale.y);
material.SetTextureScale("_DetailAlbedoMap", scale);
material.SetTextureOffset("_DetailAlbedoMap", new Vector2((detailOffset.x - baseOffset.x * scale.x), (detailOffset.y - baseOffset.y * scale.y)));
}

// Converts from legacy RenderingMode to new SurfaceType and BlendMode
static void UpdateSurfaceTypeAndBlendMode(Material material)
{
Expand Down