-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: Just check Animators for finding avatar root #465
Conversation
f0f8224
to
17f64ab
Compare
9b9d7d4
to
18147ea
Compare
if (context.GetComponent<Animator>(elem.gameObject) != null) | ||
{ | ||
candidate = elem.gameObject; | ||
if (RuntimeUtil.IsAvatarRoot(elem)) break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to observe the animator component (context.GetComponent<Animator>
or similar) as well as any properties used to determine if the animator is used as an animator root.
It's also preferable to avoid observing the same object multiple times, as each observation registers a callback used to determine if something changed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like essentially same as adding / removing VRCAvatarDescriptor in the middle of avatar hierarchy, so I need a strict (not a heuristic) definition of what is an avatar to continue working on this change.
Also, NDMF Avatar Root (as in #432 ) is not what I can actually start implementing without prior detailed design, because the change will certainly break ResoniteImportHelper and Chillaxins, and breaking non-VRChat support for a long time is not what I want to do.
Closing, because we need a different approach. |
A simpler resolution that closes #68, also ref: #432
I thought this would be breaking change if there were Animator components in the parents of VRCAvatarDescriptor.
However:
So I would propose just checking Animators, because I suppose it is safe to assume that an avatar has an outermost Animator at its root object.
It also aligns with the current behavior of
RuntimeUtil.FindAvatarRoots()
, as it ignores nested candidates.Drawbacks: Slight cost of
RuntimeUtil.IsAvatarRoot()
, andRuntimeUtil.FindAvatarInParents()
if there are nested Animators. (I think this cost is unavoidable after all.)I think it can be optimized if we can agree on "outer Animator takes precedence" rule.
(If this cost is unacceptable, so is generic Animator support and avatar root marker component would become mandatory...)
Versus avatar root component type registry: Less overengineering, and less encouraging adding a dummy VRCAvatarDescriptor.