-
Notifications
You must be signed in to change notification settings - Fork 436
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
_MainTex_ST の参照を修正 #2573
_MainTex_ST の参照を修正 #2573
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いくつか命名面でのポイントが気になりました
@@ -9,6 +10,8 @@ public class VRM10VisualPrimitive : MonoBehaviour | |||
{ | |||
[SerializeField] private PrimitiveType _primitiveType; | |||
|
|||
[SerializeField] private Material _urpMaterial; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
できれば「WebGL 用マテリアル」の意味合いの名称の方が良いと思います。
URP 自体への対応はこのクラスは元からできています。
そのうえで WebGL でのみ例外的処理を行うという意味合いだと思うので。
未来にコードを読んだときに、混乱しそうです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WebGL に限らずビルドした場合に要るような気がしてきた。
実験します。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build すると desktop でも要るようです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
現状だとたしかにそうですね!
|
||
namespace UniVRM10.VRM10Viewer | ||
{ | ||
public class CustomMaterialContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この名称だと汎用的すぎて、特別な意味を感じません。
未来にコードを読んだときに混乱しそうです。
このクラスは、ある一意なシェーダに対応する操作コンテキストになると思うので、シェーダ名を一意なものに決めるべきだなと感じます。
たとえばシェーダ名を TinyPbr
などと命名したと仮定して
CustomMaterialContext
は TinyPbrContext
CustomMaterialDescriptorGenerator
は ForcedTinyPbrMaterialDescriptorGenerator
等の命名がふさわしく感じます。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
もしくはこのクラスが「カスタムな任意のシェーダを割り当てる用の便利クラスなんだ」という定義なのであれば
このクラスの summary コメントに「このコンテキストに与えるマテリアルのシェーダは以下の命名規則に従っていることを期待する」等のコメントが欲しいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
命名規則のガイドラインを考えます。
ユニーク(namespaceに頼らない)なシェーダー名を最初に決める。
TinyPbr に改名しました。 WebGL 環境での動確に伴って ShaderGrfaph 製カスタムシェーダーの導入を兼ねました。 |
[Header("UI")] | ||
[SerializeField] | ||
Toggle m_useCustomMaterial = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VRM10Viewer の CustomMaterial として TinyPbr を使う感じ
using UnityEngine; | ||
|
||
namespace UniVRM10.VRM10Viewer | ||
{ | ||
/// <summary> | ||
/// GLTF の MaterialImporter | ||
/// </summary> | ||
public sealed class CustomMaterialDescriptorGenerator : IMaterialDescriptorGenerator | ||
public sealed class TinyPbrDescriptorGenerator : IMaterialDescriptorGenerator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あ、すみません。
あくまで MaterialDescriptorGenerator
なので TinyPbrMaterialDescriptorGenerator
が正しいと思います
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
次でなおしま 🙏
PreviewMaterialItem に残存していた "_MainTex_ST" 参照を修正しました。
また、 VRM10Viewer サンプルで ShaderGraph Material の動確をしました。
Expose した Texture2D プロパティに対する
Set as Main Texture
が必要です。_MainTex
ではなく_BaseMap
名で動作しました。MainTexture flag が無いと出るエラーメッセージ