Skip to content
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.

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

MToon10をURPに対応 #2054

Merged
merged 35 commits into from
May 9, 2023
Merged

MToon10をURPに対応 #2054

merged 35 commits into from
May 9, 2023

Conversation

notargs
Copy link
Contributor

@notargs notargs commented Apr 26, 2023

MToon10をURPに対応させたシェーダーである「vrmc_materials_mtoon_urp」を追加しました。

image
image

レビューしてほしい点

  • ifdefがかなり増えたが、メンテナンス性に支障がないかどうか
    • 今回は構造に大きく変更を入れず、ifdefを逐次入れることで対応をしたが、これにあわせてheaderファイルの構造の変更等をすればifdefが減らせる可能性はある
  • MTOON_URP, vrmc_materials_mtoon_urpなどのネーミングがどうか

@notargs notargs requested a review from Santarh April 26, 2023 07:46
@ousttrue ousttrue added the URP label Apr 26, 2023
return mtoon_linearstep(-1.0 + shadeToony, +1.0 - shadeToony, shadeInput + shadeShift);
}

half4 GetMToonURPAdditionalLighting(const UnityLighting unityLight, const MToonInput input)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

現状の構造では、単一のShaderPassから MToon_IsForwardBasePass() = true= false の両方を呼び分けることが出来ない
幸い、影響範囲はそれほど多くなかったので、一旦コードを複製することで対応(より良いアイデアがあれば教えてください)

Copy link
Contributor

Choose a reason for hiding this comment

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

Built-inRP/URP 両方あることを前提に IsForwardBasePass() 等の条件分岐そのものから全部見直せばいいかなとは思います。まあそれもまたいずれ…

@@ -32,7 +32,7 @@ MonoBehaviour:
m_MainLightShadowmapResolution: 2048
m_AdditionalLightsRenderingMode: 1
m_AdditionalLightsPerObjectLimit: 4
m_AdditionalLightShadowsSupported: 0
m_AdditionalLightShadowsSupported: 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

動作確認のため、AdditionalLightの影を有効化しておく

{
VertexPositionInfo output;
output.positionWS = mul(unity_ObjectToWorld, float4(positionOS, 1));
float4 positionCS = TransformWorldToHClip(ApplyShadowBias(output.positionWS, normalWS, _LightDirection));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ApplyShadowBiasが必要


#include "./vrmc_materials_mtoon_define.hlsl"
#include "./vrmc_materials_mtoon_utility.hlsl"
#include "./vrmc_materials_mtoon_input.hlsl"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CBufferは全てのPassで統一されている必要がある

m_Enabled: 1
m_EditorHideFlags: 0
m_Script: {fileID: 11500000, guid: 5b6d00cd3f0249a2b4ee7dc86f9a8d63, type: 3}
m_Name: MToonOutlineRenderFeature
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ScriptableRenderFeatrueとしてアウトラインを実装

@Santarh
Copy link
Contributor

Santarh commented May 8, 2023

以下解決ありがとうございます

  • URP Package を導入しているが選択していないときに、URP 版シェーダを使うと一見正常そうに見える
    • ピンク色にする
  • シェーダの名前空間及び命名
    • 他の Unity 公式シェーダの状況を見てすり合わせる(口頭解決)

次の問題はこの PR では解決せず Issue 送り

  • URP Package 非導入プロジェクトで、URP 版シェーダがコンパイルエラーになる

Copy link
Contributor

@Santarh Santarh left a comment

Choose a reason for hiding this comment

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

LGTM ありがとうございました。

この PR でやる必要のない要改善ポイントは以下

  • この PR では diff の見やすさとして ifdef を多用したが、ifdef をロジックに記述しない形でリファクタリングしたい
  • 半透明マテリアルの輪郭線の挙動が違う
  • URP Package 非導入プロジェクトで、URP 版シェーダがコンパイルエラーになる
  • 輪郭線に追加ライトのライティングを載せる

@@ -10,8 +10,10 @@ public sealed class UrpVrm10MaterialDescriptorGenerator : IMaterialDescriptorGen
{
public MaterialDescriptor Get(GltfData data, int i)
{
// mtoon
if (UrpVrm10MToonMaterialImporter.TryCreateParam(data, i, out var matDesc)) return matDesc;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines +35 to 37
output.normalWS = TransformObjectToWorldNormal(-v.normalOS);
#else
output.normalWS = UnityObjectToWorldNormal(-v.normalOS);
Copy link
Contributor

Choose a reason for hiding this comment

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

この辺はあとで TransformObjectToWorldNormal を Built-in 側だけで define して差異吸収する等のリファクタをしたい


#ifdef MTOON_URP
output.tangentWS = half4(TransformObjectToWorldDir(v.tangentOS), tangentSign);
#else
output.tangentWS = half4(UnityObjectToWorldDir(v.tangentOS), tangentSign);
Copy link
Contributor

Choose a reason for hiding this comment

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

同上

@@ -46,8 +55,22 @@ half4 MToonFragment(const FragmentInput fragmentInput) : SV_Target
mtoonInput.alpha = alpha;
half4 col = GetMToonLighting(unityLighting, mtoonInput);

#if defined(MTOON_URP) && defined(_ADDITIONAL_LIGHTS) && !defined(MTOON_PASS_OUTLINE)
Copy link
Contributor

Choose a reason for hiding this comment

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

URP 版は輪郭線に追加ライトのライティングを載せるコストが低めなので、それを有効にしてもいいかもなあ
→ Issue いき

Comment on lines +33 to +40
#ifdef MTOON_URP
half4 fogFactorAndVertexLight : TEXCOORD5; // x: fogFactor, yzw: vertex light

#if defined(REQUIRES_VERTEX_SHADOW_COORD_INTERPOLATOR)
float4 shadowCoord : TEXCOORD6;
#endif

DECLARE_LIGHTMAP_OR_SH(lightmapUV, vertexSH, 7);
Copy link
Contributor

Choose a reason for hiding this comment

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

URP Lit Forward 標準

@@ -0,0 +1,198 @@
Shader "VRM10/Universal Render Pipeline/MToon10"
Copy link
Contributor

Choose a reason for hiding this comment

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

VRM10/Universal Render Pipeline/MToon10

@@ -0,0 +1,198 @@
Shader "VRM10/Universal Render Pipeline/MToon10"
{
Properties
Copy link
Contributor

Choose a reason for hiding this comment

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

Props 等は共通


namespace VRMShaders.VRM10.MToon10.Runtime
{
public sealed class MToonOutlineRenderFeature : ScriptableRendererFeature
Copy link
Contributor

Choose a reason for hiding this comment

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

MToonOutlineRenderFeature

「URP 対応しました!」とリリースするときはこれの説明を書く必要がある

{
_pass = new MToonOutlineRenderPass
{
renderPassEvent = RenderPassEvent.AfterRenderingOpaques
Copy link
Contributor

Choose a reason for hiding this comment

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

半透明の場合、輪郭線描画に関して Built-in との挙動差になっている。

表示されるべきところでも表示されなかったりする場合があるので、要修正

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2061 にて対応


context.ExecuteCommandBuffer(cmd);
cmd.Clear();
CommandBufferPool.Release(cmd);
Copy link
Contributor

Choose a reason for hiding this comment

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

TagId を指定して得たパスを普通にレンダリングする RenderFeature

Copy link
Contributor

@Santarh Santarh left a comment

Choose a reason for hiding this comment

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

二重投稿になってしまった

@Santarh Santarh merged commit c088af1 into vrm-c:master May 9, 2023
1 check passed
@notargs notargs changed the title MToonをURPに対応 MToon10をURPに対応 May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants