From 361c5bc10efcfc0a682e731e232cbf34b74375b3 Mon Sep 17 00:00:00 2001
From: Max Roncace <me@caseif.net>
Date: Mon, 4 Nov 2024 21:16:38 -0500
Subject: [PATCH] Fix race condition while loading lights

This fixes a race condition where the renderer could start
reading the vector of lights at the same time that newly-loaded
lights were being added to it, thus causing a segfault or other
weird behavior.

This is done by creating separate queues and a corresponding mutex
for mutations to the list of lights in the renderer and flushing
them before starting to draw the lights. Because this is basically
just copying pointers, the actual timespan the mutex is held by the
renderer is very low.
---
 src/graphics/gfxman.cpp          |  2 +-
 src/graphics/opengl/renderer.cpp |  3 +++
 src/graphics/renderer.cpp        | 23 ++++++++++++++++++++---
 src/graphics/renderer.h          |  6 ++++++
 4 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/graphics/gfxman.cpp b/src/graphics/gfxman.cpp
index efe14ce..ce807eb 100644
--- a/src/graphics/gfxman.cpp
+++ b/src/graphics/gfxman.cpp
@@ -114,7 +114,7 @@ void GraphicsManager::removeLight(Light *light) {
 	if (!_renderer)
 		return;
 
-	_renderer->addLight(light);
+	_renderer->removeLight(light);
 }
 
 TexturePtr GraphicsManager::createTexture(ImageDecoder &&decoder, const std::string &label) {
diff --git a/src/graphics/opengl/renderer.cpp b/src/graphics/opengl/renderer.cpp
index 2b6f9ee..d247bd8 100644
--- a/src/graphics/opengl/renderer.cpp
+++ b/src/graphics/opengl/renderer.cpp
@@ -502,6 +502,9 @@ void Renderer::drawWorld(const std::string &stage) {
 void Renderer::drawLights() {
 	pushDebug("Draw Lights");
 
+	// flush any new additions or removals
+	flushLights();
+
 	auto stencilProgram = getProgram("deferredlight", "render_stencil", 0);
 	auto pointlightProgram = getProgram("deferredlight", "pointlight", 0);
 
diff --git a/src/graphics/renderer.cpp b/src/graphics/renderer.cpp
index c305098..af87383 100644
--- a/src/graphics/renderer.cpp
+++ b/src/graphics/renderer.cpp
@@ -112,12 +112,29 @@ void Graphics::Renderer::removeGUIElement(Graphics::GUIElement *gui) {
 }
 
 void Graphics::Renderer::addLight(Graphics::Light *light) {
-	_lights.emplace_back(light);
+	_light_queues_mutex.lock();
+	_light_add_queue.push(light);
+	_light_queues_mutex.unlock();
 }
 
 void Graphics::Renderer::removeLight(Graphics::Light *light) {
-	const auto iter = std::remove(_lights.begin(), _lights.end(), light);
-	_lights.erase(iter);
+	_light_queues_mutex.lock();
+	_light_remove_queue.push(light);
+	_light_queues_mutex.unlock();
+}
+
+void Graphics::Renderer::flushLights() {
+	_light_queues_mutex.lock();
+	while (!_light_add_queue.empty()) {
+		_lights.push_back(_light_add_queue.front());
+		_light_add_queue.pop();
+	}
+	while (!_light_remove_queue.empty()) {
+		const auto iter = std::remove(_lights.begin(), _lights.end(), _light_remove_queue.front());
+		_lights.erase(iter);
+		_light_remove_queue.pop();
+	}
+	_light_queues_mutex.unlock();
 }
 
 void Graphics::Renderer::setCamera(Graphics::Camera &camera) {
diff --git a/src/graphics/renderer.h b/src/graphics/renderer.h
index 4ca01bd..41c90c6 100644
--- a/src/graphics/renderer.h
+++ b/src/graphics/renderer.h
@@ -21,6 +21,8 @@
 #ifndef AWE_RENDERER_H
 #define AWE_RENDERER_H
 
+#include <mutex>
+#include <queue>
 #include <vector>
 
 #include "src/common/frustrum.h"
@@ -54,6 +56,7 @@ class Renderer {
 	void removeGUIElement(GUIElement *gui);
 	void addLight(Light *light);
 	void removeLight(Light *light);
+	void flushLights();
 
 	void setCamera(Camera &camera);
 
@@ -158,6 +161,9 @@ class Renderer {
 	std::vector<ImGuiElement *> _imguiElements;
 	std::vector<GUIElement *> _guiElements;
 	std::vector<Light *> _lights;
+	std::queue<Light *> _light_add_queue;
+	std::queue<Light *> _light_remove_queue;
+	std::mutex _light_queues_mutex;
 };
 
 } // End of namespace Graphics