Merge pull request #1399 from anatawa12/fix-processing-path-with-slash

fix: animations for paths with '/' in GameObject name is broken

CHANGELOG-PRERELEASE.md

@@ -16,6 +16,7 @@ The format is based on [Keep a Changelog].
 ### Removed
 
 ### Fixed
+- Animations targets GameObjects includes '/' in their name can be broken `#1399`
 
 ### Security
 

CHANGELOG.md

@@ -16,6 +16,7 @@ The format is based on [Keep a Changelog].
 ### Removed
 
 ### Fixed
+- Animations targets GameObjects includes '/' in their name can be broken `#1399`
 
 ### Security
 

Editor/APIInternal/ComponentInfos.VRCSDK.cs

@@ -55,8 +55,8 @@ protected override void CollectDependency(T component,
                     var avatarRoot = component.gameObject;
                     foreach (var parent in hips.ParentEnumerable(avatarRoot.transform))
                     {
-                        var path = RuntimeUtil.RelativePath(avatarRoot, parent.gameObject);
-                        var parentByPath = avatarRoot.transform.Find(path);
+                        var path = RuntimeUtil.RelativePath(avatarRoot, parent.gameObject)!;
+                        var parentByPath = Utils.ResolveAnimationPath(avatarRoot.transform, path);
                         collector.AddDependency(parentByPath).EvenIfDependantDisabled();
                     }
                 }

Editor/AnimatorParserV2/AnimationParser.cs

@@ -129,7 +129,7 @@ public static ImmutableNodeContainer ParseAnimation(GameObject root, AnimationCl
             Profiler.BeginSample("AnimationParser.ProcessFloatNodes");
             foreach (var binding in floatBindings)
             {
-                var obj = AnimationUtility.GetAnimatedObject(root, binding);
+                var obj = Utils.GetAnimatedObject(root, binding);
                 if (obj == null) continue;
                 var componentOrGameObject = obj is Component component ? (ComponentOrGameObject)component
                     : obj is GameObject gameObject ? (ComponentOrGameObject)gameObject
@@ -152,7 +152,7 @@ public static ImmutableNodeContainer ParseAnimation(GameObject root, AnimationCl
             Profiler.BeginSample("ProcessObjectNodes");
             foreach (var binding in objectBindings)
             {
-                var obj = AnimationUtility.GetAnimatedObject(root, binding);
+                var obj = Utils.GetAnimatedObject(root, binding);
                 if (obj == null) continue;
                 var componentOrGameObject = obj is Component component ? (ComponentOrGameObject)component
                     : obj is GameObject gameObject ? (ComponentOrGameObject)gameObject

Editor/ObjectMapping/ObjectMapping.cs

@@ -116,32 +116,8 @@ public void InitializeRecursive()
         public BeforeGameObjectTree? ResolvePath(string relative) =>
             relative == "" ? this : ResolvePathAll(relative).FirstOrDefault();
 
-        private IEnumerable<BeforeGameObjectTree> ResolvePathAll(string relative)
-        {
-            if (relative == "")
-                return new[] { this };
-            // otherwise, match as possible from start
-
-            // simplest
-            var slashIndex = relative.IndexOf('/');
-
-            if (slashIndex == -1)
-                return Children.Where(x => x.Name == relative);
-
-            for (;slashIndex != -1; slashIndex = relative.IndexOf('/', slashIndex + 1))
-            {
-                var name = relative.Substring(0, slashIndex);
-
-                if (Children.Any(x => x.Name == name))
-                {
-                    var remaining = relative.Substring(slashIndex + 1);
-
-                    return Children.Where(x => x.Name == name).SelectMany(x => x.ResolvePathAll(remaining));
-                }
-            }
-
-            return Array.Empty<BeforeGameObjectTree>();
-        }
+        private IEnumerable<BeforeGameObjectTree> ResolvePathAll(string relative) =>
+            Utils.ResolveAnimationPath(this, relative, (tree, path) => tree.Children.Where(x => x.Name == path));
     }
 
     class ComponentInfo

Internal/AnimatorOptimizer/AnyStateToEntryExit.cs

@@ -176,7 +176,7 @@ private static bool CanConvert(AOAnimatorControllerLayer layer,
                 static bool IsMeaningfulBinding(AnimationClip clip, EditorCurveBinding binding, GameObject? rootGameObject)
                 {
                     if (rootGameObject == null) return false; // we can't check no-op without rootGameObject
-                    var component = AnimationUtility.GetAnimatedObject(rootGameObject, binding);
+                    var component = Utils.GetAnimatedObject(rootGameObject, binding);
                     if (component == null) return true; // target is not exist: no-op
 
                     if (binding.isPPtrCurve)

Internal/Utils/Utils.cs

@@ -9,6 +9,7 @@
 using Unity.Collections.LowLevel.Unsafe;
 using UnityEditor;
 using UnityEngine;
+using Object = UnityEngine.Object;
 
 namespace Anatawa12.AvatarOptimizer
 {
@@ -440,5 +441,64 @@ public static TSource MaxBy<TSource, TComparable>(this IEnumerable<TSource> sour
 
             return max;
         }
+
+        /// <summary>
+        /// The helper function to resolve the animation path.
+        ///
+        /// The <see cref="AnimationUtility.GetAnimatedObject"/> and <see cref="Transform.Find"/> does not handle
+        /// game objects with slash in their name, however, animator component can have slash in their name.
+        ///
+        /// Therefore, this function resolves the animation path with slash in the name.
+        ///
+        /// This function is generic over T because we use same logic for both Transform and ObjectMapping system
+        /// </summary>
+        /// <returns>Enumerable that resolves to list of game objects matching the path</returns>
+        public static IEnumerable<T> ResolveAnimationPath<T>(
+            T root,
+            string relative,
+            Func<T, string, IEnumerable<T>> findChild
+        ) {
+            // if relative path is empty, return itself
+            if (relative == "")
+                return new[] { root };
+            // otherwise, match as possible from start
+
+            // simplest pattern: the relative is entire path
+            var slashIndex = relative.IndexOf('/');
+            if (slashIndex == -1)
+                return findChild(root, relative);
+
+            // other patterns: the relative path has slash, so we tries to match from start
+            for (;slashIndex != -1; slashIndex = relative.IndexOf('/', slashIndex + 1))
+            {
+                var name = relative[..slashIndex];
+
+                var childrenMatched = findChild(root, name);
+                // ReSharper disable once PossibleMultipleEnumeration : single element is proceed
+                if (childrenMatched.Any())
+                {
+                    var remaining = relative[(slashIndex + 1)..];
+                    // ReSharper disable once PossibleMultipleEnumeration
+                    return childrenMatched.SelectMany(x => ResolveAnimationPath(x, remaining, findChild));
+                }
+            }
+
+            // no subpath matched, so we try to match the entire path
+            return findChild(root, relative);
+        }
+
+        public static Transform? ResolveAnimationPath(Transform root, string path) =>
+            ResolveAnimationPath(root, path, (transform, name) => 
+                Enumerable.Range(0, transform.childCount)
+                    .Select(transform.GetChild)
+                    .Where(x => x.name == name))
+                .FirstOrDefault();
+
+        public static Object? GetAnimatedObject(GameObject obj, EditorCurveBinding binding)
+        {
+            var gameObject = ResolveAnimationPath(obj.transform, binding.path);
+            if (gameObject == null) return null;
+            return binding.type == typeof(GameObject) ? gameObject.gameObject : gameObject.GetComponent(binding.type);
+        }
     }
 }

Test~/Basic/ObjectMappingTest.cs

@@ -73,6 +73,33 @@ public void PathResolution(string testPath)
                 $"Expected {(transform ? transform.name : "null")} but was {resolvedName}");
         }
 
+        [Test]
+        public void ValidPathResolutionWithSlash()
+        {
+            var root = Utils.NewGameObject("root");
+            var childWithSlash = Utils.NewGameObject("child/with/slash", root.transform);
+            var son = Utils.NewGameObject("son", childWithSlash.transform);
+
+            var builder = new ObjectMappingBuilder<DummyPropInfo>(root).BuildObjectMapping().GetBeforeGameObjectTree(root);
+
+            Assert.That(builder.ResolvePath("child/with/slash")?.InstanceId, Is.EqualTo(childWithSlash.GetInstanceID()));
+            Assert.That(builder.ResolvePath("child/with/slash/son")?.InstanceId, Is.EqualTo(son.GetInstanceID()));
+
+            Assert.That(Utils.ResolveAnimationPath(root.transform, "child/with/slash")?.GetInstanceID(), Is.EqualTo(childWithSlash.transform.GetInstanceID()));
+            Assert.That(Utils.ResolveAnimationPath(root.transform, "child/with/slash/son")?.GetInstanceID(), Is.EqualTo(son.transform.GetInstanceID()));
+
+            // tests for Unity's problem
+            Assert.That(root.transform.Find("child/with/slash"), Is.Null);
+            Assert.That(root.transform.Find("child/with/slash/son"), Is.Null);
+
+            Assert.That(AnimationUtility.GetAnimatedObject(root, MakeBinding("child/with/slash")), Is.Null);
+            Assert.That(AnimationUtility.GetAnimatedObject(root, MakeBinding("child/with/slash/son")), Is.Null);
+            return;
+
+            EditorCurveBinding MakeBinding(string path) =>
+                EditorCurveBinding.FloatCurve(path, typeof(GameObject), "m_IsActive");
+        }
+
         [Test]
         public void MoveObjectTest()
         {