Unity-Technologies / com.unity.toonshader

Unity Toon Shader ( an experimental package )
Other
1.07k stars 170 forks source link

Base Map missing Tiling & Offset controls #298

Closed vegmuffin closed 5 months ago

vegmuffin commented 1 year ago

Describe the bug The base diffuse map for the toon material is missing tiling & offset parameters, while Normal, MatCap and Emission maps have this control.

To Reproduce Steps to reproduce the behavior:

  1. Install toonshader package
  2. Create a new material based on the Toon shader
  3. Scroll down to Three Color and Control Map Settings

Expected behavior The Base Map and other shading maps should have tiling & offset controls.

Desktop (please complete the following information):

Additional context I don't know if this is how it should be, but it definitely is not ideal to not have tiling & offset controls, I want to be able to tile textures. :(

H3idi-X commented 1 year ago

Thanks for your feedback! We will adopt features of Unity, not just UTS, depending on the severity and number of requests. We currently receive few requests for this feature, but if we receive a large number of same requests, we will consider implementing this. Therefore, we will leave this issue here.

dustinpb97 commented 9 months ago

Hi, I'd also like the ability to tile textures on the toon shader.

Fabrazz commented 7 months ago

+1 Would be nice for those values to be public.

You can do this via Debug Inspector and we have been doing it that way for the entirety of our current project.

Hum9183 commented 7 months ago

@H3idi-X There seems to be a lot of requests for it, so I will try to implement it, but there is one problem. I have one problem: I think it would work if I add the following line, but I don't think it would be that cool from a GUI point of view. I would appreciate your feedback.

In UTS3GUI.cs

void GUI_BasicThreeColors(Material material)
{
    m_MaterialEditor.TexturePropertySingleLine(Styles.baseColorText, mainTex, baseColor);
    //v.2.0.7 Synchronize _Color to _BaseColor.
    if (material.HasProperty("_Color"))
    {
        material.SetColor("_Color", material.GetColor("_BaseColor"));
    }
    //

    EditorGUI.indentLevel += 2;
++  m_MaterialEditor.TextureScaleOffsetProperty(mainTex);
    var applyTo1st = GUI_Toggle(material, Styles.applyTo1stShademapText, ShaderPropUse_BaseAs1st, MaterialGetInt(material, ShaderPropUse_BaseAs1st) != 0);
    EditorGUI.indentLevel -= 2;

uts-basemap-st

Fabrazz commented 7 months ago

I think it looks perfectly fine!

H3idi-X commented 7 months ago

Understood. It would not be ideal to have you use Debug Inspector to deal with the situation. We can't use much time right now, but I will consider the solution.

H3idi-X commented 6 months ago

@Hum9183, I started implementing your fix, which is essentially correct. However, we need to apply the property to both the 1st and 2nd shading maps. I believe using one sampler for the Base, 1st shading map, and 2nd shading map textures would be a good approach. Implementing this across all the render pipeline shaders will take some time.

H3idi-X commented 6 months ago

Some are done, but some look not working. we are on investigation now.

one sampler for the Base, 1st shading map, and 2nd shading map textures would be a good approach

Hum9183 commented 6 months ago

@H3idi-X That is a good idea! If Tilling&Offset is to be shared between BaseMap, 1stShading and 2ndShading, it would be quicker to set _MainTex as the texture to be passed to TRANSFORM_TEX macro as shown below.

--  SAMPLE_TEXTURE2D(texture2d, sampler, TRANSFORM_TEX(uv, _1st_ShadeMap))
++  SAMPLE_TEXTURE2D(texture2d, sampler, TRANSFORM_TEX(uv, _MainTex))

I think the actual UTS can handle this by doing the following

In UniversalToonBodyDoubleShadeWithFeather.hlsl (line 142)

#ifdef _IS_PASS_FWDBASE
    float3 Set_LightColor = lightColor.rgb;
    float3 Set_BaseColor = lerp( (_BaseColor.rgb*_MainTex_var.rgb), ((_BaseColor.rgb*_MainTex_var.rgb)*Set_LightColor), _Is_LightColor_Base );
    //v.2.0.5
--  float4 _1st_ShadeMap_var = lerp(SAMPLE_TEXTURE2D(_1st_ShadeMap,sampler_MainTex, TRANSFORM_TEX(Set_UV0, _1st_ShadeMap)),_MainTex_var,_Use_BaseAs1st);
++  float4 _1st_ShadeMap_var = lerp(SAMPLE_TEXTURE2D(_1st_ShadeMap,sampler_MainTex, TRANSFORM_TEX(Set_UV0, _MainTex)),_MainTex_var,_Use_BaseAs1st);
    float3 Set_1st_ShadeColor = lerp( (_1st_ShadeColor.rgb*_1st_ShadeMap_var.rgb), ((_1st_ShadeColor.rgb*_1st_ShadeMap_var.rgb)*Set_LightColor), _Is_LightColor_1st_Shade );
    //v.2.0.5
--  float4 _2nd_ShadeMap_var = lerp(SAMPLE_TEXTURE2D(_2nd_ShadeMap, sampler_MainTex, TRANSFORM_TEX(Set_UV0, _2nd_ShadeMap)),_1st_ShadeMap_var,_Use_1stAs2nd);
++  float4 _2nd_ShadeMap_var = lerp(SAMPLE_TEXTURE2D(_2nd_ShadeMap, sampler_MainTex, TRANSFORM_TEX(Set_UV0, _MainTex)),_1st_ShadeMap_var,_Use_1stAs2nd);
    float3 Set_2nd_ShadeColor = lerp( (_2nd_ShadeColor.rgb*_2nd_ShadeMap_var.rgb), ((_2nd_ShadeColor.rgb*_2nd_ShadeMap_var.rgb)*Set_LightColor), _Is_LightColor_2nd_Shade );
    float _HalfLambert_var = 0.5*dot(lerp( i.normalDir, normalDirection, _Is_NormalMapToBase ),lightDirection)+0.5;
H3idi-X commented 6 months ago

Oh, sorry for the confusion. What I wanted to discuss was the lines where we use TRANSFORM_TEX. Thank you for the correction and suggestions.

Hum9183 commented 6 months ago

@H3idi-X Let's see... Am I right in saying that I want to keep the UVs of the _MainTex in a variable and use them in the 1stShading and 2ndShading calculations? My apologies if I misunderstood.

float2 mainTexUV = TRANSFORM_TEX(Set_UV0, _MainTex);
float4 _1st_ShadeMap_var = SAMPLE_TEXTURE2D(_1st_ShadeMap, sampler_MainTex, mainTexUV);
float4 _2nd_ShadeMap_var= SAMPLE_TEXTURE2D(_2nd_ShadeMap,  sampler_MainTex, mainTexUV);
H3idi-X commented 6 months ago

We adopted the formor ones for the fix. We are testing the shaders with graphics test systems.

H3idi-X commented 6 months ago

merged to the main branch.

H3idi-X commented 6 months ago

Hi, we released toon shader 0.9.7-preview this morning. Will you guys please try that one?

Fabrazz commented 6 months ago

Hey @H3idi-X! Thanks for the update! It works and looks good but I've noticed two issues:

  1. If you change the tiling value and undo, it doesn't update the values in the component to the previous ones until you deselect & reselect the material.
  2. Clipping Masks should also take that tiling into account otherwise they don't work.

Unity URP 2022.3.16f1

H3idi-X commented 6 months ago

Thanks for checking! We will take a look at the two issues above.

Fpro1383 commented 6 months ago

@H3idi-X There seems to be a lot of requests for it, so I will try to implement it, but there is one problem. I have one problem: I think it would work if I add the following line, but I don't think it would be that cool from a GUI point of view. I would appreciate your feedback.

In UTS3GUI.cs

void GUI_BasicThreeColors(Material material)
{
    m_MaterialEditor.TexturePropertySingleLine(Styles.baseColorText, mainTex, baseColor);
    //v.2.0.7 Synchronize _Color to _BaseColor.
    if (material.HasProperty("_Color"))
    {
        material.SetColor("_Color", material.GetColor("_BaseColor"));
    }
    //

    EditorGUI.indentLevel += 2;
++  m_MaterialEditor.TextureScaleOffsetProperty(mainTex);
    var applyTo1st = GUI_Toggle(material, Styles.applyTo1stShademapText, ShaderPropUse_BaseAs1st, MaterialGetInt(material, ShaderPropUse_BaseAs1st) != 0);
    EditorGUI.indentLevel -= 2;

uts-basemap-st

Hello, can we create a solution to add uvs functionality, for example, change the uv base color between uv0 and uv1 by changing the script in the shader?

H3idi-X commented 5 months ago

Hi, we have applied fixes for the second of the following issues to task/clippingMaskSampler branch. Could @Hum9183 and @Fabrazz try the branch if it doesn’t make too much trouble?

Fixing the first is difficult due to the design of the Unity Editor for now.

  1. If you change the tiling value and undo, it doesn't update the values in the component to the previous ones until you deselect & reselect the material.
  2. Clipping Masks should also take that tiling into account otherwise they don't work.
Hum9183 commented 5 months ago

@H3idi-X That sounds great! In addition, it may be a good idea to synchronize the ClippingMask of the outline

In UniversalToonOutlime.hlsl (lines 96-98)

#elif _IS_OUTLINE_CLIPPING_YES
-    float4 _ClippingMask_var = SAMPLE_TEXTURE2D(_ClippingMask, sampler_MainTex, TRANSFORM_TEX(Set_UV0, _ClippingMask));
+    float4 _ClippingMask_var = SAMPLE_TEXTURE2D(_ClippingMask, sampler_MainTex, TRANSFORM_TEX(Set_UV0, _MainTex));
     float Set_MainTexAlpha = _MainTex_var.a;
H3idi-X commented 5 months ago

Good point! We fix that. Thank you!

H3idi-X commented 5 months ago

@Hum9183 We applied your suggestion and did the same to the oehter RP shaders to the branch.

Hum9183 commented 5 months ago

@H3idi-X

  1. If you change the tiling value and undo, it doesn't update the values in the component to the previous ones until you deselect & reselect the material.

I have a simple idea to solve this problem. May I make a PR?

H3idi-X commented 5 months ago

Thanks for the suggestion. As long as it doesn't affect edtior peformance,the PR would be welcome. PR to task/clippingMaskSampler branch is ideal.

H3idi-X commented 5 months ago

Hi, we measured time cunsumed in FindProperties(props). It is negligible. We maerged PR by @Hum9183 and started our internal tests.

H3idi-X commented 5 months ago

All the tests passed and the branch is merged into the main branch.

H3idi-X commented 5 months ago

Hi, we released 0.10.0-preview that we applied the undo/redo fix by @Hum9183.

Fabrazz commented 5 months ago

Can confirm on my end, works as intended now! Thanks!