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.

Sign up for GitHub

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

Calculate the text alignment offset #6724

Closed
wants to merge 22 commits into from
Closed

Calculate the text alignment offset #6724

Changes from 7 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
e6d0947
fixed extract_text_uinodes offset alignment bug
ickshonpe Nov 22, 2022
6775b48
Merge branch 'bevyengine:main' into text-wrap-justify
ickshonpe Nov 22, 2022
bcb16c0
Update crates/bevy_ui/src/render/mod.rs
ickshonpe Nov 22, 2022
a1ec7ac
Update mod.rs
ickshonpe Nov 22, 2022
6b9473a
cargo fmt --all
ickshonpe Nov 22, 2022
feebad6
fix alignment offset being multiplied by scale twice
ickshonpe Nov 22, 2022
28efa7c
cargo fmt --all
ickshonpe Nov 22, 2022
b7d9aa3
removed the comment
ickshonpe Nov 24, 2022
75a2d8c
added text alignment example
ickshonpe Nov 24, 2022
9630c33
add example metadata
ickshonpe Nov 24, 2022
3e1b211
tiny cosmetic improvement
ickshonpe Nov 24, 2022
8d7dfa3
cargo fmt -all
ickshonpe Nov 24, 2022
8334276
fixes and formatting for example
ickshonpe Nov 24, 2022
ec8d739
removed example
ickshonpe Nov 24, 2022
830a5dc
simplified the example
ickshonpe Nov 24, 2022
d6e736b
remove scaling from example
ickshonpe Nov 24, 2022
d111065
Alignment_offset *does* need to multiplied by the scale factor.
ickshonpe Nov 25, 2022
b9fb69c
Multiply by ui_node.size() by the scale factor before calculating offset
ickshonpe Nov 25, 2022
4bd17ef
Merge branch 'calculate-text-alignment' of https://github.com/ickshon…
ickshonpe Nov 25, 2022
0a86a23
forgot to remove "* scale_factor"
ickshonpe Nov 25, 2022
7bb6e52
simplified example
ickshonpe Nov 26, 2022
e7de76b
cargo fmt
ickshonpe Nov 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions crates/bevy_ui/src/render/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use bevy_render::{
Extract, RenderApp, RenderStage,
};
use bevy_sprite::{SpriteAssetEvents, TextureAtlas};
use bevy_text::{Text, TextLayoutInfo};
use bevy_text::{HorizontalAlign, Text, TextLayoutInfo, VerticalAlign};
use bevy_transform::components::GlobalTransform;
use bevy_utils::FloatOrd;
use bevy_utils::HashMap;
Expand Down Expand Up @@ -326,9 +326,22 @@ pub fn extract_text_uinodes(
if uinode.size() == Vec2::ZERO {
continue;
}
let text_glyphs = &text_layout_info.glyphs;
let alignment_offset = (uinode.size() / -2.0).extend(0.0);

let alignment_offset = Vec3::new(
match text.alignment.horizontal {
HorizontalAlign::Left => -0.5 * uinode.size().x,
HorizontalAlign::Center => -0.5 * text_layout_info.size.x,
HorizontalAlign::Right => 0.5 * uinode.size().x - text_layout_info.size.x,
},
match text.alignment.vertical {
VerticalAlign::Center => -0.5 * text_layout_info.size.y,
VerticalAlign::Top => -0.5 * uinode.size().y,
VerticalAlign::Bottom => 0.5 * uinode.size().y - text_layout_info.size.y,
},
0.0,
);

let text_glyphs = &text_layout_info.glyphs;
let mut color = Color::WHITE;
let mut current_section = usize::MAX;
for text_glyph in text_glyphs {
Expand All @@ -348,11 +361,10 @@ pub fn extract_text_uinodes(
let atlas_size = Some(atlas.size);

// NOTE: Should match `bevy_text::text2d::extract_text2d_sprite`
ickshonpe marked this conversation as resolved.
Show resolved Hide resolved

let extracted_transform = global_transform.compute_matrix()
* Mat4::from_scale(Vec3::splat(scale_factor.recip()))
* Mat4::from_translation(
alignment_offset * scale_factor + text_glyph.position.extend(0.),
);
* Mat4::from_translation(alignment_offset + text_glyph.position.extend(0.));
Copy link
Contributor

Choose a reason for hiding this comment

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

is it correct that the scale_factor should no longer be considered here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so. Scale factor is applied all over the place and it's very confusing though. I'll have another look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an example text_alignment that cycles through different TextAlignments and scale factors and it definitely looks correct. I'm worried there are other edge cases but I think that even if there are some problems it's a definite improvement on before.

Copy link
Contributor Author

@ickshonpe ickshonpe Nov 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I understand now, alignment_offset doesn't need to be multiplied by the scale factor, only the node_size does.

I was testing cases with different UiScales and forgetting that scale_factor and UiScale are independent of each other. I bought a new monitor yesterday which made everything completely obvious as now my monitors have different backend_scale_factors and you can see the text layout break when you drag the window from one screen to the other.

With the latest commits, everything should be correct now.


extracted_uinodes.uinodes.push(ExtractedUiNode {
stack_index,
Expand Down