Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add print_rich() for printing with BBCode #60675

Merged
merged 1 commit into from
Jun 28, 2022
Merged

Add print_rich() for printing with BBCode #60675

merged 1 commit into from
Jun 28, 2022

Conversation

voylin
Copy link
Contributor

@voylin voylin commented May 1, 2022

This is an attempt to make pull request #33541 work.

The addition is a 'print_rich()' function which allows the use of BBCode for printing. It prints in both in color, bold, italic, underline in the editor output and in the console through ANSI. (With the help of @Calinou for the conversion strings)

Like you can see in this screenshot, when using print(...) everything gets printed like you've typed it.
But when you use print_rich(...), BBCodes will be used.
Screenshot from 2022-05-02 13-50-53

I was also able to make the folding work that happens with the other types of logs aswell.

Sorry in advance if the way I made this work isn't very clean, I hope you guys can give me advice on how to improve this code. I tried my best and this is my first time actually working on the Godot engine. But it works. 😄

@voylin voylin requested review from a team as code owners May 1, 2022 02:43
@voylin voylin changed the title Adding print_rich for printing with BBCode Adding print_rich() for printing with BBCode May 1, 2022
@Chaosus Chaosus added this to the 4.0 milestone May 1, 2022
@voylin
Copy link
Contributor Author

voylin commented May 1, 2022

I think I fixed all the workflow issues (ran the scripts on my pc and it worked), and re-based on the master branch.

core/string/print_string.cpp Outdated Show resolved Hide resolved
@voylin voylin requested a review from a team as a code owner May 2, 2022 05:02
@voylin
Copy link
Contributor Author

voylin commented May 2, 2022

I squashed all my commits, re-based them on master, did a check with clang_format.sh and also fixed an issue with one of the Linux workflow builds (made a mistake in the mono stuff, the function name wasn't correct). So normally all workflow checks should be good now.

@voylin voylin requested a review from a team as a code owner May 2, 2022 08:59
@voylin
Copy link
Contributor Author

voylin commented May 2, 2022

Fixed the workflow issue which was happening. Sorry about that. ^^"

core/debugger/remote_debugger.h Outdated Show resolved Hide resolved
editor/editor_log.h Outdated Show resolved Hide resolved
@Merlin1846
Copy link

I don't think this would work with all terminals, which makes this not usable in the final export, right?

@voylin
Copy link
Contributor Author

voylin commented May 2, 2022

I don't think this would work with all terminals, which makes this not usable in the final export, right?

It uses ANSI so it should be supported by nearly all terminals I think, but I could be wrong of course. For which terminals would this not work?

@Calinou
Copy link
Member

Calinou commented May 2, 2022

I don't think this would work with all terminals, which makes this not usable in the final export, right?

This should work well in exported projects, unless you're targeting old Windows versions.

  • Support on Linux and macOS is pretty good for basic features (foreground/background colors and bold). Support for complex features like italic and strikethrough vary depending on the user's terminal emulator.
  • Support on Windows 10 and later requires Enable ANSI escape code processing on Windows 10 and later #44118 to be finalized and merged.
  • Windows 8.1 and prior don't have a way to support ANSI escape codes. To prevent the console output from looking garbled there, there should be a routine to replace BBCode with empty strings, which should be used when Windows 8.1 or prior is detected. This doesn't have to be done now; this can be implemented in a future pull request.
    • This stripping routine will also be useful when printing to the console on Android, iOS and HTML51. To avoid inserting ANSI escape codes in redirected output, it should also be used when non-interactive output is detected (isatty() on Linux/macOS).
    • The stripping routine could also be forcibly opted into using the NO_COLOR=1 environment variable.

Footnotes

  1. Supporting rich printing is also feasible in HTML5 using HTML in the printed text, but it'll require more work.

@miv391
Copy link
Contributor

miv391 commented May 2, 2022

There are 44 p_string_ansi.replace calls so I started thinking whether all the bbcode replaces could be done with one pass. I don't how much faster one pass implementation could be and I don't know if it even matters for the intended usage and would it be worth of much more complicated code.

@Calinou
Copy link
Member

Calinou commented May 2, 2022

There are 44 p_string_ansi.replace calls so I started thinking whether all the bbcode replaces could be done with one pass. I don't how much faster one pass implementation could be and I don't know if it even matters for the intended usage and would it be worth of much more complicated code.

I'd say the set of replace() calls is fast enough right now (see my screenshot in #33541 (comment)). This was a pure GDScript version, and the C++ version should be slightly faster by avoiding interactions with the scripting language (even though the string replacement logic was C++ in both cases).

If making it faster is desired, we can check for the presence of a tag before running its relevant replacements. For instance, if there is no [fgcolor within the string, there's no point in running all the .replace() calls for [fgcolor tags. The same should apply to other tags, as we're always performing at least two string replacements for them – but we only need to check for the opening tag's presence once.

The above optimization will be more useful if we decide to support all named colors. I've only added support for 12 named colors or so right now, but we have more than 200 in the engine.

@voylin
Copy link
Contributor Author

voylin commented May 2, 2022

There are 44 p_string_ansi.replace calls so I started thinking whether all the bbcode replaces could be done with one pass. I don't how much faster one pass implementation could be and I don't know if it even matters for the intended usage and would it be worth of much more complicated code.

Like Calinou suggested, I've added the 2 biggest pieces ([color=] and [bgcolor=]) inside of an if statement to see if the string contains any color or bgcolor tags. Hope this helps with the speed concern.

modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs Outdated Show resolved Hide resolved
doc/classes/@GlobalScope.xml Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/GD.cs Outdated Show resolved Hide resolved
@voylin
Copy link
Contributor Author

voylin commented May 3, 2022

The static checks failed because of the color tags I used in the example for the docs, so I changed the example. There was also a small mistake in the GlobalScope documentation which I have changed.

Calinou
Calinou previously requested changes May 3, 2022
core/string/print_string.cpp Outdated Show resolved Hide resolved
core/string/print_string.cpp Show resolved Hide resolved
core/string/print_string.cpp Outdated Show resolved Hide resolved
doc/classes/@GlobalScope.xml Outdated Show resolved Hide resolved
editor/editor_node.cpp Outdated Show resolved Hide resolved
core/debugger/remote_debugger.cpp Outdated Show resolved Hide resolved
@voylin voylin requested a review from Calinou May 4, 2022 00:50
editor/editor_log.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good to merge (preferably after #44118), although I have a concern about the editor UX related to filtering messages – see above.

@voylin
Copy link
Contributor Author

voylin commented May 4, 2022

Should be good to merge (preferably after #44118), although I have a concern about the editor UX related to filtering messages – see above.

Fixed the filtering message thing. The normal print messages and print_rich messages are being filtered by the same button now.

@akien-mga akien-mga removed the request for review from a team May 25, 2022 13:29
@akien-mga akien-mga requested a review from Calinou June 28, 2022 12:55
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved in PR review meeting, the code seems good and the feature is nice to have.
Would be worth a rebase to make sure it still builds and works fine before merging.

@voylin
Copy link
Contributor Author

voylin commented Jun 28, 2022

@akien-mga Rebased and checked, still working. Thanks for approving this! ^^/

@akien-mga akien-mga dismissed Calinou’s stale review June 28, 2022 16:00

Changes done.

@akien-mga akien-mga merged commit b730d2e into godotengine:master Jun 28, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@voylin voylin deleted the Add-BBCode-support-for-printing-output branch June 30, 2022 22:29
@ghost
Copy link

ghost commented Jul 3, 2022

Best feature ever!

  • Done

@ggppjj
Copy link

ggppjj commented Jul 6, 2024

Hello!

I believe nested color tags work as I would expect, but they don't seem to filter from the text logs correctly. [color=green][color=silver]*[/color].[/color] renders to a debug console as I would expect, with the * being silver and the . being green, but the [color=silver] tags are being written to the godot.log, where the top-level tags aren't.

Edit: To clarify, the nested opening tag is not filtered from the text logs but its corresponding closing tag is.

@Calinou
Copy link
Member

Calinou commented Jul 6, 2024

but the [color=silver] tags are being written to the godot.log, where the top-level tags aren't.

Nested color tags work in the editor Output panel (it's displayed using RichTextLabel), but terminal printing does not handle nested color tags as it uses a separate BBCode implementation.

Also, terminal printing does not handle color tags other than a few basic colors (silver is not supported). See print_rich()'s description in the class reference. This would need #87899 to be implemented to be resolved.

@ggppjj
Copy link

ggppjj commented Jul 7, 2024

Nested color tags work in the editor Output panel (it's displayed using RichTextLabel), but terminal printing does not handle nested color tags as it uses a separate BBCode implementation.

To clarify, I mean on-disk plaintext file log output generated by debug builds, if that isn't the same thing in this context as terminal printing.

Understood about the color support, though. Thanks for the tip there!

@Calinou
Copy link
Member

Calinou commented Jul 8, 2024

To clarify, I mean on-disk plaintext file log output generated by debug builds, if that isn't the same thing in this context as terminal printing.

That's a consequence of terminal printing (stdout). What you see in the log file is directly based on stdout with ANSI escape codes stripped since 4.3. Since terminal printing never replaced the [color=silver] tag in the first place, it's written as-is to the log file.

@AThousandShips AThousandShips changed the title Adding print_rich() for printing with BBCode Add print_rich() for printing with BBCode Jul 8, 2024
@ggppjj
Copy link

ggppjj commented Jul 8, 2024

Understood, thanks. I figured that's what the answer was once you clarified that silver is only accidentally working. I'll stick to supported codes for the time being.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants