-
-
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 1 commit
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,28 @@ 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::Px(min), _, _) => scale_value(min, scale_factor), | ||
(Val::Undefined, Val::Px(size), Val::Undefined) | (Val::Auto, Val::Px(size), Val::Auto) => { | ||
scale_value(size, scale_factor) | ||
} | ||
(Val::Percent(min), _, _) => scale_value((min / 100.) * window_size, scale_factor), | ||
(_, _, Val::Percent(max)) => scale_value((max / 100.) * window_size, scale_factor), | ||
(Val::Undefined, Val::Percent(size), Val::Undefined) => { | ||
scale_value((size / 100.) * window_size, scale_factor) | ||
} | ||
(Val::Auto, Val::Percent(size), Val::Auto) => { | ||
scale_value((size / 100.) * window_size, scale_factor) | ||
} | ||
_ => f32::MAX, | ||
} | ||
} | ||
|
@@ -49,10 +65,16 @@ 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>)>, | ||
)>, | ||
parent_query: Query<&Style>, | ||
louiidev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) { | ||
let scale_factor = windows.scale_factor(WindowId::primary()); | ||
let (scale_factor, width_constraint, height_constraint) = | ||
louiidev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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; | ||
|
||
|
@@ -78,19 +100,40 @@ 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) = parent_query.get(parent.get()) { | ||
match style.size.width { | ||
Val::Percent(percentage) => { | ||
scale_factor_width = percentage as f64 / (scale_factor * 100.) | ||
} | ||
_ => {} | ||
} | ||
match style.size.height { | ||
Val::Percent(percentage) => { | ||
scale_factor_height = percentage as f64 / (scale_factor * 100.) | ||
} | ||
_ => {} | ||
} | ||
} | ||
} | ||
|
||
let node_size = Vec2::new( | ||
text_constraint( | ||
style.min_size.width, | ||
style.size.width, | ||
style.max_size.width, | ||
scale_factor, | ||
scale_factor_width, | ||
width_constraint, | ||
), | ||
text_constraint( | ||
style.min_size.height, | ||
style.size.height, | ||
style.max_size.height, | ||
scale_factor, | ||
scale_factor_height, | ||
height_constraint, | ||
), | ||
); | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,70 @@ | ||||||
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::default() | ||||||
louiidev marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
}, | ||||||
color: Color::NONE.into(), | ||||||
..Default::default() | ||||||
louiidev marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
}) | ||||||
.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::default() | ||||||
louiidev marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
}, | ||||||
color: Color::rgb(0.10, 0.10, 0.10).into(), | ||||||
..Default::default() | ||||||
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.
Suggested change
ibid |
||||||
}) | ||||||
.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::default() | ||||||
louiidev marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
})); | ||||||
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::default() | ||||||
louiidev marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
})); | ||||||
|
||||||
}); | ||||||
|
||||||
|
||||||
} |
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