-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Allow constraining Text in percent of window & container size #5502
Changes from 7 commits
c538c84
e068a2b
712d914
3bac609
a7aaf2d
9fbd8f3
2451592
65da63b
39fee5a
fda9ca8
8fc1a62
652b666
bd72e6b
e30e1f9
c859f61
8bf0bbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,13 @@ use bevy_ecs::{ | |
query::{Changed, Or, With}, | ||
system::{Local, ParamSet, Query, Res, ResMut}, | ||
}; | ||
|
||
use bevy_hierarchy::Parent; | ||
use bevy_math::Vec2; | ||
use bevy_render::texture::Image; | ||
use bevy_sprite::TextureAtlas; | ||
use bevy_text::{DefaultTextPipeline, Font, FontAtlasSet, Text, TextError}; | ||
use bevy_window::{WindowId, Windows}; | ||
use bevy_window::Windows; | ||
|
||
#[derive(Debug, Default)] | ||
pub struct QueuedText { | ||
|
@@ -22,14 +24,26 @@ fn scale_value(value: f32, factor: f64) -> f32 { | |
|
||
/// Defines how `min_size`, `size`, and `max_size` affects the bounds of a text | ||
/// block. | ||
pub fn text_constraint(min_size: Val, size: Val, max_size: Val, scale_factor: f64) -> f32 { | ||
pub fn text_constraint( | ||
min_size: Val, | ||
size: Val, | ||
max_size: Val, | ||
scale_factor: f64, | ||
window_size: f32, | ||
) -> f32 { | ||
// Needs support for percentages | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment can probably be removed since you're implementing the support for percentages here. |
||
match (min_size, size, max_size) { | ||
(_, _, Val::Px(max)) => scale_value(max, scale_factor), | ||
(_, _, Val::Percent(max)) => scale_value((max / 100.) * window_size, scale_factor), | ||
(Val::Px(min), _, _) => scale_value(min, scale_factor), | ||
(Val::Percent(min), _, _) => scale_value((min / 100.) * window_size, scale_factor), | ||
(Val::Undefined, Val::Px(size), Val::Undefined) | (Val::Auto, Val::Px(size), Val::Auto) => { | ||
scale_value(size, scale_factor) | ||
} | ||
(Val::Undefined, Val::Percent(size), Val::Undefined) | ||
| (Val::Auto, Val::Percent(size), Val::Auto) => { | ||
scale_value((size / 100.) * window_size, scale_factor) | ||
} | ||
_ => f32::MAX, | ||
} | ||
} | ||
|
@@ -39,7 +53,7 @@ pub fn text_constraint(min_size: Val, size: Val, max_size: Val, scale_factor: f6 | |
#[allow(clippy::too_many_arguments)] | ||
pub fn text_system( | ||
mut queued_text: Local<QueuedText>, | ||
mut last_scale_factor: Local<f64>, | ||
mut last_window_details: Local<(f64, f32, f32)>, | ||
mut textures: ResMut<Assets<Image>>, | ||
fonts: Res<Assets<Font>>, | ||
windows: Res<Windows>, | ||
|
@@ -49,15 +63,25 @@ pub fn text_system( | |
mut text_queries: ParamSet<( | ||
Query<Entity, Or<(Changed<Text>, Changed<Style>)>>, | ||
Query<Entity, (With<Text>, With<Style>)>, | ||
Query<(&Text, &Style, &mut CalculatedSize)>, | ||
Query<(&Text, &Style, &mut CalculatedSize, Option<&Parent>)>, | ||
)>, | ||
style_query: Query<&Style>, | ||
) { | ||
let scale_factor = windows.scale_factor(WindowId::primary()); | ||
// FIXME: this will be problematic for multi-window UI's since its only checking the primary display | ||
let (scale_factor, window_width_constraint, window_height_constraint) = | ||
if let Some(window) = windows.get_primary() { | ||
louiidev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(window.scale_factor(), window.width(), window.height()) | ||
} else { | ||
(1., f32::MAX, f32::MAX) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this case seems like it shouldn't happen as it can't give correct values. maybe ignore / panic instead of running values without meaning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I agree, I'll fix this when I get a chance. |
||
}; | ||
|
||
let inv_scale_factor = 1. / scale_factor; | ||
|
||
#[allow(clippy::float_cmp)] | ||
if *last_scale_factor == scale_factor { | ||
if last_window_details.0 == scale_factor | ||
&& last_window_details.1 == window_width_constraint | ||
&& last_window_details.2 == window_height_constraint | ||
{ | ||
// Adds all entities where the text or the style has changed to the local queue | ||
for entity in text_queries.p0().iter() { | ||
queued_text.entities.push(entity); | ||
|
@@ -67,7 +91,9 @@ pub fn text_system( | |
for entity in text_queries.p1().iter() { | ||
queued_text.entities.push(entity); | ||
} | ||
*last_scale_factor = scale_factor; | ||
last_window_details.0 = scale_factor; | ||
last_window_details.1 = window_width_constraint; | ||
last_window_details.2 = window_height_constraint; | ||
} | ||
|
||
if queued_text.entities.is_empty() { | ||
|
@@ -78,19 +104,34 @@ pub fn text_system( | |
let mut new_queue = Vec::new(); | ||
let mut query = text_queries.p2(); | ||
for entity in queued_text.entities.drain(..) { | ||
if let Ok((text, style, mut calculated_size)) = query.get_mut(entity) { | ||
if let Ok((text, style, mut calculated_size, parent)) = query.get_mut(entity) { | ||
let mut scale_factor_width = scale_factor; | ||
let mut scale_factor_height = scale_factor; | ||
if let Some(parent) = parent { | ||
if let Ok(style) = style_query.get(parent.get()) { | ||
if let Val::Percent(percentage) = style.size.width { | ||
scale_factor_width = percentage as f64 / (scale_factor * 100.); | ||
} | ||
if let Val::Percent(percentage) = style.size.height { | ||
scale_factor_height = percentage as f64 / (scale_factor * 100.); | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels very strange to me that you're changing the scale factor instead of the size constraint There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I think that was the wrong approach by me, I'll update it to target the style.size instead |
||
|
||
let node_size = Vec2::new( | ||
text_constraint( | ||
style.min_size.width, | ||
style.size.width, | ||
style.max_size.width, | ||
scale_factor, | ||
scale_factor_width, | ||
window_width_constraint, | ||
), | ||
text_constraint( | ||
style.min_size.height, | ||
style.size.height, | ||
style.max_size.height, | ||
scale_factor, | ||
scale_factor_height, | ||
window_height_constraint, | ||
), | ||
); | ||
|
||
|
@@ -128,3 +169,71 @@ pub fn text_system( | |
|
||
queued_text.entities = new_queue; | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::text_constraint; | ||
use crate::Val; | ||
|
||
#[test] | ||
fn should_constrain_based_on_pixel_values() { | ||
assert_eq!( | ||
text_constraint(Val::Px(100.), Val::Undefined, Val::Undefined, 1., 1.), | ||
100. | ||
); | ||
assert_eq!( | ||
text_constraint(Val::Undefined, Val::Undefined, Val::Px(100.), 1., 1.), | ||
100. | ||
); | ||
assert_eq!( | ||
text_constraint(Val::Undefined, Val::Px(100.), Val::Undefined, 1., 1.), | ||
100. | ||
); | ||
} | ||
|
||
#[test] | ||
fn should_constrain_based_on_percent_values() { | ||
assert_eq!( | ||
text_constraint(Val::Percent(33.), Val::Undefined, Val::Undefined, 1., 1000.), | ||
330. | ||
); | ||
assert_eq!( | ||
text_constraint(Val::Undefined, Val::Undefined, Val::Percent(33.), 1., 1000.), | ||
330. | ||
); | ||
assert_eq!( | ||
text_constraint(Val::Undefined, Val::Percent(33.), Val::Undefined, 1., 1000.), | ||
330. | ||
); | ||
} | ||
|
||
#[test] | ||
fn should_ignore_min_if_max_is_given() { | ||
assert_eq!( | ||
text_constraint( | ||
Val::Percent(33.), | ||
Val::Undefined, | ||
Val::Percent(50.), | ||
1., | ||
1000. | ||
), | ||
500., | ||
"min in percent and max in percent" | ||
); | ||
assert_eq!( | ||
text_constraint(Val::Px(33.), Val::Undefined, Val::Px(50.), 1., 1000.), | ||
50., | ||
"min in px and max in px" | ||
); | ||
assert_eq!( | ||
text_constraint(Val::Px(33.), Val::Undefined, Val::Percent(50.), 1., 1000.), | ||
500., | ||
"min in px and max in percent" | ||
); | ||
assert_eq!( | ||
text_constraint(Val::Percent(33.), Val::Undefined, Val::Px(50.), 1., 1000.), | ||
50., | ||
"min in percent and max in px" | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,67 @@ | ||||||
use bevy::prelude::*; | ||||||
|
||||||
fn main() { | ||||||
App::new() | ||||||
.insert_resource(ClearColor(Color::rgb(0.4, 0.4, 0.4))) | ||||||
.add_plugins(DefaultPlugins) | ||||||
.add_startup_system(setup) | ||||||
.run(); | ||||||
} | ||||||
Comment on lines
+3
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found two bugs in the example:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for 2. by container I mean the item that wraps the text element, the container itself only fills half the window, so the text content should wrap at half of its container, making the text only a quarter of the window size and half of the container. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should now resize itself when re-scaling the window There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The example for me is different from the screenshot you posted in the initial PR comment, I was referring to the container (the darker area of the screen) as well, it looks like this to me. I'm really not sure what's causing this, my best guess would be something to do with the The code otherwise looks good to merge though. Note that the screenshot is the example ran with your last set of changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh right, sorry I misunderstood you. That's really strange, yeah I'll have to investigate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some borders around the text-nodes would really help here, but I don't think we have implemented that just yet so is something we could improve later |
||||||
|
||||||
fn setup(mut commands: Commands, asset_server: Res<AssetServer>) { | ||||||
commands.spawn_bundle(Camera2dBundle::default()); | ||||||
|
||||||
commands | ||||||
.spawn_bundle(NodeBundle { | ||||||
// Root Panel, covers the entire screen | ||||||
style: Style { | ||||||
size: Size::new(Val::Percent(100.0), Val::Percent(100.0)), | ||||||
justify_content: JustifyContent::SpaceBetween, | ||||||
..default() | ||||||
}, | ||||||
color: Color::NONE.into(), | ||||||
..default() | ||||||
}) | ||||||
.with_children(|parent| { | ||||||
spawn_left_panel(parent, &asset_server); | ||||||
}); | ||||||
} | ||||||
|
||||||
fn spawn_left_panel(parent: &mut ChildBuilder, asset_server: &Res<AssetServer>) { | ||||||
parent | ||||||
.spawn_bundle(NodeBundle { | ||||||
style: Style { | ||||||
flex_direction: FlexDirection::ColumnReverse, | ||||||
size: Size::new(Val::Percent(50.0), Val::Percent(100.0)), | ||||||
..default() | ||||||
}, | ||||||
color: Color::rgb(0.10, 0.10, 0.10).into(), | ||||||
..default() | ||||||
}) | ||||||
.with_children(|parent| { | ||||||
parent | ||||||
.spawn_bundle(TextBundle::from_section( | ||||||
"This is a super long text, that I would hope would get wrapped. It should only fill half the container size", | ||||||
TextStyle { | ||||||
font: asset_server.load("fonts/FiraSans-Bold.ttf"), | ||||||
font_size: 20.0, | ||||||
color: Color::WHITE, | ||||||
} | ||||||
).with_style(Style { | ||||||
max_size: Size::new(Val::Percent(50.0), Val::Undefined), | ||||||
..default() | ||||||
})); | ||||||
parent | ||||||
.spawn_bundle(TextBundle::from_section( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I honestly don't understand why rustfmt isn't doing anything to this line. I tried locally and it also looks like it doesn't care. Can you align this with the spawn_bundle above, the extra indentation looks weird. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After some quick investigation, it somehow looks like rustfmt isn't formatting anything in the entire closure. That seems like a bug in rustfmt |
||||||
"This is another super long text, that I would hope would get wrapped. It should fill the whole container", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you do this for both long strings rustfmt will work correctly
Suggested change
|
||||||
TextStyle { | ||||||
font: asset_server.load("fonts/FiraSans-Bold.ttf"), | ||||||
font_size: 20.0, | ||||||
color: Color::GRAY, | ||||||
} | ||||||
).with_style(Style { | ||||||
max_size: Size::new(Val::Percent(100.0), Val::Undefined), | ||||||
..default() | ||||||
})); | ||||||
}); | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like something that is unit testable. I'd recommend adding a few tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some unit tests