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

RichTextLabel's built-in effects are not exposed as push_() functions #77120

Open
Calinou opened this issue May 16, 2023 · 6 comments · May be fixed by #93412
Open

RichTextLabel's built-in effects are not exposed as push_() functions #77120

Calinou opened this issue May 16, 2023 · 6 comments · May be fixed by #93412

Comments

@Calinou
Copy link
Member

Calinou commented May 16, 2023

Godot version

4.0.2.stable

Issue description

I noticed something while working on #77117. These RichTextLabel methods exist in the C++ source code, but aren't exposed to GDScript unlike other built-in tags like push_bgcolor():

  • push_fade()
  • push_shake()
  • push_wave()
  • push_tornado()
  • push_rainbow()

Having these methods exposed would be useful for consistency with other tags, but also because large text is faster to format using push_*() methods compared to BBCode.

Note that set_process_internal(true) is called in the BBCode handler for those tags (example), not in the push_*() methods (other than push_customfx() which is exposed). This call should be moved to the push_* methods so that it works even if you only use push_*() methods to format your RichTextLabel's text.

cc @Eoin-ONeill-Yokai – is there a reason these methods aren't exposed?

Keywords for easier searching: RichTextEffect

@Eoin-ONeill-Yokai
Copy link
Contributor

Eoin-ONeill-Yokai commented May 16, 2023

It was probably a mistake on my part on the original MR -- I don't think there should be any problem with exposing the push functions to scripting language. And yes, the set_process_internal should be moved to accommodate.

I'll take a look tomorrow.

@akien-mga
Copy link
Member

Isn't push_customfx enough? It's also suitable for script-defined custom fx, and the current list of the builtin ones is pretty arbitrary so I'm not sure they should be exposed as part of the API.

but also because large text is faster to format using push_*() methods compared to BBCode.

How so? Do you mean faster for the programmer, or more performant?

@Calinou
Copy link
Member Author

Calinou commented Feb 26, 2024

push_customfx() works for custom effects, but there is no way to refer to the built-in effects I know of as you can't grab a handle on them from the scripting API.

How so? Do you mean faster for the programmer, or more performant?

It's more performant to use push_* methods, as well as more secure when BBCode is disabled to prevent any risk of formatting injection from untrusted input.

@Sociopathix
Copy link

Hi there, was this ever resolved? I am currently trying to use the built-in custom effects.

@Eoin-ONeill-Yokai
Copy link
Contributor

Sorry, I've been a bit busy on my end but I should be able to address this sometime this week. Let me put it to the top of my Thursday tasks and I'll try to get it done as it shouldn't be too hard to do.

@Sociopathix
Copy link

No worries! I found a workaround but I do think a lot of things could be easier if we had these functions available. :]

Eoin-ONeill-Yokai added a commit to Eoin-ONeill-Yokai/godot that referenced this issue Jun 20, 2024
Should help users who want to use GDScript in conjunction with internal
real time effects such as Tornado, Rainbow and Shake.

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

Successfully merging a pull request may close this issue.

4 participants