From 282e4231c26c172b186a5bf22a8ba7f0337ba3d6 Mon Sep 17 00:00:00 2001 From: bruvzg <7645683+bruvzg@users.noreply.github.com> Date: Wed, 12 Apr 2023 11:39:05 +0300 Subject: [PATCH] Improve line BiDi handling, prevent crash on recursive log updates. --- editor/editor_log.cpp | 5 +++ modules/text_server_adv/text_server_adv.cpp | 45 ++++++++++++++++----- modules/text_server_adv/text_server_adv.h | 1 + scene/gui/rich_text_label.cpp | 11 +++++ scene/gui/rich_text_label.h | 2 + 5 files changed, 54 insertions(+), 10 deletions(-) diff --git a/editor/editor_log.cpp b/editor/editor_log.cpp index 8d199ad9898d..1113a10e38fc 100644 --- a/editor/editor_log.cpp +++ b/editor/editor_log.cpp @@ -276,6 +276,11 @@ void EditorLog::_add_log_line(LogMessage &p_message, bool p_replace_previous) { return; } + if (unlikely(log->is_updating())) { + // The new message arrived during log RTL text processing/redraw (invalid BiDi control characters / font error), ignore it to avoid RTL data corruption. + return; + } + // Only add the message to the log if it passes the filters. bool filter_active = type_filter_map[p_message.type]->is_active(); String search_text = search_box->get_text(); diff --git a/modules/text_server_adv/text_server_adv.cpp b/modules/text_server_adv/text_server_adv.cpp index 43c7f49395ed..873367b3ab47 100644 --- a/modules/text_server_adv/text_server_adv.cpp +++ b/modules/text_server_adv/text_server_adv.cpp @@ -4138,6 +4138,7 @@ RID TextServerAdvanced::_shaped_text_substr(const RID &p_shaped, int64_t p_start new_sd->direction = sd->direction; new_sd->custom_punct = sd->custom_punct; new_sd->para_direction = sd->para_direction; + new_sd->base_para_direction = sd->base_para_direction; for (int i = 0; i < TextServer::SPACING_MAX; i++) { new_sd->extra_spacing[i] = sd->extra_spacing[i]; } @@ -4188,9 +4189,33 @@ bool TextServerAdvanced::_shape_substr(ShapedTextDataAdvanced *p_new_sd, const S if (U_SUCCESS(err)) { ubidi_setLine(p_sd->bidi_iter[ov], start, end, bidi_iter, &err); if (U_FAILURE(err)) { - ubidi_close(bidi_iter); - bidi_iter = nullptr; - ERR_PRINT(vformat("BiDi reordering for the line failed: %s", u_errorName(err))); + // Line BiDi failed (string contains incompatible control characters), try full paragraph BiDi instead. + err = U_ZERO_ERROR; + const UChar *data = p_sd->utf16.get_data(); + switch (static_cast(p_sd->bidi_override[ov].z)) { + case DIRECTION_LTR: { + ubidi_setPara(bidi_iter, data + start, end - start, UBIDI_LTR, nullptr, &err); + } break; + case DIRECTION_RTL: { + ubidi_setPara(bidi_iter, data + start, end - start, UBIDI_RTL, nullptr, &err); + } break; + case DIRECTION_INHERITED: { + ubidi_setPara(bidi_iter, data + start, end - start, p_sd->base_para_direction, nullptr, &err); + } break; + case DIRECTION_AUTO: { + UBiDiDirection direction = ubidi_getBaseDirection(data + start, end - start); + if (direction != UBIDI_NEUTRAL) { + ubidi_setPara(bidi_iter, data + start, end - start, direction, nullptr, &err); + } else { + ubidi_setPara(bidi_iter, data + start, end - start, p_sd->base_para_direction, nullptr, &err); + } + } break; + } + if (U_FAILURE(err)) { + ubidi_close(bidi_iter); + bidi_iter = nullptr; + ERR_PRINT(vformat("BiDi reordering for the line failed: %s", u_errorName(err))); + } } } else { bidi_iter = nullptr; @@ -5619,25 +5644,25 @@ bool TextServerAdvanced::_shaped_text_shape(const RID &p_shaped) { sd->script_iter = memnew(ScriptIterator(sd->text, 0, sd->text.length())); } - int base_para_direction = UBIDI_DEFAULT_LTR; + sd->base_para_direction = UBIDI_DEFAULT_LTR; switch (sd->direction) { case DIRECTION_LTR: { sd->para_direction = DIRECTION_LTR; - base_para_direction = UBIDI_LTR; + sd->base_para_direction = UBIDI_LTR; } break; case DIRECTION_RTL: { sd->para_direction = DIRECTION_RTL; - base_para_direction = UBIDI_RTL; + sd->base_para_direction = UBIDI_RTL; } break; case DIRECTION_INHERITED: case DIRECTION_AUTO: { UBiDiDirection direction = ubidi_getBaseDirection(data, sd->utf16.length()); if (direction != UBIDI_NEUTRAL) { sd->para_direction = (direction == UBIDI_RTL) ? DIRECTION_RTL : DIRECTION_LTR; - base_para_direction = direction; + sd->base_para_direction = direction; } else { sd->para_direction = DIRECTION_LTR; - base_para_direction = UBIDI_DEFAULT_LTR; + sd->base_para_direction = UBIDI_DEFAULT_LTR; } } break; } @@ -5666,14 +5691,14 @@ bool TextServerAdvanced::_shaped_text_shape(const RID &p_shaped) { ubidi_setPara(bidi_iter, data + start, end - start, UBIDI_RTL, nullptr, &err); } break; case DIRECTION_INHERITED: { - ubidi_setPara(bidi_iter, data + start, end - start, base_para_direction, nullptr, &err); + ubidi_setPara(bidi_iter, data + start, end - start, sd->base_para_direction, nullptr, &err); } break; case DIRECTION_AUTO: { UBiDiDirection direction = ubidi_getBaseDirection(data + start, end - start); if (direction != UBIDI_NEUTRAL) { ubidi_setPara(bidi_iter, data + start, end - start, direction, nullptr, &err); } else { - ubidi_setPara(bidi_iter, data + start, end - start, base_para_direction, nullptr, &err); + ubidi_setPara(bidi_iter, data + start, end - start, sd->base_para_direction, nullptr, &err); } } break; } diff --git a/modules/text_server_adv/text_server_adv.h b/modules/text_server_adv/text_server_adv.h index af3a0c78769e..4b8c3f7cd381 100644 --- a/modules/text_server_adv/text_server_adv.h +++ b/modules/text_server_adv/text_server_adv.h @@ -476,6 +476,7 @@ class TextServerAdvanced : public TextServerExtension { /* Shaped data */ TextServer::Direction para_direction = DIRECTION_LTR; // Detected text direction. + int base_para_direction = UBIDI_DEFAULT_LTR; bool valid = false; // String is shaped. bool line_breaks_valid = false; // Line and word break flags are populated (and virtual zero width spaces inserted). bool justification_ops_valid = false; // Virtual elongation glyphs are added to the string. diff --git a/scene/gui/rich_text_label.cpp b/scene/gui/rich_text_label.cpp index 1ae24b6d7009..34f576793baa 100644 --- a/scene/gui/rich_text_label.cpp +++ b/scene/gui/rich_text_label.cpp @@ -2705,6 +2705,10 @@ bool RichTextLabel::is_ready() const { return (main->first_invalid_line.load() == (int)main->lines.size() && main->first_resized_line.load() == (int)main->lines.size() && main->first_invalid_font_line.load() == (int)main->lines.size()); } +bool RichTextLabel::is_updating() const { + return updating.load() || validating.load(); +} + void RichTextLabel::set_threaded(bool p_threaded) { if (threaded != p_threaded) { _stop_thread(); @@ -2729,6 +2733,7 @@ bool RichTextLabel::_validate_line_caches() { if (updating.load()) { return false; } + validating.store(true); if (main->first_invalid_line.load() == (int)main->lines.size()) { MutexLock data_lock(data_mutex); Rect2 text_rect = _get_text_rect(); @@ -2747,6 +2752,7 @@ bool RichTextLabel::_validate_line_caches() { if (main->first_resized_line.load() == (int)main->lines.size()) { vscroll->set_value(old_scroll); + validating.store(false); return true; } @@ -2798,8 +2804,10 @@ bool RichTextLabel::_validate_line_caches() { if (fit_content) { update_minimum_size(); } + validating.store(false); return true; } + validating.store(false); stop_thread.store(false); if (threaded) { updating.store(true); @@ -2809,7 +2817,9 @@ bool RichTextLabel::_validate_line_caches() { loading_started = OS::get_singleton()->get_ticks_msec(); return false; } else { + updating.store(true); _process_line_caches(); + updating.store(false); queue_redraw(); return true; } @@ -5895,6 +5905,7 @@ RichTextLabel::RichTextLabel(const String &p_text) { set_text(p_text); updating.store(false); + validating.store(false); stop_thread.store(false); set_clip_contents(true); diff --git a/scene/gui/rich_text_label.h b/scene/gui/rich_text_label.h index fef34f726037..567528652c1b 100644 --- a/scene/gui/rich_text_label.h +++ b/scene/gui/rich_text_label.h @@ -377,6 +377,7 @@ private: bool threaded = false; std::atomic stop_thread; std::atomic updating; + std::atomic validating; std::atomic loaded; uint64_t loading_started = 0; @@ -678,6 +679,7 @@ public: void deselect(); bool is_ready() const; + bool is_updating() const; void set_threaded(bool p_threaded); bool is_threaded() const;