code review fixes

vkovec · vkovec · commit c31e7e6d1010 · 2017-10-16T16:24:10.000-04:00
-remove KeepOriginal
-don't instantiate FBX or modify any transforms/sibling indices
-don't abreviate "Renderer"
diff --git a/Assets/FbxExporters/Editor/ConvertToModel.cs b/Assets/FbxExporters/Editor/ConvertToModel.cs
@@ -62,16 +62,6 @@ public static bool OnValidateMenuItem ()
                 return true;
             }
 
-            /// <summary>
-            /// After a convert-to-model, we normally delete the original object.
-            /// That can be overridden.
-            /// </summary>
-            public enum KeepOriginal {
-                Default, // Use the export settings.
-                Keep, // Don't delete, just rename it.
-                Delete, // Delete the original hierarchy.
-            }
-
             /// <summary>
             /// Gets the export settings.
             /// </summary>
@@ -89,11 +79,9 @@ public static EditorTools.ExportSettings ExportSettings {
             /// <returns>list of instanced Model Prefabs</returns>
             /// <param name="unityGameObjectsToConvert">Unity game objects to convert to Model Prefab instances</param>
             /// <param name="path">Path to save Model Prefab; use FbxExportSettings if null</param>
-            /// <param name="keepOriginal">If set to <c>true</c> keep original gameobject hierarchy.</param>
             public static GameObject[] CreateInstantiatedModelPrefab (
                 GameObject [] unityGameObjectsToConvert,
-                string directoryFullPath = null,
-                KeepOriginal keepOriginal = KeepOriginal.Default)
+                string directoryFullPath = null)
             {
                 if (directoryFullPath == null) {
                     directoryFullPath = FbxExporters.EditorTools.ExportSettings.GetAbsoluteSavePath();
@@ -106,8 +94,7 @@ public static GameObject[] CreateInstantiatedModelPrefab (
                 foreach(var go in toExport) {
                     try {
                         wasExported.Add(Convert(go,
-                            directoryFullPath: directoryFullPath,
-                            keepOriginal: keepOriginal));
+                            directoryFullPath: directoryFullPath));
                     } catch(System.Exception xcp) {
                         Debug.LogException(xcp);
                     }
@@ -121,8 +108,6 @@ public static GameObject[] CreateInstantiatedModelPrefab (
             /// This returns a new object; the converted object may be modified or destroyed.
             ///
             /// This refreshes the asset database.
-            ///
-            /// If "keepOriginal" is set, the converted object is modified but remains in the scene.
             /// </summary>
             /// <returns>The instance that replaces 'toConvert' in the scene.</returns>
             /// <param name="toConvert">GameObject hierarchy to replace with a prefab.</param>
@@ -134,12 +119,10 @@ public static GameObject[] CreateInstantiatedModelPrefab (
             /// to a directory in which to put the fbx file. Ignored if
             /// fbxFullPath is specified. May be null, in which case we use the
             /// export settings.</param>
-            /// <param name="keepOriginal">If set to <c>true</c>, keep the original in the scene.</param>
             public static GameObject Convert (
                 GameObject toConvert,
                 string directoryFullPath = null,
-                string fbxFullPath = null,
-                KeepOriginal keepOriginal = KeepOriginal.Default)
+                string fbxFullPath = null)
             {
                 if (string.IsNullOrEmpty(fbxFullPath)) {
                     // Generate a unique filename.
@@ -182,68 +165,28 @@ public static GameObject Convert (
                     throw new System.Exception ("Failed to convert " + toConvert.name);;
                 }
 
-                // Instantiate the FBX file.
-                var unityGO = PrefabUtility.InstantiatePrefab (unityMainAsset, toConvert.scene)
-                    as GameObject;
-                if (!unityGO) {
-                    throw new System.Exception ("Failed to convert " + toConvert.name);;
-                }
-
-                // Copy the components over to the instance of the FBX.
-                SetupImportedGameObject (toConvert, unityGO);
-
-                // temporary quick fix
-                var temp = toConvert;
-                toConvert = unityGO;
-                unityGO = temp;
+                // Copy the mesh/materials from the FBX
+                UpdateFromFBX (toConvert, unityMainAsset);
 
                 // Set up the FbxPrefab component so it will auto-update.
                 // Make sure to delete whatever FbxPrefab history we had.
-                var fbxPrefab = unityGO.GetComponent<FbxPrefab>();
+                var fbxPrefab = toConvert.GetComponent<FbxPrefab>();
                 if (fbxPrefab) {
                     Object.DestroyImmediate(fbxPrefab);
                 }
-                fbxPrefab = unityGO.AddComponent<FbxPrefab>();
+                fbxPrefab = toConvert.AddComponent<FbxPrefab>();
                 fbxPrefab.SetSourceModel(unityMainAsset);
 
-                // Disconnect from the FBX file.
-                PrefabUtility.DisconnectPrefabInstance(unityGO);
-
                 // Create a prefab from the instantiated and componentized unityGO.
                 var prefabFileName = Path.ChangeExtension(projectRelativePath, ".prefab");
-                var prefab = PrefabUtility.CreatePrefab(prefabFileName, unityGO);
+                var prefab = PrefabUtility.CreatePrefab(prefabFileName, toConvert, ReplacePrefabOptions.ConnectToPrefab);
                 if (!prefab) {
                     throw new System.Exception(
                         string.Format("Failed to create prefab asset in [{0}] from fbx [{1}]",
                             prefabFileName, fbxFullPath));
                 }
 
-                // Connect to the prefab file.
-                unityGO = PrefabUtility.ConnectGameObjectToPrefab(unityGO, prefab);
-
-                // Remove (now redundant) gameobject
-                bool actuallyKeepOriginal;
-                switch(keepOriginal) {
-                case KeepOriginal.Delete:
-                    actuallyKeepOriginal = false;
-                    break;
-                case KeepOriginal.Keep:
-                    actuallyKeepOriginal = true;
-                    break;
-                case KeepOriginal.Default:
-                default:
-                    actuallyKeepOriginal = ExportSettings.keepOriginalAfterConvert;
-                    break;
-                }
-                if (!actuallyKeepOriginal) {
-                    Object.DestroyImmediate (toConvert);
-                } else {
-                    // rename and put under scene root in case we need to check values
-                    toConvert.name = "_safe_to_delete_" + toConvert.name;
-                    toConvert.SetActive (false);
-                }
-
-                return unityGO;
+                return toConvert;
             }
 
             /// <summary>
@@ -318,73 +261,75 @@ public static void EnforceUniqueNames(IEnumerable<GameObject> exportSet)
             }
 
             /// <summary>
-            /// Sets up the imported GameObject to match the original.
-            /// - Updates the name to be the same as original (i.e. remove the "(Clone)")
-            /// - Moves the imported object to the correct position in the hierarchy
-            /// - Updates the transform of the imported GameObject to match the original
-            /// - Copy over missing components and component values
+            /// Updates the meshes and materials of the exported GameObjects
+            /// to link to those imported from the FBX.
             /// </summary>
-            /// <param name="orig">Original GameObject.</param>
-            /// <param name="imported">Imported GameObject.</param>
-            public static void SetupImportedGameObject(GameObject orig, GameObject imported)
+            /// <param name="orig">Original.</param>
+            /// <param name="fbx">Fbx.</param>
+            public static void UpdateFromFBX(GameObject orig, GameObject fbx)
             {
-                Transform importedTransform = imported.transform;
-                Transform origTransform = orig.transform;
-
-                // configure transform and maintain local pose
-                importedTransform.SetParent (origTransform.parent, false);
-                importedTransform.SetSiblingIndex (origTransform.GetSiblingIndex());
-
-                // copy the components over, assuming that the hierarchy order is unchanged
-                if (origTransform.hierarchyCount != importedTransform.hierarchyCount) {
-                    Debug.LogWarning (string.Format ("Warning: Exported {0} objects, but only imported {1}",
-                        origTransform.hierarchyCount, importedTransform.hierarchyCount));
+                // recurse over orig, for each transform finding the corresponding transform in the FBX
+                // and copying the meshes and materials over from the FBX
+                var goDict = GetNameToFbxGameObject(orig, fbx);
+                var q = new Queue<Transform> ();
+                q.Enqueue (orig.transform);
+                while (q.Count > 0) {
+                    var t = q.Dequeue ();
+
+                    if (goDict [t.name] == null) {
+                        Debug.LogWarning (string.Format ("Warning: Could not find Object {0} in FBX", t.name));
+                        continue;
+                    } 
+                    CopyComponents (t.gameObject, goDict [t.name]);
+                    foreach (Transform child in t) {
+                        q.Enqueue (child);
+                    }
                 }
-                FixSiblingOrder (orig.transform, imported.transform);
-
-                // the imported GameObject will have the same name as the file to which it was imported from,
-                // which might not be the same name as the original GameObject
-                CopyComponentsRecursive (orig, imported, namesExpectedMatch:false);
             }
 
             /// <summary>
-            /// Given two hierarchies of nodes whose names match up,
-            /// make the 'imported' hierarchy have its children be in the same
-            /// order as the 'orig' hierarchy.
-            ///
-            /// The 'orig' hierarchy is not modified.
+            /// Gets a dictionary linking exported GameObject name to fbx game object.
             /// </summary>
-            public static void FixSiblingOrder(Transform orig, Transform imported){
-                foreach (Transform origChild in orig) {
-                    Transform importedChild = imported.Find (origChild.name);
-                    if (importedChild == null) {
-                        Debug.LogWarning (string.Format(
-                            "Warning: Could not find {0} in parented under {1} in import hierarchy",
-                            origChild.name, imported.name
-                        ));
-                        continue;
+            /// <returns>Dictionary containing the name to fbx game object.</returns>
+            /// <param name="orig">Original.</param>
+            /// <param name="fbx">Fbx.</param>
+            private static Dictionary<string,GameObject> GetNameToFbxGameObject(GameObject orig, GameObject fbx){
+                var nameToGO = new Dictionary<string,GameObject> ();
+
+                var q = new Queue<Transform> ();
+                q.Enqueue (orig.transform);
+                while (q.Count > 0) {
+                    var t = q.Dequeue ();
+                    nameToGO [t.name] = null;
+                    foreach (Transform child in t) {
+                        q.Enqueue (child);
                     }
-                    importedChild.SetSiblingIndex (origChild.GetSiblingIndex ());
-                    FixSiblingOrder (origChild, importedChild);
                 }
-            }
 
-            private static void CopyComponentsRecursive(GameObject from, GameObject to, bool namesExpectedMatch = true){
-                if (namesExpectedMatch && !to.name.StartsWith(from.name) || from.transform.childCount != to.transform.childCount) {
-                    Debug.LogError (string.Format("Error: hierarchies don't match (From: {0}, To: {1})", from.name, to.name));
-                    return;
+                nameToGO [orig.name] = fbx;
+
+                var fbxQ = new Queue<Transform> ();
+                foreach (Transform child in fbx.transform) {
+                    fbxQ.Enqueue (child);
                 }
 
-                CopyComponents (from, to);
-                for (int i = 0; i < from.transform.childCount; i++) {
-                    CopyComponentsRecursive(from.transform.GetChild(i).gameObject, to.transform.GetChild(i).gameObject);
+                while (fbxQ.Count > 0) {
+                    var t = fbxQ.Dequeue ();
+                    if (!nameToGO.ContainsKey (t.name)) {
+                        Debug.LogWarning (string.Format("Warning: {0} in FBX but not in converted hierarchy", t.name));
+                        continue;
+                    }
+                    nameToGO [t.name] = t.gameObject;
+                    foreach (Transform child in t) {
+                        fbxQ.Enqueue (child);
+                    }
                 }
-            }
 
+                return nameToGO;
+            }
 
             /// <summary>
-            /// Copy components on the 'from' object which is the object
-            /// in the scene that we imported from the FBX,
+            /// Copy components on the 'from' object which is the FBX,
             /// over to the 'to' object which is the object in the
             /// scene we exported.
             ///
@@ -430,9 +375,9 @@ public static void CopyComponents(GameObject to, GameObject from){
                     } else if (toComponent is MeshFilter) {
                         EditorJsonUtility.FromJsonOverwrite (json, toComponent);
                     } else if (toComponent is Renderer) {
-                        var toRen = toComponent as Renderer;
-                        var fromRen = component as Renderer;
-                        toRen.sharedMaterials = fromRen.sharedMaterials;
+                        var toRenderer = toComponent as Renderer;
+                        var fromRenderer = component as Renderer;
+                        toRenderer.sharedMaterials = fromRenderer.sharedMaterials;
                     }
                 }
             }
diff --git a/Assets/FbxExporters/Editor/UnitTests/ConvertToModelTest.cs b/Assets/FbxExporters/Editor/UnitTests/ConvertToModelTest.cs
@@ -58,37 +58,6 @@ public void TestStaticHelpers()
                 Assert.AreEqual("a 2", a2.name);
             }
 
-            // Test FixSiblingOrder
-            {
-                var a = new GameObject("a").transform;
-                var b = new GameObject("a").transform;
-
-                var a1 = new GameObject("a1").transform;
-                var a2 = new GameObject("a2").transform;
-                var a3 = new GameObject("a3").transform;
-
-                var b1 = new GameObject("a1").transform;
-                var b2 = new GameObject("a2").transform;
-                var b3 = new GameObject("a3").transform;
-
-                a1.parent = a;
-                a2.parent = a;
-                a3.parent = a;
-
-                b3.parent = b;
-                b1.parent = b;
-                b2.parent = b;
-
-                // Assert same set, different order.
-                Assert.That(ChildNames(b), Is.EquivalentTo(ChildNames(a)));
-                Assert.That(ChildNames(b), Is.Not.EqualTo(ChildNames(a)));
-
-                // Fix the sibling order. Now we have same set, same order!
-                ConvertToModel.FixSiblingOrder(a, b);
-                Assert.That(ChildNames(b), Is.EquivalentTo(ChildNames(a)));
-                Assert.That(ChildNames(b), Is.EqualTo(ChildNames(a)));
-            }
-
             // Test CopyComponents
             {
                 var a = new GameObject("a");
@@ -121,7 +90,7 @@ public void TestStaticHelpers()
                 Assert.AreEqual ("BB", b.transform.GetChild (1).name);
                 Assert.AreEqual (Vector3.zero, b1.transform.localPosition);
 
-                ConvertToModel.SetupImportedGameObject (a, b);
+                ConvertToModel.UpdateFromFBX (a, b);
 
                 Assert.IsTrue (b.GetComponent<BoxCollider> ());
                 Assert.AreEqual ("AA", b.transform.GetChild (1).name);
@@ -140,7 +109,7 @@ public void BasicTest()
 
             // Convert it to a prefab -- but keep the cube.
             var cubePrefabInstance = ConvertToModel.Convert(cube,
-                directoryFullPath: path, keepOriginal: ConvertToModel.KeepOriginal.Keep);
+                directoryFullPath: path);
 
             // Make sure it's what we expect.
             Assert.That(cube); // we kept the original
@@ -175,13 +144,12 @@ public void BasicTest()
             // Convert it again, make sure there's only one FbxPrefab (see UNI-25528).
             // Also make sure we deleted.
             var cubePrefabInstance2 = ConvertToModel.Convert(cubePrefabInstance,
-                directoryFullPath: path, keepOriginal: ConvertToModel.KeepOriginal.Delete);
+                directoryFullPath: path);
             Assert.IsFalse(cubePrefabInstance);
             Assert.That(cubePrefabInstance2.GetComponents<FbxPrefab>().Length, Is.EqualTo(1));
 
             // Create another cube, make sure the export settings drive whether we keep the cube or not.
-            ConvertToModel.Convert(cube, directoryFullPath: path,
-                    keepOriginal: ConvertToModel.KeepOriginal.Default);
+            ConvertToModel.Convert(cube, directoryFullPath: path);
             if (ConvertToModel.ExportSettings.keepOriginalAfterConvert) {
                 Assert.IsTrue(cube);
             } else {
diff --git a/Assets/FbxExporters/Editor/UnitTests/FbxPrefabTest.cs b/Assets/FbxExporters/Editor/UnitTests/FbxPrefabTest.cs
@@ -407,8 +407,7 @@ public void TestTransformAndReparenting()
             building3.transform.localScale = new Vector3(.5f, .5f, .5f);
 
             var original = ConvertToModel.Convert (root,
-                fbxFullPath: GetRandomFbxFilePath (),
-                keepOriginal: ConvertToModel.KeepOriginal.Keep);
+                fbxFullPath: GetRandomFbxFilePath ());
 
             // Make sure it's OK.
             Assert.That (original.transform.GetChild (0).name, Is.EqualTo ("building2"));
