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

AvatarRoot API: Non-VRChat avatar support in VRCSDK projects #71

Closed
wants to merge 9 commits into from

Conversation

kaikoga
Copy link
Contributor

@kaikoga kaikoga commented Oct 29, 2023

Resolves #68, with avatar root detection logic described in the issue.

This is a potentially breaking change for VRCSDK + UniVRM environment. Current Non-VRChat avatar support (implemented in #34) is meant to not break existing NDMF plugins, which targets VRChat avatars only, in all projects including VRCSDK + UniVRM setup. This PR is the breaking part.

Explanation: VRM avatars were not AvatarRoots but would be. NDMF plugins expecting VRChat avatars may try to process VRM avatars, which would fail because VRCAvatarDescriptor does not exist. VRCSDK + UniVRM environment can be created using legacy UniVRM (0.99.4). info

User mitigation: Remove all VRM avatars, or all VRMMeta / Vrm10Instance components, when using VRChat only NDMF plugins in hybrid projects.

@kaikoga kaikoga marked this pull request as ready for review October 31, 2023 22:23
@bdunderscore
Copy link
Owner

Hmm. To resolve the compatibility issue, I think I'd prefer to have plugins declare what environments they're compatible with (with "vrchat" being the default). Let me noodle on this a bit and add that API first, before merging this.

@bdunderscore
Copy link
Owner

This will also need tests in a UniVRM environment... I'll have to look into auto-importing unitypackages.

@anatawa12
Copy link
Contributor

I think we should use upm version of univrm for testing since this pr expects upm version of univrm, not unitypackage one.

@kaikoga
Copy link
Contributor Author

kaikoga commented Nov 4, 2023

If ndmf is going to be aware of environments, perhaps we can warn if VRM version defines are not defined but installation is detected through AppDomain.CurrentDomain.GetAssemblies() (this should indicate a unitypackage installation)

@bdunderscore
Copy link
Owner

Sorry for the lack of replies - I haven't been able to do much development on NDMF/MA over the past few weeks. I'll hopefully be able to spend some more time on it in December.

@kaikoga
Copy link
Contributor Author

kaikoga commented Jan 27, 2024

Thoughts for this issue:

Theoretically, there may be three types of avatar setup when we want to create an avatar for VRC and VRM.
Which strategy should we support (or recommend)?

Setup as separate avatars

  • VRCAvatar (VRCDescriptor)
  • VRMAvatar (VRMMeta)

Pros: Currently available.
Cons: When VRC avatar (or MA outfit) is modified, manual sync to VRM avatar (or MA outfit) by hand.

Conversion plugin

  • VRCAvatar (VRCDescriptor)
    • → some ndmf plugin converts VRC components into VRM counterparts

Pros: Easy.
Cons: Requires development of conversion plugin, which may not be accurate enough or well customizable.

Universal avatar

  • VRCAvatar (VRCDescriptor) (VRMMeta)
    • → some ndmf plugin strips VRM components in VRC builds, vice versa.

Pros: Possible. Proof of concept ( docs_ja component ndmf pass )
Cons: Somewhat bloated avatar.

@@ -95,8 +101,17 @@ public static string AvatarRootPath(GameObject child)
public static bool IsAvatarRoot(Transform target)
Copy link
Owner

@bdunderscore bdunderscore Sep 1, 2024

Choose a reason for hiding this comment

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

こちらも要更新?(仮実装です)

#if NDMF_VRCSDK3_AVATARS
if (context.GetComponent<VRCAvatarDescriptor>(elem.gameObject) != null)
{
candidate = elem.gameObject;
break;
}
#else
if (context.GetComponent<Animator>(elem.gameObject) != null)
{
candidate = elem.gameObject;
}
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

そうかもしれません・・・
NDMF 1.5.0 リリース後に作業を再開する予定でしたが、この機会に全体を見直しておきます。

Copy link
Owner

Choose a reason for hiding this comment

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

了解です。

@kaikoga kaikoga marked this pull request as draft September 29, 2024 09:33
@@ -94,14 +100,23 @@ public static string AvatarRootPath(GameObject child)
/// <returns></returns>
public static bool IsAvatarRoot(Transform target)
{
// First, look for platform specific avatar descriptors
// TODO: ignore nested avatar descriptors?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

たぶん以下の扱いをちゃんと考えないといけない

  • VRCAvatarDescriptor の子に Vrm10Instance
    • VRCAvatarDescriptor をアバターとみなすべき
  • Vrm10Instance の子に VRCAvatarDescriptor
    • Vrm10Instance をアバターとみなすべき
  • Animator の子に VRCAvatarDescriptor / Vrm10Instance
    • バニラの Animator はアバターではないかもしれない
      • VRChatプロジェクトだと VRCAvatarDescriptor がアバターっぽく見える
      • Resoniteアバターがメインターゲットになるなら Animator がアバターになるべきケースもある
        • その時はたぶん VRCAvatarDescriptor を外してもらった方が良い
      • FindAvatarsInScene() みたいに全てのアバターかもしれない Animator をリストアップするとエラー UI などが不便になるかもしれない
      • FindAvatarRoots() は少し賢い
    • Animator が実は CVRAvatar だったケース
      • CVRAvatar をアバターとみなしたいけど、上との区別が無理
      • VRCAvatarDescriptor を外すか、 ndmf が CVRAvatar に対応するか、 AvatarRoot マーカーコンポーネントを定義してつけてもらうか・・・

Copy link
Contributor Author

Choose a reason for hiding this comment

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

考えました #432

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Animator の子に VRCAvatarDescriptor / Vrm10Instance

「アバターの外側に Animator をつけたい」というケースがない気がしました(外側の Animator はアバターの Humanoid ボーンや Animator パラメータを触ることができないので)

@kaikoga
Copy link
Contributor Author

kaikoga commented Oct 27, 2024

I have changed my mind, and would propose a more platform agnostic avatar root definition.

See #465.

@kaikoga kaikoga closed this Oct 27, 2024
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.

Improve RuntimeUtil.IsAvatarRoot()
3 participants