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

gltfのAnimationImporterをインターフェース化 #641

Merged
merged 2 commits into from
Dec 24, 2020

Conversation

hiroj
Copy link
Contributor

@hiroj hiroj commented Dec 24, 2020

  • 実装を差し替えられる形式に変更

@hiroj hiroj requested review from ousttrue and removed request for ousttrue December 24, 2020 08:39
@@ -316,6 +316,35 @@ public static int[] GetIndices(this glTF self, int accessorIndex)
return result;
}

public static float[] GetArrayFromAccessorAsFloat(this glTF self, int accessorIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

GetFloatArrayFromAccessor のほうが命名として素直そう。
ArrayFloat に cast するように読める。


namespace UniGLTF
{
public static class AnimationImporterUtil
Copy link
Contributor

Choose a reason for hiding this comment

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

こちらは AnimationImporter.cs の移植だと解釈します

@@ -0,0 +1,7 @@
namespace UniGLTF
{
public interface IAnimationImporter
Copy link
Contributor

Choose a reason for hiding this comment

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

よさそう

@@ -85,6 +85,26 @@ public string GetSpeedLog()
}
#endregion

#region Animation
IAnimationImporter m_animationImporter;
public void SetAnimationImporter(IAnimationImporter animationImporter)
Copy link
Contributor

Choose a reason for hiding this comment

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

インタフェース差し込みなら本来はコンストラクタにしたいが……
SetMaterialImporter() が既存にあるのでそれに合わせるという形でとりあえずよさそう。
ただし SetMaterialImporter()protected になっているのでそれに合わせたほうが良さそう。

@@ -0,0 +1,10 @@
namespace UniGLTF
{
public sealed class RootAnimationImporter : IAnimationImporter
Copy link
Contributor

Choose a reason for hiding this comment

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

よさそう

return string.Join("/", path);
}

public static AnimationClip ImportAnimationClip(ImporterContext ctx, glTFAnimation animation, glTFNode root = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Util ならば、Context といったデカい状態には依存せず、状態非依存の関数群になっていて欲しい気はします。
命名としても ConvertAnimationClip(hoge, fuga, piyo ... )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

必要なものはバッファアクセッサを持っているglTFクラスだけなので、glTFを引数にとって命名を変更します

return clip;
}

private static List<AnimationClip> ImportAnimationClips(ImporterContext ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

こちらも同様

{
public void Import(ImporterContext context)
{
AnimationImporterUtil.ImportAnimation(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

実装をすべて AnimationImporterUtil に丸投げしていますが
これは、外部の IAnimationImporter 実装でも AnimationImporterUtil の関数を使うから、ということで良いのでしょうか?
状態非依存の変換関数などは Util に投げて、手続き的処理の本質部分はこちらに書かれていたほうが読みやすくはありそうです。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Animationを構築する手続きはRootAnimationImporterに移動させます

@hiroj hiroj requested a review from Santarh December 24, 2020 09:18
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.

よさそう

@@ -316,7 +316,7 @@ public static int[] GetIndices(this glTF self, int accessorIndex)
return result;
}

public static float[] GetArrayFromAccessorAsFloat(this glTF self, int accessorIndex)
public static float[] GetFloatArrayFromAccessor(this glTF self, int accessorIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -185,7 +185,7 @@ private static string RelativePathFrom(List<glTFNode> nodes, glTFNode root, glTF
return string.Join("/", path);
}

public static AnimationClip ImportAnimationClip(ImporterContext ctx, glTFAnimation animation, glTFNode root = null)
public static AnimationClip ConvertAnimationClip(glTF gltf, glTFAnimation animation, glTFNode root = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -86,7 +86,7 @@ public string GetSpeedLog()
#endregion

#region Animation
IAnimationImporter m_animationImporter;
protected IAnimationImporter m_animationImporter;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

{
public sealed class RootAnimationImporter : IAnimationImporter
{
public void Import(ImporterContext context)
{
AnimationImporterUtil.ImportAnimation(context);
// animation
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return string.Join("/", path);
}

public static AnimationClip ConvertAnimationClip(glTF gltf, glTFAnimation animation, glTFNode root = null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

importerContextをglTFに変更

@hiroj hiroj merged commit 2c72e00 into vrm-c:master Dec 24, 2020
@ousttrue ousttrue mentioned this pull request Jan 25, 2021
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