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

Feature/extract key #872

Merged
merged 6 commits into from
Apr 14, 2021
Merged

Feature/extract key #872

merged 6 commits into from
Apr 14, 2021

Conversation

ousttrue
Copy link
Contributor

@ousttrue ousttrue commented Apr 14, 2021

SubAsset を string レベルでユニークにする準備

ScriptedImporter で SubAsset を識別する SourceAssetIdentifier(Type, string) で、Type が違うけど string が同じ場合に警告が出る。
例えば、 alicia_body マテリアルに alicia_body.jpg テクスチャーがある場合に発生する。
(警告なので直さなくてもよいかもしれない)

これの対策で、string レベルでユニークにする準備。

TextureImportParam 簡略化

srgb, normal, standard に応じて、gltfname と convertedName を使い分けるのが分かりにくいので、
UnityObjectName に整理。

@ousttrue ousttrue requested a review from Santarh April 14, 2021 07:42
@ousttrue ousttrue added this to the v0.73 milestone Apr 14, 2021
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 but 何点か

/// の呼び出し時に、identifier.name と externalObject.name が同じでない運用にしてみる。
///
/// </summary>
public struct SubAssetKey
Copy link
Contributor

Choose a reason for hiding this comment

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

SubAssetKey

@@ -49,13 +49,13 @@ private enum BlendMode
Transparent
}

public static TextureImportParam BaseColorTexture(GltfParser parser, glTFMaterial src)
public static (SubAssetKey, TextureImportParam Param) BaseColorTexture(GltfParser parser, glTFMaterial src)
Copy link
Contributor

Choose a reason for hiding this comment

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

(SubAssetKey, TextureImportParam Param) 、もはや自明な型に定義してよさそうw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

struct SubAssetObject
{
public SubAssetKey Key;
public UnityEngine.Object Object;
}

的な。

@@ -173,11 +174,11 @@ public int CompareTo(ExpressionKey other)
return 0;
}

public string ExtractKey
public SubAssetKey ExtractKey
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename 漏れ?

@@ -13,7 +13,7 @@ namespace UniVRM10
[CreateAssetMenu(menuName = "VRM10/ExpressionAvatar")]
public sealed class VRM10ExpressionAvatar : ScriptableObject
{
public const string ExtractKey = "ExpressionAvatar";
public static UniGLTF.SubAssetKey ExtractKey => new UniGLTF.SubAssetKey(typeof(VRM10ExpressionAvatar), "ExpressionAvatar");
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename 漏れ?

@@ -10,7 +11,7 @@ namespace UniVRM10
[CreateAssetMenu(menuName = "VRM10/MetaObject")]
public class VRM10MetaObject : ScriptableObject
{
public const string ExtractKey = "Meta";
public static SubAssetKey ExtractKey => new SubAssetKey(typeof(VRM10MetaObject), "Meta");
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename 漏れ?

@ousttrue ousttrue requested a review from Santarh April 14, 2021 11:09
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.

よさそう

@ousttrue ousttrue merged commit ed0cfb1 into vrm-c:master Apr 14, 2021
@ousttrue ousttrue deleted the feature/extract_key branch May 17, 2021 05:21
@ousttrue ousttrue added the colorspace gamma(sRGB) / Linear label Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
colorspace gamma(sRGB) / Linear importer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants