Przeglądaj źródła

Fixing issues with Prefab link ID assignment:
- Root prefab SO would keep its link ID, meaning any instance would end up having that same ID
- Changing prefab parents of objects who already had link IDs would keep those link IDs, while not setting the mNextLinkID counter. This means that as user added more objects to the prefab their IDs could end up being duplicates of the original objects.
Added a unit test for Prefab link ID assignment

BearishSun 9 lat temu
rodzic
commit
8fb8ad8a59

+ 0 - 1
Source/BansheeCore/Include/BsPrefab.h

@@ -95,7 +95,6 @@ namespace BansheeEngine
 		HSceneObject mRoot;
 		HSceneObject mRoot;
 		UINT32 mHash;
 		UINT32 mHash;
 		String mUUID;
 		String mUUID;
-		UINT32 mNextLinkId;
 		bool mIsScene;
 		bool mIsScene;
 
 
 		/************************************************************************/
 		/************************************************************************/

+ 1 - 1
Source/BansheeCore/Include/BsPrefabRTTI.h

@@ -19,7 +19,7 @@ namespace BansheeEngine
 	private:
 	private:
 		BS_BEGIN_RTTI_MEMBERS
 		BS_BEGIN_RTTI_MEMBERS
 			BS_RTTI_MEMBER_PLAIN(mHash, 1)
 			BS_RTTI_MEMBER_PLAIN(mHash, 1)
-			BS_RTTI_MEMBER_PLAIN(mNextLinkId, 2)
+			//BS_RTTI_MEMBER_PLAIN(mNextLinkId, 2)
 			BS_RTTI_MEMBER_PLAIN(mUUID, 3)
 			BS_RTTI_MEMBER_PLAIN(mUUID, 3)
 			BS_RTTI_MEMBER_PLAIN(mIsScene, 4)
 			BS_RTTI_MEMBER_PLAIN(mIsScene, 4)
 		BS_END_RTTI_MEMBERS
 		BS_END_RTTI_MEMBERS

+ 1 - 1
Source/BansheeCore/Include/BsPrefabUtility.h

@@ -55,7 +55,7 @@ namespace BansheeEngine
 		 *
 		 *
 		 * @note	If any children of the provided object belong to another prefab they will not have IDs generated.
 		 * @note	If any children of the provided object belong to another prefab they will not have IDs generated.
 		 */
 		 */
-		static UINT32 generatePrefabIds(const HSceneObject& sceneObject, UINT32 startingId);
+		static void generatePrefabIds(const HSceneObject& sceneObject);
 
 
 		/**
 		/**
 		 * Clears all prefab "link" IDs in the provided object and its children.
 		 * Clears all prefab "link" IDs in the provided object and its children.

+ 7 - 10
Source/BansheeCore/Source/BsPrefab.cpp

@@ -10,7 +10,7 @@
 namespace BansheeEngine
 namespace BansheeEngine
 {
 {
 	Prefab::Prefab()
 	Prefab::Prefab()
-		:Resource(false), mHash(0), mNextLinkId(0), mIsScene(true)
+		:Resource(false), mHash(0), mIsScene(true)
 	{
 	{
 		
 		
 	}
 	}
@@ -25,6 +25,8 @@ namespace BansheeEngine
 	{
 	{
 		SPtr<Prefab> newPrefab = createEmpty();
 		SPtr<Prefab> newPrefab = createEmpty();
 		newPrefab->mIsScene = isScene;
 		newPrefab->mIsScene = isScene;
+
+		PrefabUtility::clearPrefabIds(sceneObject, true, false);
 		newPrefab->initialize(sceneObject);
 		newPrefab->initialize(sceneObject);
 
 
 		HPrefab handle = static_resource_cast<Prefab>(gResources()._createResourceHandle(newPrefab));
 		HPrefab handle = static_resource_cast<Prefab>(gResources()._createResourceHandle(newPrefab));
@@ -46,15 +48,7 @@ namespace BansheeEngine
 	void Prefab::initialize(const HSceneObject& sceneObject)
 	void Prefab::initialize(const HSceneObject& sceneObject)
 	{
 	{
 		sceneObject->mPrefabDiff = nullptr;
 		sceneObject->mPrefabDiff = nullptr;
-		UINT32 newNextLinkId = PrefabUtility::generatePrefabIds(sceneObject, mNextLinkId);
-
-		if (newNextLinkId < mNextLinkId)
-		{
-			BS_EXCEPT(InternalErrorException, "Prefab ran out of IDs to assign. " \
-				"Consider increasing the size of the prefab ID data type.");
-		}
-
-		mNextLinkId = newNextLinkId;
+		PrefabUtility::generatePrefabIds(sceneObject);
 
 
 		// If there are any child prefab instances, make sure to update their diffs so they are saved with this prefab
 		// If there are any child prefab instances, make sure to update their diffs so they are saved with this prefab
 		Stack<HSceneObject> todo;
 		Stack<HSceneObject> todo;
@@ -80,6 +74,7 @@ namespace BansheeEngine
 		// Clone the hierarchy for internal storage
 		// Clone the hierarchy for internal storage
 		mRoot = sceneObject->clone(false);
 		mRoot = sceneObject->clone(false);
 		mRoot->mParent = nullptr;
 		mRoot->mParent = nullptr;
+		mRoot->mLinkId = -1;
 
 
 		// Remove objects with "dont save" flag
 		// Remove objects with "dont save" flag
 		todo.push(mRoot);
 		todo.push(mRoot);
@@ -157,6 +152,8 @@ namespace BansheeEngine
 			return HSceneObject();
 			return HSceneObject();
 
 
 		mRoot->mPrefabHash = mHash;
 		mRoot->mPrefabHash = mHash;
+		mRoot->mLinkId = -1;
+
 		return mRoot->clone(false);
 		return mRoot->clone(false);
 	}
 	}
 
 

+ 40 - 4
Source/BansheeCore/Source/BsPrefabUtility.cpp

@@ -148,11 +148,43 @@ namespace BansheeEngine
 		gResources().unloadAllUnused();
 		gResources().unloadAllUnused();
 	}
 	}
 
 
-	UINT32 PrefabUtility::generatePrefabIds(const HSceneObject& sceneObject, UINT32 startingId)
+	void PrefabUtility::generatePrefabIds(const HSceneObject& sceneObject)
 	{
 	{
+		UINT32 startingId = 0;
+
 		Stack<HSceneObject> todo;
 		Stack<HSceneObject> todo;
 		todo.push(sceneObject);
 		todo.push(sceneObject);
 
 
+		while (!todo.empty())
+		{
+			HSceneObject currentSO = todo.top();
+			todo.pop();
+
+			for (auto& component : currentSO->mComponents)
+			{
+				if (component->getLinkId() != (UINT32)-1)
+					startingId = std::max(component->mLinkId + 1, startingId);
+			}
+
+			UINT32 numChildren = (UINT32)currentSO->getNumChildren();
+			for (UINT32 i = 0; i < numChildren; i++)
+			{
+				HSceneObject child = currentSO->getChild(i);
+
+				if (!child->hasFlag(SOF_DontSave))
+				{
+					if (child->getLinkId() != (UINT32)-1)
+						startingId = std::max(child->mLinkId + 1, startingId);
+
+					if (child->mPrefabLinkUUID.empty())
+						todo.push(currentSO->getChild(i));
+				}
+			}
+		}
+
+		UINT32 currentId = startingId;
+		todo.push(sceneObject);
+
 		while (!todo.empty())
 		while (!todo.empty())
 		{
 		{
 			HSceneObject currentSO = todo.top();
 			HSceneObject currentSO = todo.top();
@@ -161,7 +193,7 @@ namespace BansheeEngine
 			for (auto& component : currentSO->mComponents)
 			for (auto& component : currentSO->mComponents)
 			{
 			{
 				if (component->getLinkId() == (UINT32)-1)
 				if (component->getLinkId() == (UINT32)-1)
-					component->mLinkId = startingId++;
+					component->mLinkId = currentId++;
 			}
 			}
 
 
 			UINT32 numChildren = (UINT32)currentSO->getNumChildren();
 			UINT32 numChildren = (UINT32)currentSO->getNumChildren();
@@ -172,7 +204,7 @@ namespace BansheeEngine
 				if (!child->hasFlag(SOF_DontSave))
 				if (!child->hasFlag(SOF_DontSave))
 				{
 				{
 					if (child->getLinkId() == (UINT32)-1)
 					if (child->getLinkId() == (UINT32)-1)
-						child->mLinkId = startingId++;
+						child->mLinkId = currentId++;
 
 
 					if(child->mPrefabLinkUUID.empty())
 					if(child->mPrefabLinkUUID.empty())
 						todo.push(currentSO->getChild(i));
 						todo.push(currentSO->getChild(i));
@@ -180,7 +212,11 @@ namespace BansheeEngine
 			}
 			}
 		}
 		}
 
 
-		return startingId;
+		if (currentId < startingId)
+		{
+			BS_EXCEPT(InternalErrorException, "Prefab ran out of IDs to assign. " \
+				"Consider increasing the size of the prefab ID data type.");
+		}
 	}
 	}
 
 
 	void PrefabUtility::clearPrefabIds(const HSceneObject& sceneObject, bool recursive, bool clearRoot)
 	void PrefabUtility::clearPrefabIds(const HSceneObject& sceneObject, bool recursive, bool clearRoot)

+ 3 - 0
Source/BansheeEditor/Include/BsEditorTestSuite.h

@@ -88,6 +88,9 @@ namespace BansheeEngine
 		/** Tests prefab diff by modifiying a prefab, generating a diff and re-applying the modifications. */
 		/** Tests prefab diff by modifiying a prefab, generating a diff and re-applying the modifications. */
 		void TestPrefabDiff();
 		void TestPrefabDiff();
 
 
+		/** Tests a complex set of operations on a prefab. */
+		void TestPrefabComplex();
+
 		/**	Tests the frame allocator. */
 		/**	Tests the frame allocator. */
 		void TestFrameAlloc();
 		void TestFrameAlloc();
 	};
 	};

+ 77 - 1
Source/BansheeEditor/Source/BsEditorTestSuite.cpp

@@ -15,6 +15,7 @@
 #include "BsPrefabDiff.h"
 #include "BsPrefabDiff.h"
 #include "BsFrameAlloc.h"
 #include "BsFrameAlloc.h"
 #include "BsFileSystem.h"
 #include "BsFileSystem.h"
+#include "BsSceneManager.h"
 
 
 namespace BansheeEngine
 namespace BansheeEngine
 {
 {
@@ -410,8 +411,9 @@ namespace BansheeEngine
 		BS_ADD_TEST(EditorTestSuite::SceneObjectRecord_UndoRedo);
 		BS_ADD_TEST(EditorTestSuite::SceneObjectRecord_UndoRedo);
 		BS_ADD_TEST(EditorTestSuite::SceneObjectDelete_UndoRedo);
 		BS_ADD_TEST(EditorTestSuite::SceneObjectDelete_UndoRedo);
 		BS_ADD_TEST(EditorTestSuite::BinaryDiff);
 		BS_ADD_TEST(EditorTestSuite::BinaryDiff);
+		BS_ADD_TEST(EditorTestSuite::TestPrefabComplex);
 		BS_ADD_TEST(EditorTestSuite::TestPrefabDiff);
 		BS_ADD_TEST(EditorTestSuite::TestPrefabDiff);
-		BS_ADD_TEST(EditorTestSuite::TestFrameAlloc)
+		BS_ADD_TEST(EditorTestSuite::TestFrameAlloc);
 	}
 	}
 
 
 	void EditorTestSuite::SceneObjectRecord_UndoRedo()
 	void EditorTestSuite::SceneObjectRecord_UndoRedo()
@@ -570,6 +572,80 @@ namespace BansheeEngine
 			BS_TEST_ASSERT(orgObj->arrObjPtrB[i]->intA == newObj->arrObjPtrB[i]->intA);
 			BS_TEST_ASSERT(orgObj->arrObjPtrB[i]->intA == newObj->arrObjPtrB[i]->intA);
 	}
 	}
 
 
+	void EditorTestSuite::TestPrefabComplex()
+	{
+		HSceneObject aDeleteMe = SceneObject::create("A");
+
+		HSceneObject box = SceneObject::create("Box");
+		box->addComponent<TestComponentA>();
+
+		HSceneObject light = SceneObject::create("Directional light");
+		light->addComponent<TestComponentA>();
+
+		HSceneObject sceneRoot = gSceneManager().getRootNode();
+		HPrefab scenePrefab = Prefab::create(sceneRoot, true);
+
+		HSceneObject bDeleteMe = SceneObject::create("B");
+
+		HSceneObject _1 = SceneObject::create("1");
+
+		scenePrefab->update(sceneRoot);
+
+		aDeleteMe->destroy();
+		bDeleteMe->destroy();
+
+		HPrefab targetBoxPrefab = Prefab::create(_1, false);
+
+		HSceneObject camera = SceneObject::create("Camera");
+		camera->addComponent<TestComponentA>();
+
+		scenePrefab->update(sceneRoot);
+
+		HSceneObject target = SceneObject::create("Target");
+
+		_1->setParent(target);
+
+		HSceneObject _3 = targetBoxPrefab->instantiate();
+		_3->setName("3");
+		_3->setParent(target);
+
+		HSceneObject _10 = targetBoxPrefab->instantiate();
+		_10->setParent(target);
+		_10->setName("10");
+
+		// Ensure multiple instances of the same prefab don't have the same ID
+		BS_TEST_ASSERT(_1->getLinkId() != _3->getLinkId() && _1->getLinkId() != _10->getLinkId());
+
+		// Ensure new instances of a prefab have -1 root link ID
+		BS_TEST_ASSERT(_3->getLinkId() == -1 && _10->getLinkId() == -1);
+
+		scenePrefab->update(sceneRoot);
+
+		_1->addComponent<TestComponentA>();
+		_3->addComponent<TestComponentA>();
+		_10->addComponent<TestComponentA>();
+
+		_1->addComponent<TestComponentB>();
+		_3->addComponent<TestComponentB>();
+		_10->addComponent<TestComponentB>();
+
+		_1->breakPrefabLink();
+		_3->breakPrefabLink();
+		_10->breakPrefabLink();
+
+		HPrefab targetPrefab = Prefab::create(target, false);
+		
+		target->addComponent<TestComponentA>();
+		scenePrefab->update(sceneRoot);
+
+		targetPrefab->update(target);
+
+		box->destroy();
+		light->destroy();
+		camera->destroy();
+		target->destroy();
+	}
+
 	void EditorTestSuite::TestPrefabDiff()
 	void EditorTestSuite::TestPrefabDiff()
 	{
 	{
 		HSceneObject root = SceneObject::create("root");
 		HSceneObject root = SceneObject::create("root");