[Merged by Bors] - Implement rotation for Text2d

[nside]

Fixes #2080

[nside]

note that scale_factor could potentially be replaced by transform.scale but I wasn't clear if their semantic is the same

[CleanCut]

note that scale_factor could potentially be replaced by transform.scale but I wasn't clear if their semantic is the same

scale_factor represents the scaling of the screen (old screens are typically 1.0, newer hi-dpi screens are typically 2.0, but other values are possible), so you would want to use the transform's scale on top of that if you're going to handle scaling the text visually.

A decision on the semantics of scaling would need to be made first, though. If the desire is that scaling up the text results in blocky, pixelized (perhaps blurry?) text similar to how the SNES handled scaling, then that could be done right here I think. If the desire is that the font is actually scaled smoothly when text scales, then new glyphs will need to be generated--maybe by scaling the font size itself? Anyway, scaling is probably best left for another discussion / PR.

[CleanCut]

This is an excellent start! Thank you for working on this. 😄

I don't think this should be merged as-is. There are a couple issues with alignment_vertical and alignment_horizontal that I think should be resolved first:

• The names are confusing. These are not alignments, they are offsets. The names lead me to expect variants of HorizontalAlign or VerticalAlign.
• There is unnecessary overhead. The offsets are always passed in separate Vec3s which are already capable of representing offsets in 3 dimensions, but treated as single dimension offsets that are unconditionally added together to form a single offset. In other words, there's only a single offset and we're splitting it into two variables to pass in and recombine.

Off the top of my head, I can think of two good ways to fix this:

Suggestion 1

• Pass in a single offset Vec3, perhaps named offset or translation_offset. You'll notice the entire addition of Vec3::ZERO to the other Vec3 disappears entirely. 🎉

Suggestion 2

• Pass in the entire alignment (text.alignment) to DrawableText::draw() and do the calculations there -- then the callers no longer need to do their own redundant alignment-to-offset calculations for each thing that uses a DrawableText.
  • This will require researching why the UI code is hard-coding alignment to top-aligned, horizontally-centered. If that's what the value of text.alignment already is there, then we can stop hard-coding and pass in text.alignment instead. If that's not what the value of text.alignment is then we should find out why that's hard-coded and either a) if it is intended then preserve the hard-coding by passing in a hard-coded top-aligned, horizontally-centered alignment, or b) if that was not intended, just pass in the actual value text.alignment

---

Having said this, I'm only moderately acquainted with the text rendering code, so there may be even better options.

/cc @AlisCode @blunted2night @cart @julhe @Neo-Zhixing @tigregalis @zicklag - You've all touched code near here. Any opinions? I'd basically love a meta-review of my review for my own education. I may learn more about the rendering design in the process. 😄

[nside]

suggestion 1) sounds fine to me. 2) seems more involving as you need the calculated size to compute the offset

[nside]

updated

[CleanCut]

😄

[zicklag]

@CleanCut I don't have time to do a proper review of your review, but I think your reasoning is sound. 🙂

[CleanCut]

The way the text was rotating seemed a bit off to me, so I dug into it a bit further and it doesn't seem to be rotating correctly. If you remove the position changes from the text2d example, you can see that though the rotation is supposed to be center-aligned both horizontally and vertically, the pivot point for the rotation seems to actually be at the bottom of the text between the "e" and the "x" in "This text...". There must be some bad math somewhere...

With the following code in lines 36-38 in the text2d example it looks like this:

        //transform.translation.x = 100.0 * time.seconds_since_startup().sin() as f32;
        //transform.translation.y = 100.0 * time.seconds_since_startup().cos() as f32;
        transform.rotation = Quat::from_rotation_z(time.seconds_since_startup() as f32);

BadPivot.mp4

[nside]

it's as if the width was off. will check tonight

[nside]

strangely I get different results

[CleanCut]

@nside It's probably the scale_factor. I've got a hi-dpi screen with scale_factor == 2.0.

[nside]

that's it!

[nside]

I was able to remove scale_factor from DrawableText when fixing this

[CleanCut]

I meant you should add handling for scale_factor into the rotation calculations, not attempt to remove scale_factor handling from DrawableText. Now text is differently-sized depending on the DPI of the screen. 😢

[nside]

how do you recommend testing this on non-hi-dpi screens?

[nside]

@CleanCut I brought it back, let me know if that works. Dealing with the glyphs scale on the main transform feels a bit confusing IMO

[CleanCut]

@nside Much better! I spent a couple hours going over this. The remaining problems I see are:

1. The offsets you compute for alignment are backwards--left/right and top/bottom need to be swapped.
2. Requiring the call-site to do the scale_factor computation tightly couples the text2d and bevy_ui::widget::text modules to the implementation of DrawableText::draw.

I put together fixes for both of those and added handling for scaling in #2103 -- please take a look and let me know what path you would like to take forward. in this PR to merge that into this PR here!

[CleanCut]

At this point, I think the PR is done:

• Added rotation handling
• Added scale handling based off of Transform.scale
  • The chosen approach is to scale the entire quad, resulting in pixellated text when you scale it up. This may be a desirable effect for retro 2D games. For "smooth scaling", change the font size instead.
• Updated the text2d example to demonstrate translation, rotation, and scale separately
  • We used a QuerySet to handle all the queries in a single system. Would it be simpler for the user to understand if we used three separate systems instead?

[nside]

very nice

[CleanCut]

very nice

@nside Wouldn't have gotten done without your effort! Thanks for providing a good distraction after my root canal. 🦷

[cart]

I think these Queries should use the Query<&mut Transform, (With<Text>, With<AnimateTranslation>)> style instead. WithBundle isn't a pattern I want to encourage generally (and the behavior of it might change in the future to be "spawned with this specific bundle" instead of "has the components in this bundle"). For now, I want examples to focus on individual components.

[CleanCut]

Ah! I had first (unsuccessfully) tried With<(Text, AnimateTranslation)> -- now I know the correct approach, I can use it!

[cart]

I personally think this should be three separate systems. It is functionally three separate behaviors and separate systems removes the need for the QuerySet.

[cart]

This looks good to me! Just some minor style nits in the example.

[CleanCut]

I'll make the change!

Update: Done in a PR to this PR

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• check-doc
• check-markdown-links
• markdownlint
• run-examples