Merge pull request #38114 from lawnjelly/kessel_rulerfix

Fix batch translate to colored synchronization error

drivers/gles2/rasterizer_canvas_gles2.cpp

@@ -85,6 +85,19 @@ void RasterizerCanvasGLES2::RenderItemState::reset() {
 	joined_item = nullptr;
 }
 
+// just translate the color into something easily readable and not too verbose
+String RasterizerCanvasGLES2::BatchColor::to_string() const {
+	String sz = "{";
+	const float *data = get_data();
+	for (int c = 0; c < 4; c++) {
+		float f = data[c];
+		int val = ((f * 255.0f) + 0.5f);
+		sz += String(Variant(val)) + " ";
+	}
+	sz += "}";
+	return sz;
+}
+
 RasterizerStorageGLES2::Texture *RasterizerCanvasGLES2::_get_canvas_texture(const RID &p_texture) const {
 	if (p_texture.is_valid()) {
 
@@ -494,17 +507,17 @@ void RasterizerCanvasGLES2::_batch_translate_to_colored() {
 	for (int n = 0; n < bdata.batches.size(); n++) {
 		const Batch &source_batch = bdata.batches[n];
 
-		bool needs_new_batch;
+		bool needs_new_batch = true;
 
 		if (dest_batch) {
-			// is the dest batch the same except for the color?
+			if (dest_batch->type == source_batch.type) {
-			if ((dest_batch->batch_texture_id == source_batch.batch_texture_id) && (dest_batch->type == source_batch.type)) {
+				if (source_batch.type == Batch::BT_RECT) {
+					if (dest_batch->batch_texture_id == source_batch.batch_texture_id) {
 						// add to previous batch
 						dest_batch->num_commands += source_batch.num_commands;
 						needs_new_batch = false;
 
 						// create the colored verts (only if not default)
-				if (source_batch.type != Batch::BT_DEFAULT) {
 						int first_vert = source_batch.first_quad * 4;
 						int end_vert = 4 * (source_batch.first_quad + source_batch.num_commands);
 
@@ -518,13 +531,20 @@ void RasterizerCanvasGLES2::_batch_translate_to_colored() {
 							cv->uv = bv.uv;
 							cv->col = source_batch.color;
 						}
-				}
+					} // textures match
 				} else {
-				needs_new_batch = true;
+					// default
-			}
+					// we can still join, but only under special circumstances
-		} else {
+					// does this ever happen? not sure at this stage, but left for future expansion
-			needs_new_batch = true;
+					uint32_t source_last_command = source_batch.first_command + source_batch.num_commands;
+					if (source_last_command == dest_batch->first_command) {
+						dest_batch->num_commands += source_batch.num_commands;
+						needs_new_batch = false;
+					} // if the commands line up exactly
 				}
+			} // if both batches are the same type
+
+		} // if dest batch is valid
 
 		if (needs_new_batch) {
 			dest_batch = bdata.batches_temp.request();
@@ -646,9 +666,69 @@ void RasterizerCanvasGLES2::_batch_render_rects(const Batch &p_batch, Rasterizer
 	glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);
 }
 
+#ifdef DEBUG_ENABLED
+String RasterizerCanvasGLES2::get_command_type_string(const Item::Command &p_command) const {
+	String sz = "";
+
+	switch (p_command.type) {
+		default:
+			break;
+		case Item::Command::TYPE_LINE: {
+			sz = "l";
+		} break;
+		case Item::Command::TYPE_POLYLINE: {
+			sz = "PL";
+		} break;
+		case Item::Command::TYPE_RECT: {
+			sz = "r";
+		} break;
+		case Item::Command::TYPE_NINEPATCH: {
+			sz = "n";
+		} break;
+		case Item::Command::TYPE_PRIMITIVE: {
+			sz = "PR";
+		} break;
+		case Item::Command::TYPE_POLYGON: {
+			sz = "p";
+		} break;
+		case Item::Command::TYPE_MESH: {
+			sz = "m";
+		} break;
+		case Item::Command::TYPE_MULTIMESH: {
+			sz = "MM";
+		} break;
+		case Item::Command::TYPE_PARTICLES: {
+			sz = "PA";
+		} break;
+		case Item::Command::TYPE_CIRCLE: {
+			sz = "c";
+		} break;
+		case Item::Command::TYPE_TRANSFORM: {
+			sz = "t";
+
+			// add a bit more info in debug build
+			const Item::CommandTransform *transform = static_cast<const Item::CommandTransform *>(&p_command);
+			const Transform2D &mat = transform->xform;
+
+			sz += " ";
+			sz += String(Variant(mat.elements[2]));
+			sz += " ";
+		} break;
+		case Item::Command::TYPE_CLIP_IGNORE: {
+			sz = "CI";
+		} break;
+	} // switch
+
+	return sz;
+}
+
 void RasterizerCanvasGLES2::diagnose_batches(Item::Command *const *p_commands) {
 	int num_batches = bdata.batches.size();
 
+	BatchColor curr_color;
+	curr_color.set(Color(-1, -1, -1, -1));
+	bool first_color_change = true;
+
 	for (int batch_num = 0; batch_num < num_batches; batch_num++) {
 		const Batch &batch = bdata.batches[batch_num];
 		bdata.frame_string += "\t\tbatch ";
@@ -656,21 +736,42 @@ void RasterizerCanvasGLES2::diagnose_batches(Item::Command *const *p_commands) {
 		switch (batch.type) {
 			case Batch::BT_RECT: {
 				bdata.frame_string += "R ";
+				bdata.frame_string += itos(batch.first_command) + "-";
 				bdata.frame_string += itos(batch.num_commands);
 				bdata.frame_string += " [" + itos(batch.batch_texture_id) + "]";
+
+				bdata.frame_string += " " + batch.color.to_string();
+
 				if (batch.num_commands > 1) {
-					bdata.frame_string += " MULTI\n";
+					bdata.frame_string += " MULTI";
-				} else {
-					bdata.frame_string += "\n";
 				}
+				if (curr_color != batch.color) {
+					curr_color = batch.color;
+					if (!first_color_change) {
+						bdata.frame_string += " color";
+					} else {
+						first_color_change = false;
+					}
+				}
+				bdata.frame_string += "\n";
 			} break;
 			default: {
 				bdata.frame_string += "D ";
-				bdata.frame_string += itos(batch.num_commands) + "\n";
+				bdata.frame_string += itos(batch.first_command) + "-";
+				bdata.frame_string += itos(batch.num_commands) + " ";
+
+				int num_show = MIN(batch.num_commands, 16);
+				for (int n = 0; n < num_show; n++) {
+					const Item::Command &comm = *p_commands[batch.first_command + n];
+					bdata.frame_string += get_command_type_string(comm) + " ";
+				}
+
+				bdata.frame_string += "\n";
 			} break;
 		}
 	}
 }
+#endif
 
 void RasterizerCanvasGLES2::render_batches(Item::Command *const *p_commands, Item *p_current_clip, bool &r_reclip, RasterizerStorageGLES2::Material *p_material) {
 
@@ -1592,9 +1693,11 @@ void RasterizerCanvasGLES2::flush_render_batches(Item *p_first_item, Item *p_cur
 
 	Item::Command *const *commands = p_first_item->commands.ptr();
 
+#ifdef DEBUG_ENABLED
 	if (bdata.diagnose_frame) {
 		diagnose_batches(commands);
 	}
+#endif
 
 	render_batches(commands, p_current_clip, r_reclip, p_material);
 }

drivers/gles2/rasterizer_canvas_gles2.h

@@ -67,10 +67,15 @@ class RasterizerCanvasGLES2 : public RasterizerCanvasBaseGLES2 {
 			b = p_c.b;
 			a = p_c.a;
 		}
+		bool operator==(const BatchColor &p_c) const {
+			return (r == p_c.r) && (g == p_c.g) && (b == p_c.b) && (a == p_c.a);
+		}
+		bool operator!=(const BatchColor &p_c) const { return (*this == p_c) == false; }
 		bool equals(const Color &p_c) const {
 			return (r == p_c.r) && (g == p_c.g) && (b == p_c.b) && (a == p_c.a);
 		}
 		const float *get_data() const { return &r; }
+		String to_string() const;
 	};
 
 	struct BatchVertex {
@@ -272,8 +277,11 @@ private:
 	bool _light_scissor_begin(const Rect2 &p_item_rect, const Transform2D &p_light_xform, const Rect2 &p_light_rect) const;
 	void _calculate_scissor_threshold_area();
 
-	// debug
+	// no need to compile these in in release, they are unneeded outside the editor and only add to executable size
+#ifdef DEBUG_ENABLED
 	void diagnose_batches(Item::Command *const *p_commands);
+	String get_command_type_string(const Item::Command &p_command) const;
+#endif
 
 public:
 	void initialize();
@@ -299,31 +307,14 @@ _FORCE_INLINE_ void RasterizerCanvasGLES2::_prefill_default_batch(FillState &r_f
 #endif
 			// we do have a pending extra transform command to flush
 			// either the extra transform is in the prior command, or not, in which case we need 2 batches
-			//			if (r_fill_state.transform_extra_command_number_p1 == p_command_num) {
-			// this should be most common case
 			r_fill_state.curr_batch->num_commands += 2;
-			//			} else {
-			//				// mad ordering .. does this even happen?
-			//				int extra_command = r_fill_state.transform_extra_command_number_p1 - 1; // plus 1 based
-
-			//				// send the extra to the GPU in a batch
-			//				r_fill_state.curr_batch = _batch_request_new();
-			//				r_fill_state.curr_batch->type = Batch::BT_DEFAULT;
-			//				r_fill_state.curr_batch->first_command = extra_command;
-			//				r_fill_state.curr_batch->num_commands = 1;
-
-			//				// start default batch
-			//				r_fill_state.curr_batch = _batch_request_new();
-			//				r_fill_state.curr_batch->type = Batch::BT_DEFAULT;
-			//				r_fill_state.curr_batch->first_command = p_command_num;
-			//				r_fill_state.curr_batch->num_commands = 1;
-			//			}
 
 			r_fill_state.transform_extra_command_number_p1 = 0; // mark as sent
 			r_fill_state.extra_matrix_sent = true;
 
 			// the original mode should always be hardware transform ..
 			// test this assumption
+			//CRASH_COND(r_fill_state.orig_transform_mode != TM_NONE);
 			r_fill_state.transform_mode = r_fill_state.orig_transform_mode;
 
 			// do we need to restore anything else?