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

Jump to bottom

Refactor Style component into many smaller pieces #5513

Closed
wants to merge 1 commit into from
Closed

Refactor Style component into many smaller pieces #5513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
83 changes: 45 additions & 38 deletions crates/bevy_ui/src/flex/convert.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
use crate::{
AlignContent, AlignItems, AlignSelf, Display, FlexDirection, FlexWrap, JustifyContent,
PositionType, Size, Style, UiRect, Val,
layout_components::{
flex::{
AlignContent, AlignItems, AlignSelf, Direction, FlexLayoutQueryItem, JustifyContent,
Wrap,
},
Position,
},
Display,
};
use crate::{Size, UiRect, Val};

pub fn from_rect(
scale_factor: f64,
Expand Down Expand Up @@ -32,28 +39,28 @@ pub fn from_val_size(
}
}

pub fn from_style(scale_factor: f64, value: &Style) -> taffy::style::Style {
pub fn from_flex_layout(scale_factor: f64, value: FlexLayoutQueryItem<'_>) -> taffy::style::Style {
taffy::style::Style {
display: value.display.into(),
position_type: value.position_type.into(),
flex_direction: value.flex_direction.into(),
flex_wrap: value.flex_wrap.into(),
align_items: value.align_items.into(),
align_self: value.align_self.into(),
align_content: value.align_content.into(),
justify_content: value.justify_content.into(),
position: from_rect(scale_factor, value.position),
margin: from_rect(scale_factor, value.margin),
padding: from_rect(scale_factor, value.padding),
border: from_rect(scale_factor, value.border),
flex_grow: value.flex_grow,
flex_shrink: value.flex_shrink,
flex_basis: from_val(scale_factor, value.flex_basis),
size: from_val_size(scale_factor, value.size),
min_size: from_val_size(scale_factor, value.min_size),
max_size: from_val_size(scale_factor, value.max_size),
aspect_ratio: value.aspect_ratio,
gap: from_val_size(scale_factor, value.gap),
display: (value.control.display).into(),
position_type: (value.control.position).into(),
flex_direction: value.layout.direction.into(),
flex_wrap: value.layout.wrap.into(),
align_items: value.layout.align_items.into(),
align_self: value.child_layout.align_self.into(),
align_content: value.layout.align_content.into(),
justify_content: value.layout.justify_content.into(),
position: from_rect(scale_factor, value.control.inset.0),
margin: from_rect(scale_factor, value.spacing.margin),
padding: from_rect(scale_factor, value.spacing.padding),
border: from_rect(scale_factor, value.spacing.border),
flex_grow: value.child_layout.grow,
flex_shrink: value.child_layout.shrink,
flex_basis: from_val(scale_factor, value.child_layout.basis),
size: from_val_size(scale_factor, value.size_constraints.suggested),
min_size: from_val_size(scale_factor, value.size_constraints.min),
max_size: from_val_size(scale_factor, value.size_constraints.max),
aspect_ratio: value.size_constraints.aspect_ratio,
gap: from_val_size(scale_factor, value.layout.gap),
}
}

Expand Down Expand Up @@ -122,13 +129,13 @@ impl From<Display> for taffy::style::Display {
}
}

impl From<FlexDirection> for taffy::style::FlexDirection {
fn from(value: FlexDirection) -> Self {
impl From<Direction> for taffy::style::FlexDirection {
fn from(value: Direction) -> Self {
match value {
FlexDirection::Row => taffy::style::FlexDirection::Row,
FlexDirection::Column => taffy::style::FlexDirection::Column,
FlexDirection::RowReverse => taffy::style::FlexDirection::RowReverse,
FlexDirection::ColumnReverse => taffy::style::FlexDirection::ColumnReverse,
Direction::Row => taffy::style::FlexDirection::Row,
Direction::Column => taffy::style::FlexDirection::Column,
Direction::RowReverse => taffy::style::FlexDirection::RowReverse,
Direction::ColumnReverse => taffy::style::FlexDirection::ColumnReverse,
}
}
}
Expand All @@ -146,21 +153,21 @@ impl From<JustifyContent> for taffy::style::JustifyContent {
}
}

impl From<PositionType> for taffy::style::PositionType {
fn from(value: PositionType) -> Self {
impl From<Position> for taffy::style::PositionType {
fn from(value: Position) -> Self {
match value {
PositionType::Relative => taffy::style::PositionType::Relative,
PositionType::Absolute => taffy::style::PositionType::Absolute,
Position::Relative => taffy::style::PositionType::Relative,
Position::Absolute => taffy::style::PositionType::Absolute,
}
}
}

impl From<FlexWrap> for taffy::style::FlexWrap {
fn from(value: FlexWrap) -> Self {
impl From<Wrap> for taffy::style::FlexWrap {
fn from(value: Wrap) -> Self {
match value {
FlexWrap::NoWrap => taffy::style::FlexWrap::NoWrap,
FlexWrap::Wrap => taffy::style::FlexWrap::Wrap,
FlexWrap::WrapReverse => taffy::style::FlexWrap::WrapReverse,
Wrap::NoWrap => taffy::style::FlexWrap::NoWrap,
Wrap::Wrap => taffy::style::FlexWrap::Wrap,
Wrap::WrapReverse => taffy::style::FlexWrap::WrapReverse,
}
}
}
41 changes: 27 additions & 14 deletions crates/bevy_ui/src/flex/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
mod convert;

use crate::{CalculatedSize, Node, Style, UiScale};
use crate::{
layout_components::flex::{FlexLayoutChanged, FlexLayoutQuery, FlexLayoutQueryItem},
CalculatedSize, Node, UiScale,
};
use bevy_ecs::{
change_detection::DetectChanges,
entity::Entity,
Expand Down Expand Up @@ -60,10 +63,17 @@ impl Default for FlexSurface {
}

impl FlexSurface {
pub fn upsert_node(&mut self, entity: Entity, style: &Style, scale_factor: f64) {
/// Inserts the node into the hashmap if they don't already exist,
/// or update the contents if they already to exist.
pub fn upsert_node(
Copy link
Contributor

@TimJentzsch TimJentzsch Nov 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can we add a short comment to this function explaining what it does?
I didn't know at first what "upsert" means.

From some dictionary:

To insert rows into a database table (if they do not already exist) or update them (if they do).

(Doesn't necessarily have to be in this PR though)

&mut self,
entity: Entity,
layout: FlexLayoutQueryItem<'_>,
scale_factor: f64,
) {
let mut added = false;
let taffy = &mut self.taffy;
let taffy_style = convert::from_style(scale_factor, style);
let taffy_style = convert::from_flex_layout(scale_factor, layout);
let taffy_node = self.entity_to_taffy.entry(entity).or_insert_with(|| {
added = true;
taffy.new_leaf(taffy_style).unwrap()
Expand All @@ -77,12 +87,12 @@ impl FlexSurface {
pub fn upsert_leaf(
&mut self,
entity: Entity,
style: &Style,
layout: FlexLayoutQueryItem<'_>,
calculated_size: CalculatedSize,
scale_factor: f64,
) {
let taffy = &mut self.taffy;
let taffy_style = convert::from_style(scale_factor, style);
let taffy_style = convert::from_flex_layout(scale_factor, layout);
let measure = taffy::node::MeasureFunc::Boxed(Box::new(
move |constraints: Size<Option<f32>>, _available: Size<AvailableSpace>| {
let mut size = convert::from_f32_size(scale_factor, calculated_size.size);
Expand Down Expand Up @@ -227,10 +237,13 @@ pub fn flex_node_system(
mut scale_factor_events: EventReader<WindowScaleFactorChanged>,
mut flex_surface: ResMut<FlexSurface>,
root_node_query: Query<Entity, (With<Node>, Without<Parent>)>,
node_query: Query<(Entity, &Style, Option<&CalculatedSize>), (With<Node>, Changed<Style>)>,
full_node_query: Query<(Entity, &Style, Option<&CalculatedSize>), With<Node>>,
node_query: Query<
(Entity, FlexLayoutQuery, Option<&CalculatedSize>),
(With<Node>, FlexLayoutChanged),
>,
full_node_query: Query<(Entity, FlexLayoutQuery, Option<&CalculatedSize>), With<Node>>,
changed_size_query: Query<
(Entity, &Style, &CalculatedSize),
(Entity, FlexLayoutQuery, &CalculatedSize),
(With<Node>, Changed<CalculatedSize>),
>,
children_query: Query<(Entity, &Children), (With<Node>, Changed<Children>)>,
Expand All @@ -257,15 +270,15 @@ pub fn flex_node_system(
fn update_changed<F: ReadOnlyWorldQuery>(
flex_surface: &mut FlexSurface,
scaling_factor: f64,
query: Query<(Entity, &Style, Option<&CalculatedSize>), F>,
query: Query<(Entity, FlexLayoutQuery, Option<&CalculatedSize>), F>,
) {
// update changed nodes
for (entity, style, calculated_size) in &query {
for (entity, layout, calculated_size) in &query {
// TODO: remove node from old hierarchy if its root has changed
if let Some(calculated_size) = calculated_size {
flex_surface.upsert_leaf(entity, style, *calculated_size, scaling_factor);
flex_surface.upsert_leaf(entity, layout, *calculated_size, scaling_factor);
} else {
flex_surface.upsert_node(entity, style, scaling_factor);
flex_surface.upsert_node(entity, layout, scaling_factor);
}
}
}
Expand All @@ -276,8 +289,8 @@ pub fn flex_node_system(
update_changed(&mut flex_surface, scale_factor, node_query);
}

for (entity, style, calculated_size) in &changed_size_query {
flex_surface.upsert_leaf(entity, style, *calculated_size, scale_factor);
for (entity, layout, calculated_size) in &changed_size_query {
flex_surface.upsert_leaf(entity, layout, *calculated_size, scale_factor);
}

// clean up removed nodes
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ui/src/focus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use smallvec::SmallVec;
/// This ensures that hidden UI nodes are not interactable,
/// and do not end up stuck in an active state if hidden at the wrong time.
///
/// Note that you can also control the visibility of a node using the [`Display`](crate::ui_node::Display) property,
/// Note that you can also control the visibility of a node using the [`Display`](crate::layout_components::Display) property,
/// which fully collapses it during layout calculations.
#[derive(Component, Copy, Clone, Eq, PartialEq, Debug, Reflect, Serialize, Deserialize)]
#[reflect(Component, Serialize, Deserialize, PartialEq)]
Expand Down
115 changes: 47 additions & 68 deletions crates/bevy_ui/src/geometry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use crate::Val;
use bevy_reflect::Reflect;
use serde::{Deserialize, Serialize};
use std::ops::{Div, DivAssign, Mul, MulAssign};

/// A type which is commonly used to define positions, margins, paddings and borders.
/// A type which is commonly used throughout UI and layouting, commonly used to define inset, margins, paddings and borders.
///
/// # Examples
///
Expand Down Expand Up @@ -65,61 +66,17 @@ use std::ops::{Div, DivAssign, Mul, MulAssign};
/// right values of the position because the size of the UI element is already explicitly specified.
///
/// ```
/// # use bevy_ui::{UiRect, Size, Val, Style};
/// # use bevy_ui::{UiRect, Size, Val, SizeConstraints, Inset};
/// # use bevy_utils::default;
/// #
/// let style = Style {
/// position: UiRect { // Defining all four sides
/// left: Val::Px(100.0),
/// right: Val::Px(200.0),
/// top: Val::Px(300.0),
/// bottom: Val::Px(400.0),
/// },
/// size: Size::new(Val::Percent(100.0), Val::Percent(50.0)), // but also explicitly specifying a size
/// ..default()
/// };
/// ```
///
/// ## Margin
///
/// A margin is used to create space around UI elements, outside of any defined borders.
///
/// ```
/// # use bevy_ui::{UiRect, Val};
/// #
/// let margin = UiRect::all(Val::Auto); // Centers the UI element
/// ```
///
/// ## Padding
///
/// A padding is used to create space around UI elements, inside of any defined borders.
///
/// ```
/// # use bevy_ui::{UiRect, Val};
/// #
/// let padding = UiRect {
/// left: Val::Px(10.0),
/// right: Val::Px(20.0),
/// top: Val::Px(30.0),
/// bottom: Val::Px(40.0),
/// };
/// ```
///
/// ## Borders
///
/// A border is used to define the width of the border of a UI element.
/// let inset = Inset(UiRect {
/// left: Val::Px(100.0),
/// right: Val::Px(200.0),
/// top: Val::Px(300.0),
/// bottom: Val::Px(400.0),
/// });
///
/// ```
/// # use bevy_ui::{UiRect, Val};
/// #
/// let border = UiRect {
/// left: Val::Px(10.0),
/// right: Val::Px(20.0),
/// top: Val::Px(30.0),
/// bottom: Val::Px(40.0),
/// };
/// ```
#[derive(Copy, Clone, PartialEq, Debug, Reflect)]
#[derive(Copy, Clone, PartialEq, Debug, Reflect, Serialize, Deserialize)]
#[reflect(PartialEq)]
pub struct UiRect {
/// The value corresponding to the left side of the UI rect.
Expand All @@ -133,13 +90,6 @@ pub struct UiRect {
}

impl UiRect {
pub const DEFAULT: Self = Self {
left: Val::DEFAULT,
right: Val::DEFAULT,
top: Val::DEFAULT,
bottom: Val::DEFAULT,
};

/// Creates a new [`UiRect`] from the values specified.
///
/// # Example
Expand Down Expand Up @@ -318,6 +268,15 @@ impl UiRect {
..Default::default()
}
}

/// Alias of [`UiRect::UNDEFINED`]
pub const DEFAULT: UiRect = UiRect::UNDEFINED;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the use for DEFAULT if we already have default().

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately the default trait's method is not usable in const contexts currently. It also produces less helpful docs, as the values are not shown on hover.

I'm fine to cut this from this PR though.

Copy link

Choose a reason for hiding this comment

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

Having a DEFAULT alias here isn't too painful imo, as it clarifies what should be used by default.

Perhaps a docs would be nice to have? I would suggest:

/// Alias of [`UiRect::UNDEFINED`]


/// Creates a [`UiRect`] where all sides are [`Val::Undefined`]
pub const UNDEFINED: UiRect = UiRect::all(Val::Undefined);

/// Creates a [`UiRect`] where all sides are [`Val::Auto`]
pub const AUTO: UiRect = UiRect::all(Val::Auto);
Comment on lines +275 to +279
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have it documented somewhere what the difference between undefined and auto is?

Copy link
Contributor

Choose a reason for hiding this comment

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

@TimJentzsch In general the meaning of Auto depends on the property being set. For a Rect, the main property where Auto is relevant is margin where Auto means "take up remaining available space" and Undefined resolves to zero. Currently Taffy and (bevy_ui) also accept Auto for padding and border where it is no different to Undefined. Taffy 0.3 (unreleased) is more strict about this and introduces a new type with no Auto variant for padding and border. I'm unsure as whether bevy_ui will follow Taffy in this regard.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure as whether bevy_ui will follow Taffy in this regard.

I would be strongly in favour of doing so.

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point we should probably have that in the doc comments as well, of the respective components.

}

impl Default for UiRect {
Expand All @@ -329,7 +288,7 @@ impl Default for UiRect {
/// A 2-dimensional area defined by a width and height.
///
/// It is commonly used to define the size of a text or UI element.
#[derive(Copy, Clone, PartialEq, Debug, Reflect)]
#[derive(Copy, Clone, PartialEq, Debug, Reflect, Serialize, Deserialize)]
#[reflect(PartialEq)]
pub struct Size {
/// The width of the 2-dimensional area.
Expand All @@ -339,11 +298,6 @@ pub struct Size {
}

impl Size {
pub const DEFAULT: Self = Self {
width: Val::DEFAULT,
height: Val::DEFAULT,
};

/// Creates a new [`Size`] from a width and a height.
///
/// # Example
Expand All @@ -360,17 +314,42 @@ impl Size {
Size { width, height }
}

/// Creates a Size where both values are [`Val::Auto`].
/// Creates a new [`Size`] where both values are given in [`Val::Px`].
pub const fn px(width: f32, height: f32) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd actually really like the same for UiRect, it's annoyingly verbose to create now. But this can be on another PR.

Size {
width: Val::Px(width),
height: Val::Px(height),
}
}

/// Creates a new [`Size`] where both values are given in [`Val::Percent`].
pub const fn percent(width: f32, height: f32) -> Self {
Size {
width: Val::Percent(width),
height: Val::Percent(height),
}
}

/// Alias of [`Size::UNDEFINED`]
pub const DEFAULT: Size = Size::UNDEFINED;
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved

/// Creates a new [`Size`] where both values are [`Val::Auto`].
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
pub const AUTO: Size = Size {
width: Val::Auto,
height: Val::Auto,
};

/// Creates a Size where both values are [`Val::Undefined`].
/// Creates a new [`Size`] where both values are [`Val::Undefined`].
pub const UNDEFINED: Size = Size {
width: Val::Undefined,
height: Val::Undefined,
};

/// Creates a new [`Size`] where both values are 100 percent.
pub const FULL: Size = Size {
width: Val::FULL,
height: Val::FULL,
};
}

impl Default for Size {
Expand Down
Loading