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 a [pulse] built-in effect to RichTextLabel #77117

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented May 15, 2023

In games, blinking text is one of the more frequently used animations. It can be (sparingly) used to bring attention to important messages in a chat log or inventory tooltip, for instance.

This effect accepts the following options:

  • freq: How fast text blinks (higher is faster).
  • color: The target color multiplier for blinking. The default mostly fades out text, but not entirely (for better accessibility).
  • ease: The easing function exponent to use. Negative values provide in-out easing, which is why -2.0 is the default.

Testing project: test_richtexteffect_pulse.zip
Note: Opening the project will print this error about 100 times, but this occurs when using unmodified master branch too:

ERROR: Condition "p_ptr == nullptr" is true.
   at: free_static (core/os/memory.cpp:148)

Preview

C++-based version (this PR) on the left, script-based version on the right (included in the testing project):

test_richtexteffect_pulse.mp4

@mhilbrunner
Copy link
Member

mhilbrunner commented May 21, 2023

Seems useful! Thoughts:

  1. Would it be possible to add a parameter for total number of pulses before it stops? With the default/-1 being infinite, but its pretty common to let an important keyword pulse once (or 2-3 times) only to avoid it being distracting. Only if this won't add a ton of complexity though.
  2. At first glance, it feels a bit weird for the bobbing effect to be a pulse effect parameter instead of its own thing, but maybe thats just me.

@Riteo
Copy link
Contributor

Riteo commented May 21, 2023

@mhilbrunner

At first glance, it feels a bit weird for the bobbing effect to be a pulse effect parameter instead of its own thing, but maybe thats just me.

Yeah I agree with your thoughts and especially this one.

@Calinou
Copy link
Member Author

Calinou commented May 21, 2023

  1. Would it be possible to add a parameter for total number of pulses before it stops? With the default/-1 being infinite, but its pretty common to let an important keyword pulse once (or 2-3 times) only to avoid it being distracting. Only if this won't add a ton of complexity though.

To do this, I need to check if it's possible to set a state variable when the RichTextLabel is last modified.

  1. At first glance, it feels a bit weird for the bobbing effect to be a pulse effect parameter instead of its own thing, but maybe thats just me.

I'm not sure if a dedicated tag for bobbing is warranted, but I may as well remove the bobbing functionality entirely as I don't think it'll be used often.

@Calinou
Copy link
Member Author

Calinou commented May 23, 2023

I've pushed a commit that removes the height option from the [pulse] effect, in case we decide to go with that route.

@mhilbrunner
Copy link
Member

To do this, I need to check if it's possible to set a state variable when the RichTextLabel is last modified.

Don't spent too much time on it, can also be followup work. Would be useful though.

I've pushed a commit that removes the height option from the [pulse] effect, in case we decide to go with that route.

Makes sense IMO

@Calinou Calinou force-pushed the richtextlabel-add-pulse-effect branch from f511811 to aa68fb2 Compare May 31, 2023 11:05
@KoBeWi
Copy link
Member

KoBeWi commented Jun 1, 2023

Would it be possible to add a parameter for total number of pulses before it stops?

This wouldn't work well with text gradually appearing with visible characters.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The implementation looks ok and it works correctly.

@akien-mga
Copy link
Member

CC @Eoin-ONeill-Yokai, you might be interested ;)

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Jun 2, 2023
@akien-mga
Copy link
Member

I think we're close to consensus so I'm putting this on the 4.2 milestone (too late for 4.1 as we entered feature freeze).

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

Feature looks good to me, same for the code. Will need to be documented.

Copy link
Contributor

@Eoin-ONeill-Yokai Eoin-ONeill-Yokai left a comment

Choose a reason for hiding this comment

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

This looks great! I don't see any problems with the code.

I'm happy to see this integrated directly in the engine if others are; I admittedly only restricted it to a custom RichTextEffect implementation only as a proof of concept to make sure that the API was good enough as I had originally implemented it into the C++ directly. There's a discussion to be had on whether or not the custom RTEs could be less of a burden for users to use, but I don't think a decision should be made there until we really flesh out our asset library situation (as discussed in the sprint right now ;) )

Anyway, looks good and works as expected.

@djrain
Copy link

djrain commented Jun 4, 2023

The "ghost" effect in the docs is very nice and quite similar - seems to me that its "span" variable could fit in nicely here, and make it a lot more flexible. Can we combine them?

@Calinou
Copy link
Member Author

Calinou commented Jun 5, 2023

The "ghost" effect in the docs is very nice and quite similar - seems to me that its "span" variable could fit in nicely here, and make it a lot more flexible. Can we combine them?

This doesn't seem to be something commonly used in games. We have a custom effect system so you can still use it 🙂

Either way, I've squashed commits in this PR and rebased it against master (no other changes were made).

In games, blinking text is one of the more frequently used animations.
It can be (sparingly) used to bring attention to important messages
in a chat log or inventory tooltip, for instance.

This effect accepts the following options:

- `freq`: How fast text blinks (higher is faster).
- `color`: The target color multiplier for blinking.
  The default mostly fades out text, but not entirely (for better accessibility).
- `ease`: The easing function exponent to use.
  Negative values provide in-out easing, which is why `-2.0` is the default.
@Calinou Calinou force-pushed the richtextlabel-add-pulse-effect branch from aa68fb2 to 70e6c3c Compare June 5, 2023 16:45
@YuriSizov YuriSizov merged commit 7550b02 into godotengine:master Jul 12, 2023
@YuriSizov
Copy link
Contributor

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.

Add a [pulse] built-in effect to RichTextLabel
8 participants