Fixed image locking

generators/graph/voxel_graph_node_db.cpp

@@ -148,8 +148,8 @@ inline T skew3(T x) {
 }
 
 // This is mostly useful for generating planets from an existing heightmap
-inline float sdf_sphere_heightmap(float x, float y, float z, float r, float m, Image &im, float min_h, float max_h,
-		float norm_x, float norm_y) {
+inline float sdf_sphere_heightmap(float x, float y, float z, float r, float m, const Image &im,
+		float min_h, float max_h, float norm_x, float norm_y) {
 
 	const float d = Math::sqrt(x * x + y * y + z * z) + 0.0001f;
 	const float sd = d - r;
@@ -170,7 +170,6 @@ inline float sdf_sphere_heightmap(float x, float y, float z, float r, float m, I
 	// in cases where we want to combine the same map in shaders
 	const float ys = skew3(ny);
 	const float uvy = -0.5f * ys + 0.5f;
-	// TODO Not great, but in Godot 4.0 we won't need to lock anymore.
 	// TODO Could use bicubic interpolation when the image is sampled at lower resolution than voxels
 	const float h = get_pixel_repeat_linear(im, uvx * norm_x, uvy * norm_y);
 	return sd - m * h;
@@ -882,8 +881,7 @@ VoxelGraphNodeDB::VoxelGraphNodeDB() {
 	}
 	{
 		struct Params {
-			// TODO Should be `const` but can't because of `lock()`
-			Image *image;
+			const Image *image;
 			const ImageRangeGrid *image_range_grid;
 		};
 		NodeType &t = types[VoxelGeneratorGraph::NODE_IMAGE_2D];
@@ -913,12 +911,10 @@ VoxelGraphNodeDB::VoxelGraphNodeDB() {
 			VoxelGraphRuntime::Buffer &out = ctx.get_output(0);
 			// TODO Allow to use bilinear filtering?
 			const Params p = ctx.get_params<Params>();
-			Image &im = *p.image;
-			im.lock();
+			const Image &im = *p.image;
 			for (uint32_t i = 0; i < out.size; ++i) {
 				out.data[i] = get_pixel_repeat(im, x.data[i], y.data[i]);
 			}
-			im.unlock();
 		};
 		t.range_analysis_func = [](RangeAnalysisContext &ctx) {
 			const Interval x = ctx.get_input(0);
@@ -1091,8 +1087,7 @@ VoxelGraphNodeDB::VoxelGraphNodeDB() {
 			float max_height;
 			float norm_x;
 			float norm_y;
-			// TODO Should be `const` but isn't because of `lock()`
-			Image *image;
+			const Image *image;
 			const ImageRangeGrid *image_range_grid;
 		};
 
@@ -1137,16 +1132,11 @@ VoxelGraphNodeDB::VoxelGraphNodeDB() {
 			VoxelGraphRuntime::Buffer &out = ctx.get_output(0);
 			// TODO Allow to use bilinear filtering?
 			const Params p = ctx.get_params<Params>();
-			Image &im = *p.image;
-			// TODO Because of this shitty locking system, images aren't read-only and as a result can't be used with more than one thread!
-			// - Copy data in a custom structure?
-			// - Lock all images after compilation and unlock them in destructor?
-			im.lock();
+			const Image &im = *p.image;
 			for (uint32_t i = 0; i < out.size; ++i) {
 				out.data[i] = sdf_sphere_heightmap(x.data[i], y.data[i], z.data[i],
 						p.radius, p.factor, im, p.min_height, p.max_height, p.norm_x, p.norm_y);
 			}
-			im.unlock();
 		};
 
 		t.range_analysis_func = [](RangeAnalysisContext &ctx) {

generators/graph/voxel_graph_runtime.cpp

@@ -34,6 +34,27 @@ inline void append(std::vector<uint8_t> &mem, const T &v) {
 	*(T *)(&mem[p]) = v;
 }
 
+// The Image lock() API prevents us from reading the same image in multiple threads.
+// Compiling makes a read-only copy of all resources, so we can lock all images up-front if successful.
+// This might no longer needed in Godot 4.
+void VoxelGraphRuntime::Program::lock_images() {
+	for (size_t i = 0; i < ref_resources.size(); ++i) {
+		Ref<Image> im = ref_resources[i];
+		if (im.is_valid()) {
+			im->lock();
+		}
+	}
+}
+
+void VoxelGraphRuntime::Program::unlock_images() {
+	for (size_t i = 0; i < ref_resources.size(); ++i) {
+		Ref<Image> im = ref_resources[i];
+		if (im.is_valid()) {
+			im->unlock();
+		}
+	}
+}
+
 VoxelGraphRuntime::VoxelGraphRuntime() {
 	clear();
 }
@@ -285,8 +306,10 @@ VoxelGraphRuntime::CompilationResult VoxelGraphRuntime::_compile(const ProgramGr
 		params_copy.resize(node->params.size());
 		for (size_t i = 0; i < node->params.size(); ++i) {
 			Variant v = node->params[i];
+
 			if (v.get_type() == Variant::OBJECT) {
 				Ref<Resource> res = v;
+
 				if (res.is_null()) {
 					// duplicate() is only available in Resource,
 					// so we have to limit to this instead of Reference or Object
@@ -296,10 +319,13 @@ VoxelGraphRuntime::CompilationResult VoxelGraphRuntime::_compile(const ProgramGr
 					result.node_id = node_id;
 					return result;
 				}
+
 				res = res->duplicate();
+
 				_program.ref_resources.push_back(res);
 				v = res;
 			}
+
 			params_copy[i] = v;
 		}
 
@@ -332,6 +358,8 @@ VoxelGraphRuntime::CompilationResult VoxelGraphRuntime::_compile(const ProgramGr
 								  SIZE_T_TO_VARIANT(_program.operations.size() * sizeof(float)),
 								  SIZE_T_TO_VARIANT(_program.buffer_count))));
 
+	_program.lock_images();
+
 	CompilationResult result;
 	result.success = true;
 	return result;

generators/graph/voxel_graph_runtime.h

@@ -31,6 +31,7 @@ public:
 	};
 
 	// Contains the data the program will modify while it runs.
+	// The same state can be re-used with multiple programs, but it should be prepared before doing that.
 	class State {
 	public:
 		inline const Buffer &get_buffer(uint16_t address) const {
@@ -276,9 +277,13 @@ private:
 				r.deleter(r.ptr);
 			}
 			heap_resources.clear();
+			unlock_images();
 			ref_resources.clear();
 			buffer_count = -1;
 		}
+
+		void lock_images();
+		void unlock_images();
 	};
 
 	Program _program;

generators/simple/voxel_generator_image.cpp

@@ -4,11 +4,11 @@
 
 namespace {
 
-inline float get_height_repeat(Image &im, int x, int y) {
+inline float get_height_repeat(const Image &im, int x, int y) {
 	return im.get_pixel(wrap(x, im.get_width()), wrap(y, im.get_height())).r;
 }
 
-inline float get_height_blurred(Image &im, int x, int y) {
+inline float get_height_blurred(const Image &im, int x, int y) {
 	float h = get_height_repeat(im, x, y);
 	h += get_height_repeat(im, x + 1, y);
 	h += get_height_repeat(im, x - 1, y);
@@ -25,6 +25,9 @@ VoxelGeneratorImage::VoxelGeneratorImage() {
 
 VoxelGeneratorImage::~VoxelGeneratorImage() {
 	memdelete(_parameters_lock);
+	if (_parameters.image.is_valid()) {
+		_parameters.image->unlock();
+	}
 }
 
 void VoxelGeneratorImage::set_image(Ref<Image> im) {
@@ -34,7 +37,15 @@ void VoxelGeneratorImage::set_image(Ref<Image> im) {
 	_image = im;
 	Ref<Image> copy = im.is_valid() ? im->duplicate() : Ref<Image>();
 	RWLockWrite wlock(_parameters_lock);
+	// lock() prevents us from reading the same image from multiple threads, so we lock it up-front.
+	// This might no longer be needed in Godot 4.
+	if (_parameters.image.is_valid()) {
+		_parameters.image->unlock();
+	}
 	_parameters.image = copy;
+	if (_parameters.image.is_valid()) {
+		_parameters.image->lock();
+	}
 }
 
 Ref<Image> VoxelGeneratorImage::get_image() const {
@@ -61,9 +72,7 @@ void VoxelGeneratorImage::generate_block(VoxelBlockRequest &input) {
 	}
 
 	ERR_FAIL_COND(params.image.is_null());
-	Image &image = **params.image;
-
-	image.lock();
+	const Image &image = **params.image;
 
 	if (params.blur_enabled) {
 		VoxelGeneratorHeightmap::generate(
@@ -77,8 +86,6 @@ void VoxelGeneratorImage::generate_block(VoxelBlockRequest &input) {
 				input.origin_in_voxels, input.lod);
 	}
 
-	image.unlock();
-
 	out_buffer.compress_uniform_channels();
 }