Fixed a bunch of bugs with async resource loading

CamelotClient/CamelotClient.cpp

@@ -205,8 +205,11 @@ int CALLBACK WinMain(
 	gResources().unload(testTexRef);
 	gResources().unload(dbgMeshRef);
 
-	testTexRef = static_resource_cast<Texture>(gResources().load("C:\\ExportTest.tex"));
-	dbgMeshRef = static_resource_cast<Mesh>(gResources().load("C:\\ExportMesh.mesh"));
+	testTexRef = static_resource_cast<Texture>(gResources().loadAsync("C:\\ExportTest.tex"));
+	dbgMeshRef = static_resource_cast<Mesh>(gResources().loadAsync("C:\\ExportMesh.mesh"));
+	
+	dbgMeshRef.waitUntilLoaded();
+	testTexRef.waitUntilLoaded();
 
 	testMaterial->setTexture("tex", testTexRef);
 	gResources().create(testMaterial, "C:\\ExportMaterial.mat", true);
@@ -217,8 +220,6 @@ int CALLBACK WinMain(
 
 	//_ASSERT(_CrtCheckMemory());
 
-	//dbgMeshRef.waitUntilLoaded();
-
 	testRenderable->setMesh(dbgMeshRef);
 	testRenderable->setMaterial(testMaterial);
 

CamelotRenderer/Include/CmApplication.h

@@ -66,11 +66,6 @@ namespace CamelotEngine
 
 		volatile bool mRunMainLoop;
 
-		/**
-		 * @brief	Callback called from the render thread in order to initialize resources.
-		 */
-		void updateResourcesCallback();
-
 		/**
 		 * @brief	Runs the OS specific message pump.
 		 */

CamelotRenderer/Include/CmResources.h

@@ -51,11 +51,6 @@ namespace CamelotEngine
 		Resources(const String& metaDataFolder);
 		~Resources();
 
-		/**
-		 * @brief	Called every frame by the main loop.
-		 */
-		void update();
-
 		/**
 		 * @brief	Loads the resource from a given path. Returns null if resource can't be loaded.
 		 *
@@ -72,7 +67,7 @@ namespace CamelotEngine
 
 		/**
 		 * @brief	Loads the resource asynchronously. Initially returned resource should not be used
-		 * 			until BaseResourceHandle.isResolved gets set to true. (Set only at the end of each frame)
+		 * 			until BaseResourceHandle.isLoaded gets set to true.
 		 *
 		 * @param	filePath	Full pathname of the file.
 		 * 						
@@ -91,7 +86,7 @@ namespace CamelotEngine
 
 		/**
 		* @brief	Loads the resource with the given UUID asynchronously. Initially returned resource should not be used
-		* 			until BaseResourceHandle.isResolved gets set to true. (Set only at the end of each frame)
+		* 			until BaseResourceHandle.isLoaded gets set to true.
 		 *
 		 * @param	uuid	UUID of the resource to load. 
 		 *
@@ -152,6 +147,9 @@ namespace CamelotEngine
 		map<String, ResourceMetaDataPtr>::type mResourceMetaData;
 		map<String, ResourceMetaDataPtr>::type mResourceMetaData_FilePath;
 
+		CM_MUTEX(mInProgressResourcesMutex);
+		CM_MUTEX(mLoadedResourceMutex);
+
 		ResourceRequestHandler* mRequestHandler;
 		ResourceResponseHandler* mResponseHandler;
 
@@ -178,6 +176,9 @@ namespace CamelotEngine
 		const String& getPathFromUUID(const String& uuid) const;
 		const String& getUUIDFromPath(const String& path) const;
 
+		void notifyResourceLoadingFinished(BaseResourceHandle& handle);
+		void notifyNewResourceLoaded(BaseResourceHandle& handle);
+
 		String mMetaDataFolderPath;
 	};
 

CamelotRenderer/Source/CmApplication.cpp

@@ -112,7 +112,6 @@ namespace CamelotEngine
 				}
 				
 				renderSystem->queueCommand(boost::bind(&Application::updateMessagePump, this));
-				renderSystem->queueCommand(boost::bind(&Application::updateResourcesCallback, this));
 				mPrimaryRenderContext->submitToGpu();
 				renderSystem->queueCommand(boost::bind(&Application::frameRenderingFinishedCallback, this));
 			}
@@ -135,11 +134,6 @@ namespace CamelotEngine
 		WindowEventUtilities::messagePump();
 	}
 
-	void Application::updateResourcesCallback()
-	{
-		gResources().update();
-	}
-
 	void Application::frameRenderingFinishedCallback()
 	{
 		CM_LOCK_MUTEX(mFrameRenderingFinishedMutex);

CamelotRenderer/Source/CmRenderSystem.cpp

@@ -91,7 +91,7 @@ namespace CamelotEngine {
 	void RenderSystem::initialize()
 	{
 		mRenderThreadId = CM_THREAD_CURRENT_ID;
-		mCommandQueue = new CommandQueue(CM_THREAD_CURRENT_ID);
+		mCommandQueue = new CommandQueue(CM_THREAD_CURRENT_ID, true);
 
 		initRenderThread();
 

CamelotRenderer/Source/CmResources.cpp

@@ -34,16 +34,17 @@ namespace CamelotEngine
 	{
 		ResourceLoadRequestPtr resRequest = boost::any_cast<ResourceLoadRequestPtr>(res->getRequest()->getData());
 
-		auto iterFind = gResources().mInProgressResources.find(resRequest->resource.getUUID());
-		gResources().mInProgressResources.erase(iterFind);
-
 		if(res->getRequest()->getAborted())
 			return;
 
+		gResources().notifyResourceLoadingFinished(resRequest->resource);
+
 		if(res->succeeded())
 		{
 			ResourceLoadResponsePtr resResponse = boost::any_cast<ResourceLoadResponsePtr>(res->getData());
 			
+			// This should be thread safe without any sync primitives, if other threads read a few cycles out of date value
+			// and think this resource isn't created when it really is, it hardly makes any difference
 			resRequest->resource.resolve(resResponse->rawResource);
 
 			if(!gResources().metaExists_UUID(resResponse->rawResource->getUUID()))
@@ -52,7 +53,7 @@ namespace CamelotEngine
 				gResources().addMetaData(resResponse->rawResource->getUUID(), resRequest->filePath);
 			}
 
-			gResources().mLoadedResources[resResponse->rawResource->getUUID()] = resRequest->resource;
+			gResources().notifyNewResourceLoaded(resRequest->resource);
 		}
 		else
 		{
@@ -107,11 +108,6 @@ namespace CamelotEngine
 			delete mResponseHandler;
 	}
 
-	void Resources::update()
-	{
-		mWorkQueue->processResponses();
-	}
-
 	BaseResourceHandle Resources::load(const String& filePath)
 	{
 		return loadInternal(filePath, true);
@@ -162,24 +158,38 @@ namespace CamelotEngine
 
 		String uuid = getUUIDFromPath(filePath);
 
-		auto iterFind = mLoadedResources.find(uuid);
-		if(iterFind != mLoadedResources.end()) // Resource is already loaded
 		{
-			return iterFind->second;
+			CM_LOCK_MUTEX(mLoadedResourceMutex);
+			auto iterFind = mLoadedResources.find(uuid);
+			if(iterFind != mLoadedResources.end()) // Resource is already loaded
+			{
+				return iterFind->second;
+			}
 		}
 
-		auto iterFind2 = mInProgressResources.find(uuid);
-		if(iterFind2 != mInProgressResources.end()) // We're already loading this resource
+		bool resourceLoadingInProgress = false;
+		BaseResourceHandle existingResource;
+
 		{
-			ResourceAsyncOp& asyncOp = iterFind2->second;
+			CM_LOCK_MUTEX(mInProgressResourcesMutex);
+			auto iterFind2 = mInProgressResources.find(uuid);
+			if(iterFind2 != mInProgressResources.end()) 
+			{
+				existingResource = iterFind2->second.resource;
+				resourceLoadingInProgress = true;
+			}
+		}
 
+		if(resourceLoadingInProgress) // We're already loading this resource
+		{
 			if(!synchronous)
-				return asyncOp.resource;
+				return existingResource;
 			else
 			{
-				// It's already being loaded asynchronously but we want it right away,
-				// so abort the async operation and load it right away.
-				mWorkQueue->abortRequest(asyncOp.requestID);
+				// Previously being loaded as async but now we want it synced, so we wait
+				existingResource.waitUntilLoaded();
+
+				return existingResource;
 			}
 		}
 
@@ -201,7 +211,10 @@ namespace CamelotEngine
 		newAsyncOp.resource = newResource;
 		newAsyncOp.requestID = requestId;
 
-		mInProgressResources[uuid] = newAsyncOp;
+		{
+			CM_LOCK_MUTEX(mInProgressResourcesMutex);
+			mInProgressResources[uuid] = newAsyncOp;
+		}
 
 		mWorkQueue->addRequest(mWorkQueueChannel, resRequest, 0, synchronous);
 		return newResource;
@@ -229,17 +242,23 @@ namespace CamelotEngine
 
 		resource->destroy();
 
-		mLoadedResources.erase(resource.getUUID());
+		{
+			CM_LOCK_MUTEX(mLoadedResourceMutex);
+			mLoadedResources.erase(resource.getUUID());
+		}
 	}
 
 	void Resources::unloadAllUnused()
 	{
 		vector<BaseResourceHandle>::type resourcesToUnload;
 
-		for(auto iter = mLoadedResources.begin(); iter != mLoadedResources.end(); ++iter)
 		{
-			if(iter->second.getInternalPtr().unique()) // We just have this one reference, meaning nothing is using this resource
-				resourcesToUnload.push_back(iter->second);
+			CM_LOCK_MUTEX(mLoadedResourceMutex);
+			for(auto iter = mLoadedResources.begin(); iter != mLoadedResources.end(); ++iter)
+			{
+				if(iter->second.getInternalPtr().unique()) // We just have this one reference, meaning nothing is using this resource
+					resourcesToUnload.push_back(iter->second);
+			}
 		}
 
 		for(auto iter = resourcesToUnload.begin(); iter != resourcesToUnload.end(); ++iter)
@@ -277,7 +296,11 @@ namespace CamelotEngine
 
 		save(resource);
 
-		mLoadedResources[resource->getUUID()] = resource;
+		{
+			CM_LOCK_MUTEX(mLoadedResourceMutex);
+
+			mLoadedResources[resource->getUUID()] = resource;
+		}
 	}
 
 	void Resources::save(BaseResourceHandle resource)
@@ -402,6 +425,20 @@ namespace CamelotEngine
 		return findIter != mResourceMetaData_FilePath.end();
 	}
 
+	void Resources::notifyResourceLoadingFinished(BaseResourceHandle& handle)
+	{
+		CM_LOCK_MUTEX(mInProgressResourcesMutex);
+
+		mInProgressResources.erase(handle.getUUID());
+	}
+
+	void Resources::notifyNewResourceLoaded(BaseResourceHandle& handle)
+	{
+		CM_LOCK_MUTEX(mLoadedResourceMutex);
+
+		mLoadedResources[handle.getUUID()] = handle;
+	}
+
 	CM_EXPORT Resources& gResources()
 	{
 		return Resources::instance();

CamelotRenderer/TODO.txt

@@ -14,16 +14,9 @@ Pass
  - A way to bind buffers to a Pass, while specifying buffer range
  - GpuParams support for bools, buffers, structs
 
-Add support for include file resource
-Make sure we can add an include file to a HighLevelGpuProgram, and make sure it uses it
- - Also a way to list all referenced includes, and a way to remove them
-
- Importer for shaders
-  - If GpuProgram is immutable then how do I add an include file to it after calling import()?
-  - Figure out a better way of assigning include files
+Importers for shaders.
 
 Seems there is a possible deadlock when starting the render thread, while waiting for the thread to be started
-Get rid of resource handlers in Resources?
 
 Deserialization issues:
  - Currently Reflectable is not allowed to hold a ReflectablePtr
@@ -44,6 +37,7 @@ Can be delayed:
    - RENDERWINDOWDESC accepts a "externalWindow" flag and an "externalHandle" so when creating the primary window with RenderSystem::initialize we don't always need to create a new window
    - Actually new OpenGL seems to support creating context without a window with the help of wglCreateContextAttribsARB and wglMakeCurrent:
  - ImportOptions
+  - Make sure Gpu prog include files can get assigned here
  - Instead of doing setThisPtr on every CoreGpuObject, use intrusive shared_ptr instead?
  - OpenGL render window no longer looks for a monitor index
  - Test if async loading still works
@@ -60,6 +54,17 @@ Can be delayed:
 
 ---------------------------------------------------
 
+<<<<Resource changes and reimport>>>>
+Use case example:
+ - User reimports a .gpuproginc file
+   - Dependencies are held by CoreGpuObjectManager. Objects are required to register/unregister
+      their dependencies in their methods manually.
+ - Editor calls SomeClass::reimport(handle) after it detects a change 
+   - this will load a new resource, release the old one and assign the new one to Handle without changing the GUID
+   - In order to make it thread safe force all threads to finish what they're doing and pause them until the switch is done
+     - Should be okay considering this should only happen in edit-mode so performance isn't imperative
+   - reimport is recursively called on all dependant objects as well.
+
 <<<<Handle multithreaded object management>>>:
  - Make everything that is possible immutable. Once created it cant be changed.
   - Example are shaders, state objects and similar

CamelotUtility/Include/CmWorkQueue.h

@@ -180,10 +180,8 @@ namespace CamelotEngine
 		unsigned long mResposeTimeLimitMS;
 
 		typedef deque<Request*>::type RequestQueue;
-		typedef deque<Response*>::type ResponseQueue;
 		RequestQueue mRequestQueue;
 		RequestQueue mProcessQueue;
-		ResponseQueue mResponseQueue;
 
 		/// Thread function
 		struct WorkerFunc CM_THREAD_WORKER_INHERIT
@@ -269,7 +267,6 @@ namespace CamelotEngine
 		CM_MUTEX(mInitMutex)
 		CM_MUTEX(mRequestMutex)
 		CM_MUTEX(mProcessMutex)
-		CM_MUTEX(mResponseMutex)
 		CM_RW_MUTEX(mRequestHandlerMutex);
 
 #if CM_THREAD_SUPPORT
@@ -403,15 +400,8 @@ namespace CamelotEngine
 		/** Returns whether the queue is trying to shut down. */
 		bool isShuttingDown() const { return mShuttingDown; }
 
-		/** Process the responses in the queue.
-		@remarks
-			This method is public, and must be called from the main render
-			thread to 'pump' responses through the system.
-		*/
-		void processResponses(); 
-
 	protected:
-		void processRequestResponse(Request* r, bool synchronous);
+		void processRequestResponse(Request* r);
 		Response* processRequest(Request* r);
 		void processResponse(Response* r);
 

CamelotUtility/Source/CmWorkQueue.cpp

@@ -48,12 +48,6 @@ namespace CamelotEngine {
 			delete (*i);
 		}
 		mRequestQueue.clear();
-
-		for (ResponseQueue::iterator i = mResponseQueue.begin(); i != mResponseQueue.end(); ++i)
-		{
-			delete (*i);
-		}
-		mResponseQueue.clear();
 	}
 	//---------------------------------------------------------------------
 	void WorkQueue::startup(bool forceRestart)
@@ -219,8 +213,7 @@ namespace CamelotEngine {
 #endif
 		}
 
-
-		processRequestResponse(req, true);
+		processRequestResponse(req);
 
 		return rid;
 
@@ -241,7 +234,7 @@ namespace CamelotEngine {
 		mRequestQueue.push_back(req);
 		notifyWorkers();
 #else
-		processRequestResponse(req, true);
+		processRequestResponse(req);
 #endif
 	}
 	//---------------------------------------------------------------------
@@ -273,19 +266,6 @@ namespace CamelotEngine {
 						}
 					}
 			}
-
-			{
-				CM_LOCK_MUTEX(mResponseMutex)
-
-					for (ResponseQueue::iterator i = mResponseQueue.begin(); i != mResponseQueue.end(); ++i)
-					{
-						if( (*i)->getRequest()->getID() == id )
-						{
-							(*i)->abortRequest();
-							break;
-						}
-					}
-			}
 	}
 	//---------------------------------------------------------------------
 	void WorkQueue::abortRequestsByChannel(UINT16 channel)
@@ -311,18 +291,6 @@ namespace CamelotEngine {
 						}
 					}
 			}
-
-			{
-				CM_LOCK_MUTEX(mResponseMutex)
-
-					for (ResponseQueue::iterator i = mResponseQueue.begin(); i != mResponseQueue.end(); ++i)
-					{
-						if( (*i)->getRequest()->getChannel() == channel )
-						{
-							(*i)->abortRequest();
-						}
-					}
-			}
 	}
 	//---------------------------------------------------------------------
 	void WorkQueue::abortAllRequests()
@@ -342,15 +310,6 @@ namespace CamelotEngine {
 						(*i)->abortRequest();
 					}
 			}
-
-			{
-				CM_LOCK_MUTEX(mResponseMutex)
-
-					for (ResponseQueue::iterator i = mResponseQueue.begin(); i != mResponseQueue.end(); ++i)
-					{
-						(*i)->abortRequest();
-					}
-			}
 	}
 	//---------------------------------------------------------------------
 	void WorkQueue::setPaused(bool pause)
@@ -399,43 +358,13 @@ namespace CamelotEngine {
 		return i->second;
 	}
 	//---------------------------------------------------------------------
-	void WorkQueue::processResponses() 
-	{
-		// TODO Low priority - Processing a lot of responses can cause a frame rate spike. Maybe limit the processing to Xms?
-
-		// keep going until we run out of responses
-		while(true)
-		{
-			Response* response = 0;
-			{
-				CM_LOCK_MUTEX(mResponseMutex)
-
-					if (mResponseQueue.empty())
-						break; // exit loop
-					else
-					{
-						response = mResponseQueue.front();
-						mResponseQueue.pop_front();
-					}
-			}
-
-			if (response)
-			{
-				processResponse(response);
-
-				delete response;
-
-			}
-		}
-	}
-	//---------------------------------------------------------------------
-	void WorkQueue::processRequestResponse(Request* r, bool synchronous)
+	void WorkQueue::processRequestResponse(Request* r)
 	{
 		Response* response = processRequest(r);
 
 		CM_LOCK_MUTEX(mProcessMutex)
 
-			RequestQueue::iterator it;
+		RequestQueue::iterator it;
 		for( it = mProcessQueue.begin(); it != mProcessQueue.end(); ++it )
 		{
 			if( (*it) == r )
@@ -460,24 +389,9 @@ namespace CamelotEngine {
 					return;
 				}
 			}
-			if (synchronous)
-			{
-				processResponse(response);
-				delete response;
-			}
-			else
-			{
-				if( response->getRequest()->getAborted() )
-				{
-					// destroy response user data
-					response->abortRequest();
-				}
-				// Queue response
-				CM_LOCK_MUTEX(mResponseMutex)
-					mResponseQueue.push_back(response);
-				// no need to wake thread, this is processed by the main thread
-			}
 
+			processResponse(response);
+			delete response;
 		}
 		else
 		{
@@ -555,7 +469,7 @@ namespace CamelotEngine {
 
 		if (request)
 		{
-			processRequestResponse(request, false);
+			processRequestResponse(request);
 		}