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

Feat material unused property remove #1041

Merged

Conversation

ReinaS-64892
Copy link
Contributor

マテリアルにシェーダーの差し替えなどを行った時などに、テクスチャが全く使用されないけれどビルドに含まれる状態が発生し、場合によっては多くのVRAMが浪費されてしまいます。
この問題は、lilToonであればインスペクターから「未使用のテクスチャを外す」を使用すれば解決できますが、これは挙動が変わらない軽量化なので、T&O で自動で行えると非常に良いと思います!

このプルリクエストではlilToonの物を T&O に 組み込む形で実装し、マテリアルから未使用のテクスチャが削除されます。
ですが...マテリアルの置き換えアニメーションによって処理前の物に置き換えられる問題や、マテリアルの置き換えアニメーションにしか参照が存在しないマテリアルが処理できていません。

処理できていない...とはいっても、私にできると思われる範囲を大幅に超えているので、この機能があったほうが良いと思うのであれば、そのあたりの実装をお願いしたいです。

Copy link
Owner

@anatawa12 anatawa12 left a comment

Choose a reason for hiding this comment

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

提案ありがとうございます。雑(?)に消すか、シェーダの中身も理解して消すかを考えてて未実装でした。

抽象的な点で2点指摘させてください。

  1. この機能自体はremove unused objectsの1機能でいいと思うのでその中にし、skipRemoveMaterialUnusedPropertiesをadvanced settings内に追加してください
  2. SkinnedMeshRendererについてはMeshInfo2の方で置き換える必要があります。

アニメーションの方は別途 別PR で対応を検討します

@anatawa12 anatawa12 added this to the v1.8.0 milestone May 2, 2024
@ReinaS-64892
Copy link
Contributor Author

ReinaS-64892 commented May 2, 2024

前者をとりあえずやってみましたが...後者の

SkinnedMeshRendererについてはMeshInfo2の方で置き換える必要があります。

というのはいったいどうすればいいでしょうか...?
マテリアル周りの情報は、MeshInfo2 にないように見えます。あるのであればどこにあるのか教えてください!

@anatawa12
Copy link
Owner

マテリアルはMeshInfo2のSubMeshの中に配列になって入っています

if (state.SkipRemoveMaterialUnusedProperties) { return; }

var renderers = context.GetComponents<Renderer>();
var swapDict = renderers.SelectMany(i => i.sharedMaterials)
Copy link
Owner

Choose a reason for hiding this comment

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

こっちでもMeshInfo2使わないといけないですね。

アニメーションの対応を今後追加することを考えたらSwapMaterialArray(に相当する関数)内で必要に応じてMaterialCleaningを呼び出したいです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

前者は修正、MaterialCleaningは呼べるようにアクセス装飾氏を調整しました。

少し気になるのですが...MaterialCleaningをアニメーション関連で呼ぶ場合に、ここのswapDictを何かしらの手段で渡さないと、MaterialCleaningされた別の同一マテリアルが発生し、マテリアルが増加する可能性があると思うので、その時に適宜調整する必要があると思います。

Copy link
Owner

Choose a reason for hiding this comment

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

アニメーションに関する処理もこのPassないでやるつもりです。

そして意図が伝わってなかったのですが先にすべてのマテリアルをDictionaryに追加するのではなく、適宜Dictionaryにあればそれを、なければ作成して追加の形にしてほしいという意味です

@ReinaS-64892 ReinaS-64892 marked this pull request as ready for review May 4, 2024 10:06
@anatawa12 anatawa12 added the enhancement New feature or request label May 4, 2024
@ReinaS-64892
Copy link
Contributor Author

少し忘れていたけど...今の状態だとアニメーションがあると普通に壊れる状態ですが、CHANGLOGを書いておいた方がいいですか?

@anatawa12
Copy link
Owner

アニメーションがあると壊れます?単に無駄に生成されるだけではない感じですか

@ReinaS-64892
Copy link
Contributor Author

たしかに壊れるというより、ビルドに含まれるマテリアルが増えるだけで済むから問題ないのか、少し勘違いしてた。

//https://github.com/lilxyzw/lilToon/blob/b96470d3dd9092b840052578048b2307fe6d8786/Assets/lilToon/Editor/lilMaterialUtils.cs#L658-L686
//
//https://light11.hatenadiary.com/entry/2018/12/04/224253
public static void RemoveUnusedProperties(Material material)
Copy link
Owner

Choose a reason for hiding this comment

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

そういえばfallback shaderの対応を考え忘れてる気がする

よっぽどのことがないともとシェーダにないプロパティが関係あることないと思うけど

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

カスタム指定ができるliltoonなんかでもOverrideTagでどうにかしてるので、VRCFallbackが指定されてる場合にそれに対応するシェーダのプロパティをどっかで持っておいて、それを下に消さないプロパティを決めると良さそう

@anatawa12
Copy link
Owner

Optimize Textureのためにマテリアルの複製処理が入ったので、inplaceでやるように書き換えたほうが良さそうですね。私が引き継ぐつもりはありますが Reina さんは更新を行う意思がありますか

@ReinaS-64892
Copy link
Contributor Author

そんなないけど、時間が足りてないならやるよ

@anatawa12
Copy link
Owner

時間は足りていないので、やっていただけるとありがたいです。

@ReinaS-64892
Copy link
Contributor Author

🆗

@ReinaS-64892
Copy link
Contributor Author

そういえば、書くの忘れてたけど In Place でクリーニングするように書き換えてみたよ!

@anatawa12
Copy link
Owner

ありがとうございます。アニメーションの対応入れて少し修正したらマージします

@anatawa12
Copy link
Owner

動作確認も取れたのでマージします〜

image image

@anatawa12 anatawa12 merged commit d0e9ba1 into anatawa12:master Oct 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants