-
Notifications
You must be signed in to change notification settings - Fork 433
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
[WebGL] fix sample webgl build and github-action #2561
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.
いくつか気になりポイントあります
@@ -99,7 +99,8 @@ public static async Task<Vrm10Instance> LoadBytesAsync( | |||
IMaterialDescriptorGenerator materialGenerator = null, | |||
VrmMetaInformationCallback vrmMetaInformationCallback = null, | |||
CancellationToken ct = default, | |||
ImporterContextSettings importerContextSettings = null) | |||
ImporterContextSettings importerContextSettings = null, | |||
IVrm10SpringBoneRuntime springboneRuntime = null) |
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.
Vrm10.LoadPathAsync()
には既に追加されている引数
Assets/VRM/Editor/BuildClass.cs
Outdated
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.
またいずれ後々、できればこういうクラスは Assets/Development
のほうに持っていって
ユーザには関係ないファイルは配布パッケージからは除きたいところですね
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.
最初 SimpleViewer/plugins を VRM10Viewer /plugins にコピーしてみたのだけど、
SimpleViewer/plugins と名前が衝突するということが起きました。
衝突しないように中身を変えるのが面倒だったので
共通のコピーひとつで済ませようかと思いました。
なので共通にしつつ、もっとユニークな名前に変えます。 WebGLFileDialog
=> UniVRM_Sample_WebGLFileDialog
など。
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.
Assets/UniGLTF/Runtime
に移動していますが
これは Sample だとまずそうな感じですか?
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.
Sample のリソースを共通で配置する場所が無いのでやむを得ず。
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.
Assets/UniGLTF/Editor の方がましかも?
Assets/UniGLTF/Editor/plugins に移動して UniVRM_Sample_WebGLFileDialog に rename |
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.
現在の Unity では プラグインは特にディレクトリ名に強制力はないので
Assets/UniGLTF/Plugins/
などを作ってそこに入れればいいかなと思います。
そのうえでプラグインファイル自体の Target Platform がちゃんと Rectrict できればいいかな、と。
ただし結局 Assets/UniGLTF/
以下に置く場合は、WebGL ビルドを行いたいライブラリ利用ユーザの手元に必ず入ってしまうファイルになってしまうので、あまりよくはないとは思います。
なのでどうしても入れざるを得ない場合は UniGLTF
などとプラグイン名に接頭辞をつける、等の措置を行った方がよいだろうとは思います。
結局そんな状況になるので、めんどくさいとは思いますが、各サンプルごとに別名でコピーしたほうがよいように思えます。
あと、もう VRM 0.X 向け (Assets/VRM) のサンプルはいらないんじゃないでしょうか?
コピーを作ってユニークな名前をつけました。 |
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.
LGTM ですが、meta ファイルがひとつだけ残ってしまってますね
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.
meta が残ってますね
SimpleViewer と VRM10Viewer の WebGL ビルドを整理しました。
合わせて WebGL ビルドを gh-pages に展開するアクション(SimpleViewer) を作成しました。
うまくいったら action は拡張予定です。
下記をこれの次で予定しています。