godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.46k stars 21.27k forks source link

Using print_rich on a text containing a newline wrapped in the [color] BBcode tag doesn't strip away the closing tag. #97965

Open Shiva-Shadowsong opened 1 month ago

Shiva-Shadowsong commented 1 month ago

Tested versions

System information

Godot v4.3.stable (bce457cec) - Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1660 SUPER (NVIDIA; 31.0.15.2802) - AMD Ryzen 5 5600X 6-Core Processor (12 Threads)

Issue description

When using print_rich to print a string that contains a [color] BBcode tag - if the string wrapped in that tag contains a newline ("\n"), the closing [/color] tag won't be stripped away, resulting in an output where the color works, but the "[/color]" tag remains as part of the printed string.

Steps to reproduce

  1. Create a string that has a "\n" in it.
  2. Wrap it in a color tag like [color=yellow].
  3. Output it with print_rich.

Minimal reproduction project (MRP)

I guess there's no need to upload a whole MRP, this can be tested with 1 line of code, try it out:

func _ready() -> void:
    print_rich("[color=yellow]Test\ncolor tag with newline.[/color]")

image

dustdfg commented 1 month ago

Also reproducible in Godot 4.0.4

I am going to try to fix it

dustdfg commented 1 month ago

Funny fact. It doesn't strip BBcode if it is printed into Godot Console but it is stripped when it is printed into OS console screen-1728545875

dustdfg commented 1 month ago

It is not only not stripped away but also isn't considered as fallback to white text which affects following text...

screen-1728546548

The os console still shows correct results: screen-1728546564

Calinou commented 1 month ago

cc @bruvzg

print_rich() always resets BBCode tags on every print, so you don't need to add a closing [/color] tag. This might be enough to avoid the issue, although we should still see if we can avoid it in the first place (as the auto-close behavior isn't obvious).

Funny fact. It doesn't strip BBcode if it is printed into Godot Console but it is stripped when it is printed into OS console

The editor Output panel uses the BBCode implementation from RichTextLabel to display it, while terminal output uses a custom BBCode-to-ANSI conversion routine that only supports a subset of BBCode and behaves differently. See https://github.com/godotengine/godot/issues/87899.

The os console still shows correct results:

I wouldn't say it's correct behavior, it's just that the BBCode-to-ANSI conversion routine always replaces [/color] with a "reset color" tag, even if [color] was never opened.

elchukc commented 1 month ago

print_rich() always resets BBCode tags on every print, so you don't need to add a closing [/color] tag. This might be enough to avoid the issue, although we should still see if we can avoid it in the first place (as the auto-close behavior isn't obvious). ... terminal output uses a custom BBCode-to-ANSI conversion routine that only supports a subset of BBCode and behaves differently.

Likely not intended behaviour, the code inside that custom parser explicitly tries to replace [/color].

if (p_string_ansi.contains("[color")) {
    p_string_ansi = p_string_ansi.replace("[color=black]", "\u001b[30m");
    ...
    p_string_ansi = p_string_ansi.replace("[/color]", "\u001b[39m");    // On this line
}
dustdfg commented 1 month ago

print_rich() always resets BBCode tags on every print, so you don't need to add a closing [/color] tag. This might be enough to avoid the issue, although we should still see if we can avoid it in the first place (as the auto-close behavior isn't obvious).

While auto-close behavior is "correct" for single color print, it isn't correct in multi color print like in my example because user will expect to see white printed in white color.

dustdfg commented 1 month ago

Likely not intended behaviour, the code inside that custom parser explicitly tries to replace [/color].

if (p_string_ansi.contains("[color")) {
  p_string_ansi = p_string_ansi.replace("[color=black]", "\u001b[30m");
  ...
  p_string_ansi = p_string_ansi.replace("[/color]", "\u001b[39m");    // On this line
}

Looks like it is intended. As I understand the line you pointed resets color to default when sees closing tag so it is likely normal because if color is already default nothing changes

EDIT: Or you meant that it should print closing tag as is if there wasn't opening tag? If yes, I think it is too much effort to conform to the specs for such a small inconsistence

elchukc commented 1 month ago

EDIT: Or you meant that it should print closing tag as is if there wasn't opening tag? If yes, I think it is too much effort to conform to the specs for such a small inconsistence

I just meant that it looks like it's trying to replace the closing tag, but somehow the closing tag is still showing in the output.

dustdfg commented 1 month ago

EDIT: Or you meant that it should print closing tag as is if there wasn't opening tag? If yes, I think it is too much effort to conform to the specs for such a small inconsistence

I just meant that it looks like it's trying to replace the closing tag, but somehow the closing tag is still showing in the output.

The code that prints to os terminal is different from code that prints to godot console. And here you are viewing the code that prints to os terminal. If you will comment the line of code you mentioned, your os terminal will print the same result as in godot console. "\u001b[39m" is an escape code for terminals