ImguiSystem: properly implement ImGui and fix several bugs

Moved implementation to dedicated class 'CImguiSystem' with the following changes:

* Added mutex for ImGui_ImplWin32_WndProcHandler(), the window proc is ran from a different thread than CImguiSystem::SampleFrame() is being called from. Needs a mutex as both functions interact with the input queue list.

* Added init stages for debugging and preventing the system from running certain parts of the code if a certain stage hasn't been reached yet.

* NULL out ImGuiContext::ConfigNavWindowingKeyNext , we don't need any nav window shortcuts as users don't need then in-game, and it also interferes with certain key binds some users have.

* CImguiSystem::SampleFrame() (previously ImguiSystem_SampleFrame()), is now being called from CMaterialSystem::SwapBuffers(), as that function is called from the main thread while also getting updated during level loads/etc, so instead of switching between 2 static buffers for as long as the levels are being loaded, we imgui updates properly too and directly swap afterwards.

* Removed hack in CGame::WindowProc() where we would only run windowproc if we aren't shutting the game down. This was needed as there was a chance of a crash at some rare cases, but this was simply due to a missing mutex which has been addressed and fixed as of this commit.

* Lock mutex during init and shutdown.

r5dev/engine/host_state.cpp

@@ -125,8 +125,6 @@ void CHostState::FrameUpdate(CHostState* pHostState, double flCurrentTime, float
 #endif // !CLIENT_DLL
 #ifndef DEDICATED
 	RCONClient()->RunFrame();
-
-	ImguiSystem_SampleFrame();
 #endif // !DEDICATED
 
 	// Disable "warning C4611: interaction between '_setjmp' and C++ object destruction is non-portable"
@@ -147,8 +145,7 @@ void CHostState::FrameUpdate(CHostState* pHostState, double flCurrentTime, float
 		{
 			Cbuf_Execute();
 
-			HostStates_t oldState = g_pHostState->m_iCurrentState;
-
+			const HostStates_t oldState = g_pHostState->m_iCurrentState;
 			switch (g_pHostState->m_iCurrentState)
 			{
 			case HostStates_t::HS_NEW_GAME:
@@ -452,7 +449,7 @@ void CHostState::State_NewGame(void)
 
 #ifndef CLIENT_DLL
 	const bool bSplitScreenConnect = m_bSplitScreenConnect;
-	m_bSplitScreenConnect = 0;
+	m_bSplitScreenConnect = false;
 
 	if (!g_pServerGameClients) // Init Game if it ain't valid.
 	{

r5dev/engine/sys_mainwind.cpp

@@ -30,16 +30,10 @@ void CGame::PlayStartupVideos(void)
 //-----------------------------------------------------------------------------
 int CGame::WindowProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
 {
-	if (!ImguiSystem_IsInitialized())
+	if (!ImguiSystem()->IsInitialized())
 		return CGame__WindowProc(hWnd, uMsg, wParam, lParam);
 
-	const IEngine::EngineState_t state = g_pEngine->GetState();
-
-	if (state == IEngine::DLL_CLOSE ||
-		state == IEngine::DLL_RESTART)
-		return CGame__WindowProc(hWnd, uMsg, wParam, lParam);
-
-	ImGui_ImplWin32_WndProcHandler(hWnd, uMsg, wParam, lParam);
+	ImguiSystem()->MessageHandler(hWnd, uMsg, wParam, lParam);
 
 	if (uMsg == WM_KEYDOWN || uMsg == WM_SYSKEYDOWN)
 	{

r5dev/gameui/imgui_system.cpp

@@ -1,4 +1,8 @@
 //=============================================================================//
+// 
+// Purpose: Dear ImGui engine implementation
+// 
+//=============================================================================//
 
 #include "imgui/misc/imgui_snapshot.h"
 #include "engine/sys_mainwind.h"
@@ -9,16 +13,23 @@
 
 #include "imgui_system.h"
 
-static ImDrawDataSnapshot s_imguiSnapshotData;
-static bool s_imguiSystemInitialized = false;
-
-bool ImguiSystem_IsInitialized()
+//-----------------------------------------------------------------------------
+// Constructors/Destructors.
+//-----------------------------------------------------------------------------
+CImguiSystem::CImguiSystem()
 {
-	return s_imguiSystemInitialized;
+	m_systemInitState = ImguiSystemInitStage_e::IM_PENDING_INIT;
 }
 
-bool ImguiSystem_Init()
+//-----------------------------------------------------------------------------
+// Initializes the imgui system. If this fails, false would be returned and the
+// implementation won't run.
+//-----------------------------------------------------------------------------
+bool CImguiSystem::Init()
 {
+	Assert(ThreadInMainThread(), "CImguiSystem::Init() should only be called from the main thread!");
+	Assert(m_systemInitState == ImguiSystemInitStage_e::IM_PENDING_INIT, "CImguiSystem::Init() called recursively?");
+
 	///////////////////////////////////////////////////////////////////////////
 	IMGUI_CHECKVERSION();
 	ImGuiContext* const context = ImGui::CreateContext();
@@ -26,9 +37,12 @@ bool ImguiSystem_Init()
 	if (!context)
 		return false;
 
+	AUTO_LOCK(m_snapshotBufferMutex);
+	AUTO_LOCK(m_inputEventQueueMutex);
+
 	// This is required to disable the ctrl+tab menu as some users use this
 	// shortcut for other things in-game. See: https://github.com/ocornut/imgui/issues/4828
-	context->ConfigNavWindowingKeyNext = ImGuiMod_Shift | ImGuiKey_Space;
+	context->ConfigNavWindowingKeyNext = 0;
 	context->ConfigNavWindowingKeyPrev = 0;
 
 	ImGuiViewport* const vp = ImGui::GetMainViewport();
@@ -37,50 +51,81 @@ bool ImguiSystem_Init()
 	ImGuiIO& io = ImGui::GetIO();
 	io.ConfigFlags |= ImGuiConfigFlags_IsSRGB;
 
-	const bool win32ImplInit = ImGui_ImplWin32_Init(g_pGame->GetWindow());
+	if (!ImGui_ImplWin32_Init(g_pGame->GetWindow()) || 
+		!ImGui_ImplDX11_Init(D3D11Device(), D3D11DeviceContext()))
+	{
+		Assert(0);
 
-	if (!win32ImplInit)
+		m_systemInitState = ImguiSystemInitStage_e::IM_INIT_FAILURE;
 		return false;
+	}
 
-	const bool dx11ImplInit = ImGui_ImplDX11_Init(D3D11Device(), D3D11DeviceContext());
-
-	if (!dx11ImplInit)
-		return false;
-
-	s_imguiSystemInitialized = true;
+	m_systemInitState = ImguiSystemInitStage_e::IM_SYSTEM_INIT;
 	return true;
 }
 
-void ImguiSystem_Shutdown()
+//-----------------------------------------------------------------------------
+// Shuts the imgui system down, frees all allocated buffers.
+//-----------------------------------------------------------------------------
+void CImguiSystem::Shutdown()
 {
-	if (!s_imguiSystemInitialized)
+	Assert(ThreadInMainThread(), "CImguiSystem::Shutdown() should only be called from the main thread!");
+	Assert(m_systemInitState != ImguiSystemInitStage_e::IM_PENDING_INIT, "CImguiSystem::Shutdown() called recursively?");
+
+	// Nothing to shutdown.
+	if (m_systemInitState == ImguiSystemInitStage_e::IM_PENDING_INIT)
 		return;
 
-	s_imguiSystemInitialized = false;
+	AUTO_LOCK(m_snapshotBufferMutex);
+	AUTO_LOCK(m_inputEventQueueMutex);
+
+	m_systemInitState = ImguiSystemInitStage_e::IM_PENDING_INIT;
 
 	ImGui_ImplDX11_Shutdown();
 	ImGui_ImplWin32_Shutdown();
+
 	ImGui::DestroyContext();
 
-	s_imguiSnapshotData.Clear();
+	m_snapshotData.Clear();
 }
 
-void ImguiSystem_SwapBuffers()
+//-----------------------------------------------------------------------------
+// Copies currently drawn data into the snapshot buffer which is queued to be
+// rendered in the render thread. This should only be called from the same
+// thread SampleFrame() is being called from.
+//-----------------------------------------------------------------------------
+void CImguiSystem::SwapBuffers()
 {
-	if (!s_imguiSystemInitialized)
+	Assert(ThreadInMainThread(), "CImguiSystem::SwapBuffers() should only be called from the main thread!");
+
+	if (m_systemInitState < ImguiSystemInitStage_e::IM_FRAME_SAMPLED)
 		return;
 
+	AUTO_LOCK(m_snapshotBufferMutex);
 	ImDrawData* const drawData = ImGui::GetDrawData();
 
-	if (drawData)
-		s_imguiSnapshotData.SnapUsingSwap(drawData, ImGui::GetTime());
+	// Nothing has been drawn, nothing to swap
+	if (!drawData)
+		return;
+
+	m_snapshotData.SnapUsingSwap(drawData, ImGui::GetTime());
+
+	if (m_systemInitState == ImguiSystemInitStage_e::IM_FRAME_SAMPLED)
+		m_systemInitState = ImguiSystemInitStage_e::IM_FRAME_SWAPPED;
 }
 
-void ImguiSystem_SampleFrame()
+//-----------------------------------------------------------------------------
+// Draws the ImGui panels and applies all queued input events.
+//-----------------------------------------------------------------------------
+void CImguiSystem::SampleFrame()
 {
-	if (!s_imguiSystemInitialized)
+	Assert(ThreadInMainThread(), "CImguiSystem::SampleFrame() should only be called from the main thread!");
+
+	if (m_systemInitState == ImguiSystemInitStage_e::IM_PENDING_INIT)
 		return;
 
+	AUTO_LOCK(m_inputEventQueueMutex);
+
 	ImGui_ImplDX11_NewFrame();
 	ImGui_ImplWin32_NewFrame();
 
@@ -94,12 +139,37 @@ void ImguiSystem_SampleFrame()
 
 	ImGui::EndFrame();
 	ImGui::Render();
+
+	if (m_systemInitState == ImguiSystemInitStage_e::IM_SYSTEM_INIT)
+		m_systemInitState = ImguiSystemInitStage_e::IM_FRAME_SAMPLED;
 }
 
-void ImguiSystem_RenderFrame()
+//-----------------------------------------------------------------------------
+// Renders the drawn frame out which has been swapped to the snapshot buffer.
+//-----------------------------------------------------------------------------
+void CImguiSystem::RenderFrame()
 {
-	if (!s_imguiSystemInitialized)
+	if (m_systemInitState < ImguiSystemInitStage_e::IM_FRAME_SWAPPED)
 		return;
 
-	ImGui_ImplDX11_RenderDrawData(&s_imguiSnapshotData.DrawData);
+	AUTO_LOCK(m_snapshotBufferMutex);
+	ImGui_ImplDX11_RenderDrawData(&m_snapshotData.DrawData);
+}
+
+//-----------------------------------------------------------------------------
+// Window procedure handler.
+//-----------------------------------------------------------------------------
+LRESULT CImguiSystem::MessageHandler(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam)
+{
+	AUTO_LOCK(ImguiSystem()->m_inputEventQueueMutex);
+
+	extern IMGUI_IMPL_API LRESULT ImGui_ImplWin32_WndProcHandler(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam);
+	return ImGui_ImplWin32_WndProcHandler(hwnd, msg, wParam, lParam);
+}
+
+static CImguiSystem s_imguiSystem;
+
+CImguiSystem* ImguiSystem()
+{
+	return &s_imguiSystem;
 }

r5dev/gameui/imgui_system.h

@@ -1,14 +1,67 @@
+//=============================================================================//
+// 
+// Purpose: Dear ImGui engine implementation
+// 
+//=============================================================================//
 #ifndef IMGUI_SYSTEM_H
 #define IMGUI_SYSTEM_H
+#include "imgui/misc/imgui_snapshot.h"
 
-extern bool ImguiSystem_IsInitialized();
+class CImguiSystem
+{
+public:
+	CImguiSystem();
 
-extern bool ImguiSystem_Init();
-extern void ImguiSystem_Shutdown();
+	bool Init();
+	void Shutdown();
 
-extern void ImguiSystem_SwapBuffers();
+	void SwapBuffers();
 
-extern void ImguiSystem_SampleFrame();
-extern void ImguiSystem_RenderFrame();
+	void SampleFrame();
+	void RenderFrame();
+
+	// statics:
+	static LRESULT MessageHandler(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam);
+
+	// inlines:
+	inline bool IsInitialized() const
+	{
+		return m_systemInitState >= ImguiSystemInitStage_e::IM_SYSTEM_INIT;
+	};
+
+private:
+	enum class ImguiSystemInitStage_e
+	{
+		// When the system failed to initialize, the stage would be set to
+		// this.
+		IM_INIT_FAILURE = -1,
+
+		IM_PENDING_INIT,
+		IM_SYSTEM_INIT,
+
+		// State gets set to this when the first frame has been sampled.
+		IM_FRAME_SAMPLED,
+
+		// State gets set to this then buffers have been swapped for the first
+		// time.
+		IM_FRAME_SWAPPED
+	};
+
+	ImguiSystemInitStage_e m_systemInitState;
+	ImDrawDataSnapshot m_snapshotData;
+
+	// Mutex used during swapping and rendering, we draw the windows in the
+	// main thread, and render it in the render thread. The only place this
+	// mutex is used is during snapshot swapping and during rendering
+	mutable CThreadFastMutex m_snapshotBufferMutex;
+
+	// Mutex used between ImGui window procedure handling and drawing, see
+	// https://github.com/ocornut/imgui/issues/6895. In this engine the window
+	// is ran in thread separate from the main thread, therefore it needs a
+	// lock to control access as main calls SampleFrame().
+	mutable CThreadFastMutex m_inputEventQueueMutex;
+};
+
+CImguiSystem* ImguiSystem();
 
 #endif // IMGUI_SYSTEM_H

r5dev/materialsystem/cmaterialsystem.cpp

@@ -122,7 +122,7 @@ void* __fastcall DispatchDrawCall(int64_t a1, uint64_t a2, int a3, int a4, int64
 //---------------------------------------------------------------------------------
 ssize_t SpinPresent(void)
 {
-	ImguiSystem_RenderFrame();
+	ImguiSystem()->RenderFrame();
 
 	const ssize_t val = v_SpinPresent();
 	return val;
@@ -130,7 +130,9 @@ ssize_t SpinPresent(void)
 
 void* CMaterialSystem::SwapBuffers(CMaterialSystem* pMatSys)
 {
-	ImguiSystem_SwapBuffers();
+	ImguiSystem()->SampleFrame();
+	ImguiSystem()->SwapBuffers();
+
 	return CMaterialSystem__SwapBuffers(pMatSys);
 }
 

r5dev/thirdparty/imgui/misc/imgui_snapshot.h

@@ -3,6 +3,7 @@
 //  snapshot.SnapUsingSwap(ImGui::GetDrawData(), ImGui::GetTime());
 //  [...]
 //  ImGui_ImplDX11_RenderDrawData(&snapshot.DrawData);
+#pragma once
 
 // FIXME: Could store an ID in ImDrawList to make this easier for user.
 #include "imgui_internal.h" // ImPool<>, ImHashData

r5dev/windows/id3dx.cpp

@@ -350,8 +350,8 @@ void DirectX_Init()
 		Error(eDLL_T::COMMON, 0xBAD0C0DE, "Failed to detour process: error code = %08x\n", hr);
 	}
 
-	if (!ImguiSystem_Init())
-		Error(eDLL_T::MS, 0, "ImGui system initialization failed!\n");
+	if (!ImguiSystem()->Init())
+		Error(eDLL_T::COMMON, 0, "ImguiSystem()->Init() failed!\n");
 }
 
 void DirectX_Shutdown()
@@ -371,8 +371,7 @@ void DirectX_Shutdown()
 	// Commit the transaction
 	DetourTransactionCommit();
 
-	if (ImguiSystem_IsInitialized())
-		ImguiSystem_Shutdown();
+	ImguiSystem()->Shutdown();
 }
 
 void VDXGI::GetAdr(void) const

r5dev/windows/id3dx.h

@@ -8,7 +8,6 @@
 void DirectX_Init();
 void DirectX_Shutdown();
 
-extern LRESULT ImGui_ImplWin32_WndProcHandler(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam);
 extern HRESULT __stdcall Present(IDXGISwapChain* pSwapChain, UINT nSyncInterval, UINT nFlags);
 extern bool LoadTextureBuffer(unsigned char* buffer, int len, ID3D11ShaderResourceView** out_srv, int* out_width, int* out_height);