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

[1.0] RuntimeLoad と meta 取得 を実装 #1196

Merged
merged 11 commits into from
Sep 8, 2021

Conversation

ousttrue
Copy link
Contributor

@ousttrue ousttrue commented Sep 8, 2021

#1190

  • 便利関数 GltfZipOrGlbFileParser を追加
  • サンプル SimpelViewer Vrm10Viewer を更新
  • migrate 時にオリジナルの情報を格納する Vrm0Meta を追加
  • 関連する docfx を更新

@ousttrue ousttrue added this to the v0.83 milestone Sep 8, 2021
@ousttrue ousttrue requested a review from Santarh September 8, 2021 08:14
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.

いくつか

/// <summary>
/// Parse file and detect file type by file extension.
/// </summary>
public sealed class GltfZipOrGlbFileParser
Copy link
Contributor

Choose a reason for hiding this comment

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

あー実はすでに AmbiguousGltfFileParser というのがありまして、それとダブってます

Copy link
Contributor

Choose a reason for hiding this comment

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

Ambiguous という命名自体はたしかに微妙さを感じているので、統合して自動判別GLTFパーサーみたいなクラス名にしたいっすね

@@ -79,7 +79,7 @@ public virtual async Task<RuntimeGltfInstance> LoadAsync(IAwaitCaller awaitCalle
{
if (awaitCaller == null)
{
awaitCaller = new TaskCaller();
awaitCaller = new ImmediateCaller();
Copy link
Contributor

Choose a reason for hiding this comment

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

yosi

public readonly Vrm10FileType FileType;
public readonly String Message;

public Vrm10Data(GltfData data, VRMC_vrm vrm, Vrm10FileType fileType, string message)
public Vrm10Data(GltfData data, VRMC_vrm vrm, Vrm10FileType fileType, string message, Migration.Vrm0Meta oldMeta = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

そもそもなぜ 0.x の meta 情報を保持するようになったんでしょう?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

新旧の meta に互換性が無いところがあるので、その部分を default 値(不許可)に変換することになります。
それに対する対策としてオリジナルにアクセスできるようにしようという感じです。

また、
vrm-c/vrm-specification#181
ということもあり新しいコンポーネントにロードするが vrm-1 になったわけではないという意味も兼ねます。

Copy link
Contributor

Choose a reason for hiding this comment

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

主たる理由は後者の、ライセンシーの問題ということで了解です

{
Data = data;
VrmExtension = vrm;
FileType = fileType;
Message = message;
OldMeta = oldMeta;
Copy link
Contributor

Choose a reason for hiding this comment

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

Old って…何?となるので VRM 0.x の Meta である、という命名が欲しい。
たとえば… MigratedVrm0xMeta とか

/// <summary>
/// 非同期実行
/// </summary>
public sealed class TaskCaller : IAwaitCaller
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.

迂闊に使うと危ない(デッドロックとか)可能性はあり。Immediate は安全。

@ousttrue ousttrue requested a review from Santarh September 8, 2021 08:47
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.

よさそう

/// </summary>
public sealed class AmbiguousGltfFileParser
public sealed class AutoGltfFileParser
Copy link
Contributor

Choose a reason for hiding this comment

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

良いと思います

@@ -18,7 +18,7 @@ public class Vrm10Data
{
public GltfData Data { get; }
public UniGLTF.Extensions.VRMC_vrm.VRMC_vrm VrmExtension { get; }
public readonly Migration.Vrm0Meta OldMeta;
public readonly Migration.Vrm0Meta OriginalMetaBeforeMigration;
Copy link
Contributor

Choose a reason for hiding this comment

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

よさげ

@ousttrue ousttrue merged commit a2307b4 into vrm-c:master Sep 8, 2021
@ousttrue ousttrue deleted the fix10/oldmeta_when_migrate branch October 27, 2021 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants