Merge pull request #91630 from RandomShaper/enh_mat_sh_update

Let materials' shaders update happen on loader threads

core/object/message_queue.cpp

@@ -224,64 +224,7 @@ void CallQueue::_call_function(const Callable &p_callable, const Variant *p_args
 	}
 }
 
-Error CallQueue::_transfer_messages_to_main_queue() {
-	if (pages.size() == 0) {
-		return OK;
-	}
-
-	CallQueue *mq = MessageQueue::main_singleton;
-	DEV_ASSERT(!mq->allocator_is_custom && !allocator_is_custom); // Transferring pages is only safe if using the same alloator parameters.
-
-	mq->mutex.lock();
-
-	// Here we're transferring the data from this queue to the main one.
-	// However, it's very unlikely big amounts of messages will be queued here,
-	// so PagedArray/Pool would be overkill. Also, in most cases the data will fit
-	// an already existing page of the main queue.
-
-	// Let's see if our first (likely only) page fits the current target queue page.
-	uint32_t src_page = 0;
-	{
-		if (mq->pages_used) {
-			uint32_t dst_page = mq->pages_used - 1;
-			uint32_t dst_offset = mq->page_bytes[dst_page];
-			if (dst_offset + page_bytes[0] < uint32_t(PAGE_SIZE_BYTES)) {
-				memcpy(mq->pages[dst_page]->data + dst_offset, pages[0]->data, page_bytes[0]);
-				mq->page_bytes[dst_page] += page_bytes[0];
-				src_page++;
-			}
-		}
-	}
-
-	// Any other possibly existing source page needs to be added.
-
-	if (mq->pages_used + (pages_used - src_page) > mq->max_pages) {
-		fprintf(stderr, "Failed appending thread queue. Message queue out of memory. %s\n", mq->error_text.utf8().get_data());
-		mq->statistics();
-		mq->mutex.unlock();
-		return ERR_OUT_OF_MEMORY;
-	}
-
-	for (; src_page < pages_used; src_page++) {
-		mq->_add_page();
-		memcpy(mq->pages[mq->pages_used - 1]->data, pages[src_page]->data, page_bytes[src_page]);
-		mq->page_bytes[mq->pages_used - 1] = page_bytes[src_page];
-	}
-
-	mq->mutex.unlock();
-
-	page_bytes[0] = 0;
-	pages_used = 1;
-
-	return OK;
-}
-
 Error CallQueue::flush() {
-	// Thread overrides are not meant to be flushed, but appended to the main one.
-	if (unlikely(this == MessageQueue::thread_singleton)) {
-		return _transfer_messages_to_main_queue();
-	}
-
 	LOCK_MUTEX;
 
 	if (pages.size() == 0) {

core/object/message_queue.h

@@ -98,8 +98,6 @@ private:
 		}
 	}
 
-	Error _transfer_messages_to_main_queue();
-
 	void _add_page();
 
 	void _call_function(const Callable &p_callable, const Variant *p_args, int p_argcount, bool p_show_error);

scene/resources/canvas_item_material.cpp

@@ -33,13 +33,11 @@
 #include "core/version.h"
 
 Mutex CanvasItemMaterial::material_mutex;
-SelfList<CanvasItemMaterial>::List *CanvasItemMaterial::dirty_materials = nullptr;
+SelfList<CanvasItemMaterial>::List CanvasItemMaterial::dirty_materials;
 HashMap<CanvasItemMaterial::MaterialKey, CanvasItemMaterial::ShaderData, CanvasItemMaterial::MaterialKey> CanvasItemMaterial::shader_map;
 CanvasItemMaterial::ShaderNames *CanvasItemMaterial::shader_names = nullptr;
 
 void CanvasItemMaterial::init_shaders() {
-	dirty_materials = memnew(SelfList<CanvasItemMaterial>::List);
-
 	shader_names = memnew(ShaderNames);
 
 	shader_names->particles_anim_h_frames = "particles_anim_h_frames";
@@ -48,14 +46,13 @@ void CanvasItemMaterial::init_shaders() {
 }
 
 void CanvasItemMaterial::finish_shaders() {
-	memdelete(dirty_materials);
+	dirty_materials.clear();
+
 	memdelete(shader_names);
-	dirty_materials = nullptr;
+	shader_names = nullptr;
 }
 
 void CanvasItemMaterial::_update_shader() {
-	dirty_materials->remove(&element);
-
 	MaterialKey mk = _compute_key();
 	if (mk.key == current_key.key) {
 		return; //no update required in the end
@@ -153,8 +150,9 @@ void CanvasItemMaterial::_update_shader() {
 void CanvasItemMaterial::flush_changes() {
 	MutexLock lock(material_mutex);
 
-	while (dirty_materials->first()) {
-		dirty_materials->first()->self()->_update_shader();
+	while (dirty_materials.first()) {
+		dirty_materials.first()->self()->_update_shader();
+		dirty_materials.first()->remove_from_list();
 	}
 }
 
@@ -162,16 +160,10 @@ void CanvasItemMaterial::_queue_shader_change() {
 	MutexLock lock(material_mutex);
 
 	if (_is_initialized() && !element.in_list()) {
-		dirty_materials->add(&element);
+		dirty_materials.add(&element);
 	}
 }
 
-bool CanvasItemMaterial::_is_shader_dirty() const {
-	MutexLock lock(material_mutex);
-
-	return element.in_list();
-}
-
 void CanvasItemMaterial::set_blend_mode(BlendMode p_blend_mode) {
 	blend_mode = p_blend_mode;
 	_queue_shader_change();
@@ -288,7 +280,7 @@ CanvasItemMaterial::CanvasItemMaterial() :
 
 	current_key.invalid_key = 1;
 
-	_mark_initialized(callable_mp(this, &CanvasItemMaterial::_queue_shader_change));
+	_mark_initialized(callable_mp(this, &CanvasItemMaterial::_queue_shader_change), callable_mp(this, &CanvasItemMaterial::_update_shader));
 }
 
 CanvasItemMaterial::~CanvasItemMaterial() {

scene/resources/canvas_item_material.h

@@ -98,12 +98,11 @@ private:
 	}
 
 	static Mutex material_mutex;
-	static SelfList<CanvasItemMaterial>::List *dirty_materials;
+	static SelfList<CanvasItemMaterial>::List dirty_materials;
 	SelfList<CanvasItemMaterial> element;
 
 	void _update_shader();
 	_FORCE_INLINE_ void _queue_shader_change();
-	_FORCE_INLINE_ bool _is_shader_dirty() const;
 
 	BlendMode blend_mode = BLEND_MODE_MIX;
 	LightMode light_mode = LIGHT_MODE_NORMAL;

scene/resources/material.cpp

@@ -82,24 +82,25 @@ void Material::_validate_property(PropertyInfo &p_property) const {
 	}
 }
 
-void Material::_mark_initialized(const Callable &p_queue_shader_change_callable) {
+void Material::_mark_ready() {
+	init_state = INIT_STATE_INITIALIZING;
+}
+
+void Material::_mark_initialized(const Callable &p_add_to_dirty_list, const Callable &p_update_shader) {
 	// If this is happening as part of resource loading, it is not safe to queue the update
-	// as an addition to the dirty list, unless the load is happening on the main thread.
-	if (ResourceLoader::is_within_load() && Thread::get_caller_id() != Thread::get_main_id()) {
+	// as an addition to the dirty list. It would be if the load is happening on the main thread,
+	// but even so we'd rather perform the update directly instead of using the dirty list.
+	if (ResourceLoader::is_within_load()) {
 		DEV_ASSERT(init_state != INIT_STATE_READY);
 		if (init_state == INIT_STATE_UNINITIALIZED) { // Prevent queueing twice.
-			// Let's mark this material as being initialized.
 			init_state = INIT_STATE_INITIALIZING;
-			// Knowing that the ResourceLoader will eventually feed deferred calls into the main message queue, let's do these:
-			// 1. Queue setting the init state to INIT_STATE_READY finally.
-			callable_mp(this, &Material::_mark_initialized).bind(p_queue_shader_change_callable).call_deferred();
-			// 2. Queue an individual update of this material.
-			p_queue_shader_change_callable.call_deferred();
+			callable_mp(this, &Material::_mark_ready).call_deferred();
+			p_update_shader.call_deferred();
 		}
 	} else {
 		// Straightforward conditions.
 		init_state = INIT_STATE_READY;
-		p_queue_shader_change_callable.callv(Array());
+		p_add_to_dirty_list.call();
 	}
 }
 
@@ -603,8 +604,6 @@ void BaseMaterial3D::finish_shaders() {
 }
 
 void BaseMaterial3D::_update_shader() {
-	dirty_materials.remove(&element);
-
 	MaterialKey mk = _compute_key();
 	if (mk == current_key) {
 		return; //no update required in the end
@@ -1855,6 +1854,7 @@ void BaseMaterial3D::flush_changes() {
 
 	while (dirty_materials.first()) {
 		dirty_materials.first()->self()->_update_shader();
+		dirty_materials.first()->remove_from_list();
 	}
 }
 
@@ -1866,12 +1866,6 @@ void BaseMaterial3D::_queue_shader_change() {
 	}
 }
 
-bool BaseMaterial3D::_is_shader_dirty() const {
-	MutexLock lock(material_mutex);
-
-	return element.in_list();
-}
-
 void BaseMaterial3D::set_albedo(const Color &p_albedo) {
 	albedo = p_albedo;
 
@@ -2824,7 +2818,7 @@ BaseMaterial3D::EmissionOperator BaseMaterial3D::get_emission_operator() const {
 
 RID BaseMaterial3D::get_shader_rid() const {
 	MutexLock lock(material_mutex);
-	if (element.in_list()) { // _is_shader_dirty() would create anoder mutex lock
+	if (element.in_list()) {
 		((BaseMaterial3D *)this)->_update_shader();
 	}
 	ERR_FAIL_COND_V(!shader_map.has(current_key), RID());
@@ -3412,7 +3406,7 @@ BaseMaterial3D::BaseMaterial3D(bool p_orm) :
 
 	current_key.invalid_key = 1;
 
-	_mark_initialized(callable_mp(this, &BaseMaterial3D::_queue_shader_change));
+	_mark_initialized(callable_mp(this, &BaseMaterial3D::_queue_shader_change), callable_mp(this, &BaseMaterial3D::_update_shader));
 }
 
 BaseMaterial3D::~BaseMaterial3D() {

scene/resources/material.h

@@ -62,7 +62,8 @@ protected:
 
 	void _validate_property(PropertyInfo &p_property) const;
 
-	void _mark_initialized(const Callable &p_queue_shader_change_callable);
+	void _mark_ready();
+	void _mark_initialized(const Callable &p_add_to_dirty_list, const Callable &p_update_shader);
 	bool _is_initialized() { return init_state == INIT_STATE_READY; }
 
 	GDVIRTUAL0RC(RID, _get_shader_rid)
@@ -466,7 +467,6 @@ private:
 
 	void _update_shader();
 	_FORCE_INLINE_ void _queue_shader_change();
-	_FORCE_INLINE_ bool _is_shader_dirty() const;
 
 	bool orm;
 

scene/resources/particle_process_material.cpp

@@ -33,14 +33,12 @@
 #include "core/version.h"
 
 Mutex ParticleProcessMaterial::material_mutex;
-SelfList<ParticleProcessMaterial>::List *ParticleProcessMaterial::dirty_materials = nullptr;
+SelfList<ParticleProcessMaterial>::List ParticleProcessMaterial::dirty_materials;
 HashMap<ParticleProcessMaterial::MaterialKey, ParticleProcessMaterial::ShaderData, ParticleProcessMaterial::MaterialKey> ParticleProcessMaterial::shader_map;
 RBSet<String> ParticleProcessMaterial::min_max_properties;
 ParticleProcessMaterial::ShaderNames *ParticleProcessMaterial::shader_names = nullptr;
 
 void ParticleProcessMaterial::init_shaders() {
-	dirty_materials = memnew(SelfList<ParticleProcessMaterial>::List);
-
 	shader_names = memnew(ShaderNames);
 
 	shader_names->direction = "direction";
@@ -140,15 +138,13 @@ void ParticleProcessMaterial::init_shaders() {
 }
 
 void ParticleProcessMaterial::finish_shaders() {
-	memdelete(dirty_materials);
-	dirty_materials = nullptr;
+	dirty_materials.clear();
 
 	memdelete(shader_names);
+	shader_names = nullptr;
 }
 
 void ParticleProcessMaterial::_update_shader() {
-	dirty_materials->remove(&element);
-
 	MaterialKey mk = _compute_key();
 	if (mk == current_key) {
 		return; //no update required in the end
@@ -1170,8 +1166,9 @@ void ParticleProcessMaterial::_update_shader() {
 void ParticleProcessMaterial::flush_changes() {
 	MutexLock lock(material_mutex);
 
-	while (dirty_materials->first()) {
-		dirty_materials->first()->self()->_update_shader();
+	while (dirty_materials.first()) {
+		dirty_materials.first()->self()->_update_shader();
+		dirty_materials.first()->remove_from_list();
 	}
 }
 
@@ -1179,16 +1176,10 @@ void ParticleProcessMaterial::_queue_shader_change() {
 	MutexLock lock(material_mutex);
 
 	if (_is_initialized() && !element.in_list()) {
-		dirty_materials->add(&element);
+		dirty_materials.add(&element);
 	}
 }
 
-bool ParticleProcessMaterial::_is_shader_dirty() const {
-	MutexLock lock(material_mutex);
-
-	return element.in_list();
-}
-
 bool ParticleProcessMaterial::has_min_max_property(const String &p_name) {
 	return min_max_properties.has(p_name);
 }
@@ -2330,7 +2321,7 @@ ParticleProcessMaterial::ParticleProcessMaterial() :
 
 	current_key.invalid_key = 1;
 
-	_mark_initialized(callable_mp(this, &ParticleProcessMaterial::_queue_shader_change));
+	_mark_initialized(callable_mp(this, &ParticleProcessMaterial::_queue_shader_change), callable_mp(this, &ParticleProcessMaterial::_update_shader));
 }
 
 ParticleProcessMaterial::~ParticleProcessMaterial() {

scene/resources/particle_process_material.h

@@ -186,7 +186,7 @@ private:
 	}
 
 	static Mutex material_mutex;
-	static SelfList<ParticleProcessMaterial>::List *dirty_materials;
+	static SelfList<ParticleProcessMaterial>::List dirty_materials;
 
 	struct ShaderNames {
 		StringName direction;
@@ -293,7 +293,6 @@ private:
 
 	void _update_shader();
 	_FORCE_INLINE_ void _queue_shader_change();
-	_FORCE_INLINE_ bool _is_shader_dirty() const;
 
 	Vector3 direction;
 	float spread = 0.0f;