Skip to content

Make it possible to change character transform in RichTextEffect - #77819

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
botero-dev:char-fx-tests
Sep 26, 2023
Merged

Make it possible to change character transform in RichTextEffect#77819
akien-mga merged 1 commit into
godotengine:masterfrom
botero-dev:char-fx-tests

Conversation

@botero-dev

@botero-dev botero-dev commented Jun 3, 2023

Copy link
Copy Markdown
Contributor

This PR adds a Transform2D field to the CharFXTransform object, this lets a subclass of RichTextEffect to specify custom transforms including rotations and scaling. Partially implements godotengine/godot-proposals#3417

charfxtransform.demo.mp4

Additionally, I moved an include so rebuilds are faster.

@djrain

djrain commented Jun 4, 2023

Copy link
Copy Markdown

Just found myself wanting this, and came looking - nice timing! Can it be set up to scale from the center of the glyph by default? I imagine that would be desired in most cases.

@botero-dev
botero-dev force-pushed the char-fx-tests branch 3 times, most recently from f7ec3e7 to 2ed1ea5 Compare June 13, 2023 05:20
@botero-dev

Copy link
Copy Markdown
Contributor Author

I did some cleanup and ran some experiments.
The transform origin is still the baseline left. It makes the most sense to keep it there because then it is easy to apply offsets before and after the transform to shift the transform origin.

The two problems I found:

  • Font size is not sent to the user in the CharFXTransform data structure. Font size is needed to get some glyph metrics.
  • Shadows are transformed using the glyph origin instead of the shadow origin. for outlines it makes sense but for shadows it feels weird.

Also, the text server, and some font-wide metrics like ascent and descent could be provided by CharFXTransform, as it is very likely that these will be asked for from scripting anyways.

Those will get tackled in my next pass. For now enjoy one of my tests:

charfxtransform.demo2.mp4

@botero-dev

botero-dev commented Jun 17, 2023

Copy link
Copy Markdown
Contributor Author

Had some more fun today:
image
To achieve this, I ended up setting the transform from whatever location the layout engine tells me it would place the character, and letting the user transform this (instead of my previous attempt of using an identity transform and adding the offsets myself right before the drawing.

Effects can be stacked: (the lag at the beggining seems to be screen recording program)

44a4d9e5f939ef269f18b3bf1666f42e.mp4

You can try out these by compiling and running this demo project:
TextTransformEffects.zip
The curves need to be reassigned when launching the project, I guess it is some Godot serialization issue out of the scope of this issue.
Additionally I found an issue with Curve2D sampling but I'll report that one separately in a minute.

I am in a point where the feature would be good enough for my usage, but I feel like some feedback on this would be appreciated. some points to discuss:

  • I would like shadows to be transformed using their own pivot instead of the main character pivot, so when I rotate a glyph 180 degrees, the shadow would still be shifted down. This could be done in two ways:

    • pushing the shadow transform separately with its own draw_set_transform_matrix call
    • somehow relying on the user applying this offset himself (and forwarding the shadow offset data to the user through the CharFXTransform)
  • When calling font_draw_glyph_outline It asks for offset coordinates, I assume that internally it would try to do sub-pixel antialiasing if the inputs are right... I am changing those values and actually making those values after the user may have done many transform operations. Is there anything I should do to play well with the system?

I still haven't exposed font_size to the CharFXTransform object, so that will be part of my next push.

Comment thread scene/gui/rich_text_label.h Outdated
Comment on lines 356 to 357

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Forward declarations should be created at the top of the file. (although it's interesting that you can inline them 🤔)

Comment thread scene/gui/rich_text_label.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This shouldn't be in the code. You could write a comment in the PR instead.

@KoBeWi KoBeWi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tested that it works, but I can't say if the implementation is 100% correct (looks fine to me tho). I mainly wonder if there is some negative performance impact if someone doesn't modify the transform, because the char is transformed regardless of that.

Comment thread scene/gui/rich_text_effect.h Outdated
Comment thread scene/gui/rich_text_effect.h Outdated
Comment thread scene/gui/rich_text_effect.cpp Outdated
Comment thread scene/gui/rich_text_label.cpp Outdated
Comment thread scene/gui/rich_text_label.cpp Outdated
Comment thread scene/gui/rich_text_label.cpp Outdated
Comment thread scene/gui/rich_text_label.cpp Outdated
Comment thread scene/gui/rich_text_label.cpp Outdated
Comment thread scene/gui/rich_text_label.cpp Outdated
@IntangibleMatter

Copy link
Copy Markdown
Contributor

I've been thinking about something like this lately too! It would make text effects wayyyyy more versatile, and it would make godotengine/godot-proposals#7270 even more powerful!

@akien-mga

Copy link
Copy Markdown
Member

@0xafbf Are you able to address the review comments?

The PR should also be rebased, and the commits squashed into one.

@botero-dev

Copy link
Copy Markdown
Contributor Author

yes, I'll update this pr by the end of the week

@botero-dev
botero-dev force-pushed the char-fx-tests branch 2 times, most recently from 502bd89 to 34671c7 Compare August 23, 2023 04:56
Use absolute transforms for CharFX

fix formatting
@botero-dev

Copy link
Copy Markdown
Contributor Author

Hello, I updated the PR with some improvements:

  • Cleaned up most of the suggestions
  • Added the paragraph offset to the character offset so effects can deal better with multi line texts
  • Added some code that fixes shadows (so 180º rotated glyphs don't draw the shadow in the opposite direction)

I feel the same concern on pushing transform matrices per character. I don't know how to profile that very well, but I guess that adding a hundred of RichTextLabel should do the trick. I'll do that in my next pass

For optimization, I have conflicting ideas on what approach to take:

  • Have a boolean that only marks true if an ItemFX effect is found, that way we can avoid other checks elsewhere
  • Have a boolean set in the RichTextEffect script similar to how the BBCode name works, that we can read internally to enable transform matrices
  • Check the matrix that is about to be pushed vs the last pushed, and avoid doing it if it is the same, the downside of this is that matrix math needs to be done either way

In the end it may not be the worst thing to push one matrix per character, because in the end we can filter it so only happens when ItemFX are used (so for example documentation viewer wouldn't push matrices per character), and the rest of the strings that would use this would very likely not exceed a hundred of characters. I think we'll know better when I do the tests

Also, Drawing shadows uses a different transform. On one hand I think that we can do some inverse transforms to find the location to layout the shadow so it is placed correctly when it is drawn. But that feels hacky and maybe not very valuable for the reason mentioned above. On the other hand I think that it would be cool to extend the RichTextEffect system to tell the user if the character being processed is a shadow, an outline, or the character itself, and let the user apply color and transform effects individually for each of these passes. That would be done out of the scope of this PR, and maybe require some changes on how the text render passes are issued.

I include an updated version of the example project with some implemented effects to aid in testing:

TextTransformEffects.zip

@akien-mga
akien-mga requested a review from bruvzg August 28, 2023 09:43
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Sep 26, 2023
@akien-mga
akien-mga merged commit 21cdedb into godotengine:master Sep 26, 2023
@akien-mga

Copy link
Copy Markdown
Member

Thanks!

@feserr

feserr commented Oct 5, 2023

Copy link
Copy Markdown

In which version it will be added? 4.2?

@akien-mga

Copy link
Copy Markdown
Member

Yes, refer to the milestone in the side bar:
image

@botero-dev
botero-dev deleted the char-fx-tests branch October 25, 2024 14:03
BendyLand pushed a commit to BendyLand/voltaire that referenced this pull request Aug 2, 2026
Make it possible to change character transform in RichTextEffect
BendyLand pushed a commit to BendyLand/voltaire that referenced this pull request Aug 2, 2026
Make it possible to change character transform in RichTextEffect
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.

9 participants