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

UnityObjectDestoyer 改め RuntimeGltfInstance #1021

Merged
merged 12 commits into from
Jun 11, 2021

Conversation

ousttrue
Copy link
Contributor

@ousttrue ousttrue commented Jun 10, 2021

#1018

  • DisposeOnGameObjectDestroyed を廃止して、Load 関数から UnityObjectManager が返るようになった。これを Destroy することで 関連リソースは開放される。
  • 引き換えに ScriptedImporter のときのみ、UnityObjectManager から AssetImporter に移動するようになった

新しい使い方は以下のような感じ。

            // import
            using (var context = new ImporterContext(parser))
            using (var loaded = context.Load()) // すぐ削除する場合。
            {
                loaded.ShowMeshes(); // ImorterContext から移動した
                loaded.EnableUpdateWhenOffscreen(); // ImorterContext から移動した
            }

インタフェース。デストロイすべき UnityEngine.Object をため込む

    public interface IResponsibilityForDestroyObjects : IDisposable
    {
        void TransferOwnership(TakeResponsibilityForDestroyObjectFunc take);
    }

@ousttrue ousttrue requested a review from Santarh June 10, 2021 11:23
@ousttrue ousttrue changed the title UnityObjectDestoyer 改め UnityObjectManager [WIP] UnityObjectDestoyer 改め UnityObjectManager Jun 10, 2021
@ousttrue ousttrue force-pushed the feature10/UnityObjectManager branch from 5efbcef to 81e02c8 Compare June 10, 2021 11:39
@ousttrue ousttrue changed the title [WIP] UnityObjectDestoyer 改め UnityObjectManager UnityObjectDestoyer 改め UnityObjectManager Jun 10, 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.

よさそう!
名称だけ…

@@ -21,7 +21,7 @@ public static void OnGUIAnimation(ScriptedImporter importer, GltfParser parser)
}
}

importer.DrawRemapGUI<AnimationClip>(parser.GLTF.animations.Select(x => new SubAssetKey(typeof(AnimationClip), x.name)));
importer.DrawRemapGUI<AnimationClip>(AnimationImporterUtil.EnumerateSubAssetKeys(parser.GLTF));
Copy link
Contributor

Choose a reason for hiding this comment

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

yosasou

{
public static class UnityObjectDestoyer
{
public static void DestroyRuntimeOrEditor(UnityEngine.Object o)
Copy link
Contributor

Choose a reason for hiding this comment

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

😄

/// TransferOwnership により、破棄責任を移譲することができる。
///
/// </summary>
public interface IResponsibilityForDestroyObjects : IDisposable
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

Choose a reason for hiding this comment

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

これも名前がもうひとひねりほしい気がするが、思いつかない……
長ったらしく書くと、このインタフェースを実装するクラスの責務は「所有権を持つ UnityEngine.Object を移譲することができる」だと思うのだが、う~ん…

/// <summary>
/// Mesh, Material, Texture などを抱えておいて確実に破棄できるようにする
/// </summary>
public class UnityObjectManager : MonoBehaviour, IResponsibilityForDestroyObjects
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

Choose a reason for hiding this comment

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

ユーザの期待としては Load API の返り値は「ロードした glTF/VRM モデルそのもの」だと思うので、そういう名称にしてしまっていいと思う。
ModelInstance とか glTFModel とか

Copy link
Contributor

Choose a reason for hiding this comment

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

インスタンスの変数名も loaded じゃなくて loadedModel とか model でよさそう


public void Dispose()
{
UnityObjectDestoyer.DestroyRuntimeOrEditor(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

this.gameObject を Destroy すべきかも

Copy link
Contributor Author

Choose a reason for hiding this comment

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

たしかに!

@Santarh
Copy link
Contributor

Santarh commented Jun 11, 2021

サンプルコードもいずれ合わせて要修正
https://vrm.dev/docs/univrm/programming/runtime_import/

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

@@ -61,7 +61,7 @@ public class ImporterContext : IResponsibilityForDestroyObjects
};

#region Load. Build unity objects
public virtual async Task<UnityObjectManager> LoadAsync(IAwaitCaller awaitCaller = null, Func<string, IDisposable> MeasureTime = null)
public virtual async Task<RuntimeGltfInstance> LoadAsync(IAwaitCaller awaitCaller = null, Func<string, IDisposable> MeasureTime = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

よさそう!

/// </summary>
public class UnityObjectManager : MonoBehaviour, IResponsibilityForDestroyObjects
public class RuntimeGltfInstance : MonoBehaviour, IResponsibilityForDestroyObjects
Copy link
Contributor

Choose a reason for hiding this comment

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

よさそう!

/// <summary>
/// this is UniGLTF root gameObject
/// </summary>
public GameObject Root => this.gameObject;
Copy link
Contributor

Choose a reason for hiding this comment

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

よさそう!

@@ -58,7 +64,7 @@ public void TransferOwnership(TakeResponsibilityForDestroyObjectFunc take)

public void Dispose()
{
UnityObjectDestoyer.DestroyRuntimeOrEditor(this);
UnityObjectDestoyer.DestroyRuntimeOrEditor(this.gameObject);
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 requested a review from Santarh June 11, 2021 08:21
@ousttrue ousttrue merged commit b871311 into vrm-c:master Jun 11, 2021
@ousttrue ousttrue changed the title UnityObjectDestoyer 改め UnityObjectManager UnityObjectDestoyer 改め RuntimeGltfInstance Jun 11, 2021
@ousttrue ousttrue deleted the feature10/UnityObjectManager branch October 27, 2021 06:00
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