pr#378 - fix review comments about when we create new prefabs

Author: Benoit Hudson

Assets/FbxExporters/Editor/ConvertToModel.cs

@@ -137,18 +137,27 @@ public static GameObject Convert (
                 string prefabFullPath = null, 
                 EditorTools.ConvertToPrefabSettingsSerialize exportOptions = null)
             {
-                // If we selected the something that's already backed by an FBX, don't export.
+                // If we selected the something that's already backed by an
+                // FBX, don't export.
                 var mainAsset = GetOrCreateFbxAsset (toConvert, fbxDirectoryFullPath, fbxFullPath, exportOptions);
 
+                // If we selected a prefab *instance* then break the
+                // connection. We only want to update an existing prefab if
+                // we're converting an existing prefab (not an instance).
+                if (PrefabUtility.GetPrefabType(toConvert) == PrefabType.PrefabInstance) {
+                    PrefabUtility.DisconnectPrefabInstance(toConvert);
+                }
+
                 // Get 'toConvert' into an editable state. We can't edit
                 // assets, and toConvert might be an asset.
                 var toConvertInstance = GetOrCreateInstance (toConvert);
 
                 // Set it up to track the FbxPrefab.
                 SetupFbxPrefab (toConvertInstance, mainAsset);
 
-                // Now get 'toConvertInstance' into a prefab. If toConvert is already a prefab,
-                // this is equivalent to an 'apply' ; if it's not, we're creating a new prefab.
+                // Now get 'toConvertInstance' into a prefab. If toConvert is
+                // already a prefab, this is equivalent to an 'apply' ; if it's
+                // not, we're creating a new prefab.
                 var prefab = ApplyOrCreatePrefab (toConvertInstance, prefabDirectoryFullPath, prefabFullPath);
 
                 if (toConvertInstance == toConvert) {
@@ -166,10 +175,11 @@ public static GameObject Convert (
             }
 
             /// <summary>
-            /// Check whether GetOrCreateFbxAsset will be exporting an fbx file, or reusing one.
+            /// Check whether <see>Convert</see> will be exporting an fbx file,
+            /// or reusing one.
             /// </summary>
-            public static bool WillExportFbx(GameObject go) {
-                return GetFbxAssetOrNull(go) == null;
+            public static bool WillExportFbx(GameObject toConvert) {
+                return GetFbxAssetOrNull(toConvert) == null;
             }
 
             /// <summary>
@@ -266,24 +276,46 @@ public static GameObject GetOrCreateInstance(GameObject toConvert)
             }
 
             /// <summary>
-            /// Check whether ApplyOrCreatePrefab will be creating a new prefab, or updating one.
+            /// Check whether <see>Convert</see> will be creating a new prefab, or
+            /// updating one.
+            ///
+            /// Note that ApplyOrCreatePrefab will create in different
+            /// circumstances than Convert will.
             /// </summary>
-            public static bool WillCreatePrefab(GameObject go) {
-                return PrefabUtility.GetPrefabType(go) != PrefabType.PrefabInstance;
+            public static bool WillCreatePrefab(GameObject toConvert) {
+                return PrefabUtility.GetPrefabType(toConvert) != PrefabType.Prefab;
             }
 
             /// <summary>
             /// Create a prefab from 'instance', or apply 'instance' to its
             /// prefab if it's already an instance of a prefab.
             ///
-            /// Return the new or updated prefab.
+            /// If it's an instance of a model prefab (an fbx file) we will
+            /// lose that connection and point to a new prefab file.
+            ///
+            /// To avoid applying to an existing prefab, break the prefab
+            /// connection with <c>PrefabUtility.DisconnectPrefabInstance</c>.
+            ///
+            /// Returns the new or updated prefab.
             /// </summary>
+            /// <param name="instance">A GameObject in the scene. After this
+            /// call, it will be a prefab instance (it might already be
+            /// one).</param>
+            /// <param name="prefabFullPath">The full path to a prefab file we
+            /// will create. Ignored if <paramref name="instance"/> is a
+            /// prefab instance already. May be null, in which case we'll use <paramref
+            /// name="prefabDirectoryFullPath"/> to generate a new name</param>
+            /// <param name="prefabDirectoryFullPath">The full path to a
+            /// directory that will hold the new prefab. Ignored if <paramref
+            /// name="instance"/> is a prefab instance already. Ignored if
+            /// <paramref name="prefabFullPath"/> is provided. May be null, in
+            /// which case we'll use the project export settings.</param>
+            /// <returns>The new or existing prefab.</returns>
             public static GameObject ApplyOrCreatePrefab(GameObject instance,
                 string prefabDirectoryFullPath = null,
                 string prefabFullPath = null)
             {
                 if(PrefabUtility.GetPrefabType(instance) == PrefabType.PrefabInstance) {
-                    // Apply: there's already a prefab.
                     return PrefabUtility.ReplacePrefab(instance, PrefabUtility.GetPrefabParent(instance));
                 }
 

Assets/FbxExporters/Editor/UnitTests/ConvertToModelTest.cs

@@ -115,24 +115,48 @@ public void TestStaticHelpers()
                 Assert.AreEqual(1, a.GetComponents<FbxPrefab>().Length);
             }
 
-            // Test ApplyOrCreatePrefab and WillCreatePrefab
+            // Test ApplyOrCreatePrefab
             {
                 // Create a hierarchy that isn't a prefab, apply it (creating a new prefab).
                 var a = CreateHierarchy();
-                Assert.That(ConvertToModel.WillCreatePrefab(a));
+
                 var aPath = GetRandomPrefabAssetPath();
                 var aPrefab = ConvertToModel.ApplyOrCreatePrefab(a, prefabFullPath: aPath);
                 Assert.AreEqual(aPrefab, PrefabUtility.GetPrefabParent(a));
 
                 // Now apply it again (replacing aPrefab). Make sure we use the
                 // same file rather than creating a new one.
                 var bPath = GetRandomPrefabAssetPath();
-                Assert.That(!ConvertToModel.WillCreatePrefab(a));
                 var bPrefab = ConvertToModel.ApplyOrCreatePrefab(a, prefabFullPath: bPath);
                 Assert.AreEqual(bPrefab, PrefabUtility.GetPrefabParent(a));
                 Assert.AreEqual(aPath, AssetDatabase.GetAssetPath(bPrefab));
             }
 
+            // Test WillCreatePrefab
+            // Remember that it answers what convert will do, not what
+            // apply-or-create will do (it's different).
+            {
+                // Create depends on prefab type.
+                var a = CreateHierarchy();
+
+                // None => create
+                Assert.That(ConvertToModel.WillCreatePrefab(a));
+
+                // PrefabInstance => create
+                // Prefab => don't create
+                // DisconnectedPrefabInstance => create
+                var aPrefab = PrefabUtility.CreatePrefab(GetRandomPrefabAssetPath(), a);
+                Assert.That(ConvertToModel.WillCreatePrefab(a));
+                Assert.That(!ConvertToModel.WillCreatePrefab(aPrefab));
+                PrefabUtility.DisconnectPrefabInstance(a);
+                Assert.That(ConvertToModel.WillCreatePrefab(a));
+
+                // ModelPrefabInstance or ModelPrefab => create
+                var aFbx = ExportToFbx(a);
+                Assert.That(ConvertToModel.WillCreatePrefab(aFbx));
+                Assert.That(ConvertToModel.WillCreatePrefab(PrefabUtility.InstantiatePrefab(aFbx) as GameObject));
+            }
+
             // Test CopyComponents
             {
                 var a = GameObject.CreatePrimitive (PrimitiveType.Cube);