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

Feature10/refactor materialimporter #739

Merged
merged 7 commits into from
Feb 10, 2021

Conversation

ousttrue
Copy link
Contributor

#732 の準備

  • MaterialFactory を導入し、 ImporterContext から Material, Texture 関連を委譲
  • MaterialImporter を解体
    • UnlitMaterialItem, PBRMaterialItem, MToonMaterialItem に責務を委譲
  • IShaderStore を削除(UniUnlit 導入により仕事が無くなった)

@ousttrue ousttrue requested review from Santarh and hiroj February 10, 2021 11:29
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.

作業途中ということでよし

namespace UniGLTF
{
public delegate TextureItem GetTextureItemFunc(int i);
public delegate Shader GetShaderFunc();
Copy link
Contributor

Choose a reason for hiding this comment

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

使ってない?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GetShaderFunc IShaderStore が思ったより消えて不要になってました。
次で消しておきます。

public delegate MaterialItemBase MaterialImporter(int i, glTFMaterial x, bool hasVertexColor);


public class MaterialFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

MaterialFactory 自体が中間生成物の UnityEngine.Object を生成する可能性があるので
コレ自体が IDisposable である必要はありそう

@ousttrue ousttrue merged commit 95a5dd5 into vrm-c:master Feb 10, 2021
@ousttrue ousttrue deleted the feature10/refactor_materialimporter branch April 2, 2021 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants