From f12164d311339452ddde28f2b27161d9d185364f Mon Sep 17 00:00:00 2001
From: kleonc <9283098+kleonc@users.noreply.github.com>
Date: Fri, 21 Jul 2023 14:32:38 +0200
Subject: [PATCH] Fix CanvasModulate logic for updating canvas modulate

---
 scene/2d/canvas_modulate.cpp | 67 +++++++++++++++++++++++++++---------
 scene/2d/canvas_modulate.h   |  8 +++++
 2 files changed, 59 insertions(+), 16 deletions(-)

diff --git a/scene/2d/canvas_modulate.cpp b/scene/2d/canvas_modulate.cpp
index 7dd2e75f097..2c5c6a1a168 100644
--- a/scene/2d/canvas_modulate.cpp
+++ b/scene/2d/canvas_modulate.cpp
@@ -30,32 +30,67 @@
 
 #include "canvas_modulate.h"
 
+void CanvasModulate::_on_in_canvas_visibility_changed(bool p_new_visibility) {
+	RID canvas = get_canvas();
+	StringName group_name = "_canvas_modulate_" + itos(canvas.get_id());
+
+	ERR_FAIL_COND_MSG(p_new_visibility == is_in_group(group_name), vformat("CanvasModulate becoming %s in the canvas already %s in the modulate group. Buggy logic, please report.", p_new_visibility ? "visible" : "invisible", p_new_visibility ? "was" : "was not"));
+
+	if (p_new_visibility) {
+		bool has_active_canvas_modulate = get_tree()->has_group(group_name); // Group would be removed if empty; otherwise one CanvasModulate within must be active.
+		add_to_group(group_name);
+		if (!has_active_canvas_modulate) {
+			is_active = true;
+			RS::get_singleton()->canvas_set_modulate(canvas, color);
+		}
+	} else {
+		remove_from_group(group_name);
+		if (is_active) {
+			is_active = false;
+			CanvasModulate *new_active = Object::cast_to<CanvasModulate>(get_tree()->get_first_node_in_group(group_name));
+			if (new_active) {
+				new_active->is_active = true;
+				RS::get_singleton()->canvas_set_modulate(canvas, new_active->color);
+			} else {
+				RS::get_singleton()->canvas_set_modulate(canvas, Color(1, 1, 1, 1));
+			}
+		}
+	}
+
+	update_configuration_warnings();
+}
+
 void CanvasModulate::_notification(int p_what) {
 	switch (p_what) {
 		case NOTIFICATION_ENTER_CANVAS: {
-			if (is_visible_in_tree()) {
-				RS::get_singleton()->canvas_set_modulate(get_canvas(), color);
-				add_to_group("_canvas_modulate_" + itos(get_canvas().get_id()));
+			is_in_canvas = true;
+			bool visible_in_tree = is_visible_in_tree();
+			if (visible_in_tree) {
+				_on_in_canvas_visibility_changed(true);
 			}
+			was_visible_in_tree = visible_in_tree;
 		} break;
 
 		case NOTIFICATION_EXIT_CANVAS: {
-			if (is_visible_in_tree()) {
-				RS::get_singleton()->canvas_set_modulate(get_canvas(), Color(1, 1, 1, 1));
-				remove_from_group("_canvas_modulate_" + itos(get_canvas().get_id()));
+			is_in_canvas = false;
+			if (was_visible_in_tree) {
+				_on_in_canvas_visibility_changed(false);
 			}
 		} break;
 
 		case NOTIFICATION_VISIBILITY_CHANGED: {
-			if (is_visible_in_tree()) {
-				RS::get_singleton()->canvas_set_modulate(get_canvas(), color);
-				add_to_group("_canvas_modulate_" + itos(get_canvas().get_id()));
-			} else {
-				RS::get_singleton()->canvas_set_modulate(get_canvas(), Color(1, 1, 1, 1));
-				remove_from_group("_canvas_modulate_" + itos(get_canvas().get_id()));
+			if (!is_in_canvas) {
+				return;
 			}
 
-			update_configuration_warnings();
+			bool visible_in_tree = is_visible_in_tree();
+			if (visible_in_tree == was_visible_in_tree) {
+				return;
+			}
+
+			_on_in_canvas_visibility_changed(visible_in_tree);
+
+			was_visible_in_tree = visible_in_tree;
 		} break;
 	}
 }
@@ -69,7 +104,7 @@ void CanvasModulate::_bind_methods() {
 
 void CanvasModulate::set_color(const Color &p_color) {
 	color = p_color;
-	if (is_visible_in_tree()) {
+	if (is_active) {
 		RS::get_singleton()->canvas_set_modulate(get_canvas(), color);
 	}
 }
@@ -81,12 +116,12 @@ Color CanvasModulate::get_color() const {
 PackedStringArray CanvasModulate::get_configuration_warnings() const {
 	PackedStringArray warnings = Node::get_configuration_warnings();
 
-	if (is_visible_in_tree() && is_inside_tree()) {
+	if (is_in_canvas && is_visible_in_tree()) {
 		List<Node *> nodes;
 		get_tree()->get_nodes_in_group("_canvas_modulate_" + itos(get_canvas().get_id()), &nodes);
 
 		if (nodes.size() > 1) {
-			warnings.push_back(RTR("Only one visible CanvasModulate is allowed per scene (or set of instantiated scenes). The first created one will work, while the rest will be ignored."));
+			warnings.push_back(RTR("Only one visible CanvasModulate is allowed per canvas.\nWhen there are more than one, only one of them will be active. Which one is undefined."));
 		}
 	}
 
diff --git a/scene/2d/canvas_modulate.h b/scene/2d/canvas_modulate.h
index 3b11cf71f19..08ded52e233 100644
--- a/scene/2d/canvas_modulate.h
+++ b/scene/2d/canvas_modulate.h
@@ -38,6 +38,14 @@ class CanvasModulate : public Node2D {
 
 	Color color = Color(1, 1, 1, 1);
 
+	// CanvasModulate is in canvas-specific modulate group when both in canvas and visible in tree.
+	// Exactly one CanvasModulate in each such non-empty group is active.
+	bool is_in_canvas = false;
+	bool was_visible_in_tree = false; // Relevant only when in canvas.
+	bool is_active = false;
+
+	void _on_in_canvas_visibility_changed(bool p_new_visibility);
+
 protected:
 	void _notification(int p_what);
 	static void _bind_methods();