From 54cd6d3077faa76616e86cc50f754d4b001cf258 Mon Sep 17 00:00:00 2001 From: lawnjelly Date: Mon, 20 Apr 2020 19:32:10 +0100 Subject: [PATCH] Fix batch translate to colored synchronization error In rare circumstances default batches were being joined incorrectly, causing visual regressions. This logic has been fixed. In addition slightly more output information has been added to frame diagnosis mode. --- drivers/gles2/rasterizer_canvas_gles2.cpp | 159 ++++++++++++++++++---- drivers/gles2/rasterizer_canvas_gles2.h | 29 ++-- 2 files changed, 141 insertions(+), 47 deletions(-) diff --git a/drivers/gles2/rasterizer_canvas_gles2.cpp b/drivers/gles2/rasterizer_canvas_gles2.cpp index 52e277c0fc3..22e6faecb57 100644 --- a/drivers/gles2/rasterizer_canvas_gles2.cpp +++ b/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,37 +507,44 @@ 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->batch_texture_id == source_batch.batch_texture_id) && (dest_batch->type == source_batch.type)) { - // add to previous batch - dest_batch->num_commands += source_batch.num_commands; - needs_new_batch = false; + if (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); + // create the colored verts (only if not default) + int first_vert = source_batch.first_quad * 4; + int end_vert = 4 * (source_batch.first_quad + source_batch.num_commands); - for (int v = first_vert; v < end_vert; v++) { - const BatchVertex &bv = bdata.vertices[v]; - BatchVertexColored *cv = bdata.vertices_colored.request(); + for (int v = first_vert; v < end_vert; v++) { + const BatchVertex &bv = bdata.vertices[v]; + BatchVertexColored *cv = bdata.vertices_colored.request(); #ifdef DEBUG_ENABLED - CRASH_COND(!cv); + CRASH_COND(!cv); #endif - cv->pos = bv.pos; - cv->uv = bv.uv; - cv->col = source_batch.color; - } + cv->pos = bv.pos; + cv->uv = bv.uv; + cv->col = source_batch.color; + } + } // textures match + } else { + // default + // we can still join, but only under special circumstances + // does this ever happen? not sure at this stage, but left for future expansion + 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 } - } else { - needs_new_batch = true; - } - } else { - needs_new_batch = true; - } + } // 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(&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"; - } else { - bdata.frame_string += "\n"; + bdata.frame_string += " MULTI"; } + 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); } diff --git a/drivers/gles2/rasterizer_canvas_gles2.h b/drivers/gles2/rasterizer_canvas_gles2.h index 5d11d502907..7d23f0d54be 100644 --- a/drivers/gles2/rasterizer_canvas_gles2.h +++ b/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?