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

fix text height calc #11589

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions crates/bevy_text/src/pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,10 @@ impl TextMeasureInfo {
max: Vec2::ZERO,
};

let min = info.compute_size(Vec2::new(0.0, f32::INFINITY));
let max = info.compute_size(Vec2::INFINITY);
info.min = min;
info.max = max;
let min_width = info.compute_size(Vec2::new(0.0, f32::INFINITY));
let max_width = info.compute_size(Vec2::INFINITY);
info.min = min_width.min(max_width);
info.max = min_width.max(max_width);
Comment on lines -180 to +183
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the values of these properties were good before. But the naming was confusing. Calling these max_width_size and min_width_size might be less confusing?

info
}

Expand Down
33 changes: 21 additions & 12 deletions crates/bevy_ui/src/widget/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl Measure for TextMeasure {
width: Option<f32>,
height: Option<f32>,
available_width: AvailableSpace,
_available_height: AvailableSpace,
available_height: AvailableSpace,
) -> Vec2 {
let x = width.unwrap_or_else(|| match available_width {
AvailableSpace::Definite(x) => {
Expand All @@ -64,16 +64,20 @@ impl Measure for TextMeasure {
AvailableSpace::MaxContent => self.info.max.x,
});

height
.map_or_else(
|| match available_width {
AvailableSpace::Definite(_) => self.info.compute_size(Vec2::new(x, f32::MAX)),
AvailableSpace::MinContent => Vec2::new(x, self.info.min.y),
AvailableSpace::MaxContent => Vec2::new(x, self.info.max.y),
},
|y| Vec2::new(x, y),
)
.ceil()
let y = height.unwrap_or_else(|| {
if width.is_some() || matches!(available_width, AvailableSpace::Definite(_)) {
// given a definite width we have a definite height
self.info.compute_size(Vec2::new(x, f32::MAX)).y
} else {
Comment on lines +68 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this case for width.is_some() seems correct to me, and the main change that I do think is necessary in this PR.

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'll check with only this change, thanks for looking it over

match available_height {
AvailableSpace::MinContent => self.info.min.y,
AvailableSpace::MaxContent => self.info.max.y,
AvailableSpace::Definite(_) => self.info.compute_size(Vec2::new(x, f32::MAX)).y,
}
Comment on lines +72 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be changed back to available_width, which would allow the available_width case in the above PR to be removed.

}
});

Vec2::new(x, y).ceil()
}
}

Expand All @@ -88,7 +92,12 @@ fn create_text_measure(
match TextMeasureInfo::from_text(&text, fonts, scale_factor) {
Ok(measure) => {
if text.linebreak_behavior == BreakLineOn::NoWrap {
content_size.set(FixedMeasure { size: measure.max });
// with no wrapping the text will always be a single line, which is as wide as
// possible and as short as possible. so we use the max width and min height
// directly.
content_size.set(FixedMeasure {
size: Vec2::new(measure.max.x, measure.min.y),
robtfm marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't change the value of min and max above then this doesn't need to be changed either.

});
} else {
content_size.set(TextMeasure { info: measure });
}
Expand Down