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

Add calculated expressions to Dimension #232

Closed
wants to merge 14 commits into from
Closed

Add calculated expressions to Dimension #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5271156
Move style to module folder
TimJentzsch Sep 28, 2022
ec5803e
Add CalcDimension to calculate dimensions
TimJentzsch Sep 28, 2022
20710d2
Fix errors caused by Dimension not being Copy
TimJentzsch Sep 29, 2022
ed1ee24
Implement maybe_resolve for CalcDimension
TimJentzsch Sep 29, 2022
a8c4952
Move dimension to separate module
TimJentzsch Sep 29, 2022
6da4f75
Implement numerical operations for Dimension
TimJentzsch Sep 29, 2022
abdeb59
Simplify match statements for Mul and Div implementations
TimJentzsch Sep 29, 2022
86e8987
Improve documentation for calculated dimensions
TimJentzsch Oct 12, 2022
aeb06df
Fix errors with style not being copy in tests
TimJentzsch Oct 12, 2022
675623d
Fix calculated dimensions breaking no-std
TimJentzsch Oct 12, 2022
fa28a28
Add value calculation to release notes
TimJentzsch Oct 12, 2022
d6aeabf
Improve documentation for CalcDimension
TimJentzsch Oct 12, 2022
d234d35
Don't clone Dimensions in FlexboxLayout methods
TimJentzsch Oct 12, 2022
b87f83d
Box CalcDimension instead of operation paramters
TimJentzsch Oct 12, 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
11 changes: 10 additions & 1 deletion RELEASES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
# Release Notes

## Unreleased

### Unreleased Added

- `taffy::style::Dimension` now implements the `+`, `-`, `*` and `/` operators.
- If both sides of the calculation have the same type (e.g. points), the result is calculated immediately.
- If both sides have different types (e.g. adding points and percentage), the new `Dimension::Calc` variant is constructed. The result will be calculated when the absolute value is resolved.
- If any side is `Dimension::Undefined`, the result will be `Dimension::Undefined`. The same happens for `Dimension::Auto`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that having Dimension::Auto on any side resolved to Dimension::Auto or Dimension::Undefined? The wording of this sentence makes it ambiguous.


## 0.2.0

### 0.2.0 Added
Expand All @@ -14,7 +23,7 @@
### 0.2.0 Changed

- `Size<f32>.zero()` is now `Size::<f32>::ZERO`
- `Point<f32>.zero()` is now `Point::<f32>::ZERO`
- `Point<f32>.zero()` is now `Point::<f32>::ZERO`
- renamed `taffy::node::Taffy.new_leaf()` -> `taffy::node::Taffy.new_leaf_with_measure()`
- removed the public `Number` type; a more idiomatic `Option<f32>` is used instead
- the associated public `MinMax` and `OrElse` traits have also been removed; these should never have been public
Expand Down
65 changes: 33 additions & 32 deletions src/flexbox.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ struct AlgoConstants {
impl Taffy {
/// Computes the layout of [`Taffy`] according to the flexbox algorithm
pub(crate) fn compute(&mut self, root: Node, size: Size<Option<f32>>) {
let style = self.nodes[root].style;
let style = self.nodes[root].style.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this should use a reference rather than a clone.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't get that to work:

error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
   --> src/flexbox.rs:119:30
    |
112 |         let style = &self.nodes[root].style;
    |                      ---------- immutable borrow occurs here
...
119 |             let first_pass = self.compute_preliminary(root, style.size.maybe_resolve(size), size, false, true);
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
...
126 |                         .maybe_max(style.min_size.width.maybe_resolve(size.width))
    |                                    ---------------------------------------------- immutable borrow later used here

For more information about this error, try `rustc --explain E0502`.
error: could not compile `taffy` due to previous error

Copy link
Collaborator

Choose a reason for hiding this comment

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

At worst you should be able to clone just the individual properties you want (size, min_size, max_size) rather than the whole style struct. But I think you can avoid even that by pulling the maybe_resolve calculations up above the compute_preliminary calls and dropping the reference before making those calls.

let has_root_min_max = style.min_size.width.is_defined()
|| style.min_size.height.is_defined()
|| style.max_size.width.is_defined()
Expand Down Expand Up @@ -277,7 +277,7 @@ impl Taffy {
min_size: child_style.min_size.maybe_resolve(constants.node_inner_size),
max_size: child_style.max_size.maybe_resolve(constants.node_inner_size),

position: child_style.position.zip_size(constants.node_inner_size, |p, s| p.maybe_resolve(s)),
position: child_style.position.clone().zip_size(constants.node_inner_size, |p, s| p.maybe_resolve(s)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it not possible to avoid the clone here? It's being resolved later on this line, which indicates to me that it should be possible. Maybe it's just zip_size method having the wrong signature that's causing the issue here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we change zip_size to take &self then we have to clone in the implementation of that function when creating the new Rect. Not sure if that's better or worse.

Copy link
Collaborator

Choose a reason for hiding this comment

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

position: Size {
     width: child_style.position.width.maybe_resolve(constants.node_inner_size.width),
     height: child_style.position.height.maybe_resolve(constants.node_inner_size.height),
}

ought to work, right? So I think something's being lost in the zip abstraction somewhere.

margin: child_style.margin.resolve_or_default(constants.node_inner_size.width),
padding: child_style.padding.resolve_or_default(constants.node_inner_size.width),
border: child_style.border.resolve_or_default(constants.node_inner_size.width),
Expand Down Expand Up @@ -372,11 +372,12 @@ impl Taffy {
) {
// TODO - this does not follow spec. See the TODOs below
for child in flex_items.iter_mut() {
let child_style = self.nodes[child.node].style;
let child_style = &self.nodes[child.node].style;

// A. If the item has a definite used flex basis, that’s the flex base size.

let flex_basis = child_style.flex_basis.maybe_resolve(constants.node_inner_size.main(constants.dir));
let flex_basis =
child_style.flex_basis.clone().maybe_resolve(constants.node_inner_size.main(constants.dir));
if flex_basis.is_some() {
child.flex_basis = flex_basis.unwrap_or(0.0);
continue;
Expand Down Expand Up @@ -934,9 +935,9 @@ impl Taffy {
.map(|child| {
let child_style = &self.nodes[child.node].style;
if child_style.align_self(&self.nodes[node].style) == AlignSelf::Baseline
&& child_style.cross_margin_start(constants.dir) != Dimension::Auto
&& child_style.cross_margin_end(constants.dir) != Dimension::Auto
&& child_style.cross_size(constants.dir) == Dimension::Auto
&& child_style.cross_margin_start(constants.dir) != &Dimension::Auto
&& child_style.cross_margin_end(constants.dir) != &Dimension::Auto
&& child_style.cross_size(constants.dir) == &Dimension::Auto
{
max_baseline - child.baseline + child.hypothetical_outer_size.cross(constants.dir)
} else {
Expand Down Expand Up @@ -998,9 +999,9 @@ impl Taffy {
child.target_size.set_cross(
constants.dir,
if child_style.align_self(&self.nodes[node].style) == AlignSelf::Stretch
&& child_style.cross_margin_start(constants.dir) != Dimension::Auto
&& child_style.cross_margin_end(constants.dir) != Dimension::Auto
&& child_style.cross_size(constants.dir) == Dimension::Auto
&& child_style.cross_margin_start(constants.dir) != &Dimension::Auto
&& child_style.cross_margin_end(constants.dir) != &Dimension::Auto
&& child_style.cross_size(constants.dir) == &Dimension::Auto
{
(line_cross_size - child.margin.cross_axis_sum(constants.dir))
.maybe_max(child.min_size.cross(constants.dir))
Expand Down Expand Up @@ -1037,10 +1038,10 @@ impl Taffy {

for child in line.items.iter_mut() {
let child_style = &self.nodes[child.node].style;
if child_style.main_margin_start(constants.dir) == Dimension::Auto {
if child_style.main_margin_start(constants.dir) == &Dimension::Auto {
num_auto_margins += 1;
}
if child_style.main_margin_end(constants.dir) == Dimension::Auto {
if child_style.main_margin_end(constants.dir) == &Dimension::Auto {
num_auto_margins += 1;
}
}
Expand All @@ -1050,14 +1051,14 @@ impl Taffy {

for child in line.items.iter_mut() {
let child_style = &self.nodes[child.node].style;
if child_style.main_margin_start(constants.dir) == Dimension::Auto {
if child_style.main_margin_start(constants.dir) == &Dimension::Auto {
if constants.is_row {
child.margin.start = margin;
} else {
child.margin.top = margin;
}
}
if child_style.main_margin_end(constants.dir) == Dimension::Auto {
if child_style.main_margin_end(constants.dir) == &Dimension::Auto {
if constants.is_row {
child.margin.end = margin;
} else {
Expand Down Expand Up @@ -1143,8 +1144,8 @@ impl Taffy {
let free_space = line_cross_size - child.outer_target_size.cross(constants.dir);
let child_style = &self.nodes[child.node].style;

if child_style.cross_margin_start(constants.dir) == Dimension::Auto
&& child_style.cross_margin_end(constants.dir) == Dimension::Auto
if child_style.cross_margin_start(constants.dir) == &Dimension::Auto
&& child_style.cross_margin_end(constants.dir) == &Dimension::Auto
{
if constants.is_row {
child.margin.top = free_space / 2.0;
Expand All @@ -1153,13 +1154,13 @@ impl Taffy {
child.margin.start = free_space / 2.0;
child.margin.end = free_space / 2.0;
}
} else if child_style.cross_margin_start(constants.dir) == Dimension::Auto {
} else if child_style.cross_margin_start(constants.dir) == &Dimension::Auto {
if constants.is_row {
child.margin.top = free_space;
} else {
child.margin.start = free_space;
}
} else if child_style.cross_margin_end(constants.dir) == Dimension::Auto {
} else if child_style.cross_margin_end(constants.dir) == &Dimension::Auto {
if constants.is_row {
child.margin.bottom = free_space;
} else {
Expand Down Expand Up @@ -1467,7 +1468,7 @@ impl Taffy {
let container_width = constants.container_size.width.into();
let container_height = constants.container_size.height.into();

let child_style = self.nodes[child].style;
let child_style = self.nodes[child].style.clone();
Comment on lines -1470 to +1471
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think perhaps child_style here ought to be a reference rather than a clone.


// X-axis
let child_position_start = child_style.position.start.maybe_resolve(container_width);
Expand Down Expand Up @@ -1819,25 +1820,25 @@ mod tests {
let mut tree = Taffy::with_capacity(16);

let style = FlexboxLayout::default();
let node_id = tree.new_leaf(style).unwrap();
let node_id = tree.new_leaf(style.clone()).unwrap();

let node_size = Size::NONE;
let parent_size = Size::NONE;

let constants = Taffy::compute_constants(&tree.nodes[node_id], node_size, parent_size);

assert!(constants.dir == style.flex_direction);
assert!(constants.is_row == style.flex_direction.is_row());
assert!(constants.is_column == style.flex_direction.is_column());
assert!(constants.is_wrap_reverse == (style.flex_wrap == FlexWrap::WrapReverse));
assert!(constants.dir == style.clone().flex_direction);
assert!(constants.is_row == style.clone().flex_direction.is_row());
assert!(constants.is_column == style.clone().flex_direction.is_column());
assert!(constants.is_wrap_reverse == (style.clone().flex_wrap == FlexWrap::WrapReverse));

let margin = style.margin.resolve_or_default(parent_size);
let margin = style.clone().margin.resolve_or_default(parent_size);
assert_eq!(constants.margin, margin);

let border = style.border.resolve_or_default(parent_size);
let border = style.clone().border.resolve_or_default(parent_size);
assert_eq!(constants.border, border);

let padding = style.padding.resolve_or_default(parent_size);
let padding = style.clone().padding.resolve_or_default(parent_size);

// TODO: Replace with something less hardcoded?
let padding_border = Rect {
Expand Down Expand Up @@ -1867,15 +1868,15 @@ mod tests {
let style: FlexboxLayout =
FlexboxLayout { display: Flex, size: Size::from_points(50.0, 50.0), ..Default::default() };

let grandchild_00 = taffy.new_leaf(style).unwrap();
let grandchild_00 = taffy.new_leaf(style.clone()).unwrap();

let grandchild_01 = taffy.new_leaf(style).unwrap();
let grandchild_01 = taffy.new_leaf(style.clone()).unwrap();

let grandchild_02 = taffy.new_leaf(style).unwrap();
let grandchild_02 = taffy.new_leaf(style.clone()).unwrap();

let child_00 = taffy.new_with_children(style, &[grandchild_00, grandchild_01]).unwrap();
let child_00 = taffy.new_with_children(style.clone(), &[grandchild_00, grandchild_01]).unwrap();

let child_01 = taffy.new_with_children(style, &[grandchild_02]).unwrap();
let child_01 = taffy.new_with_children(style.clone(), &[grandchild_02]).unwrap();

let root = taffy
.new_with_children(
Expand Down
2 changes: 1 addition & 1 deletion src/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ mod tests {
let style =
FlexboxLayout { display: Display::None, flex_direction: FlexDirection::RowReverse, ..Default::default() };

let node = taffy.new_leaf(style).unwrap();
let node = taffy.new_leaf(style.clone()).unwrap();

let res = taffy.style(node);
assert!(res.is_ok());
Expand Down
21 changes: 12 additions & 9 deletions src/resolve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::prelude::{Dimension, Rect, Size};
/// Will return a default value if it unable to resolve.
pub(crate) trait ResolveOrDefault<TContext, TOutput> {
/// Resolve a dimension that might be dependent on a context, with a default fallback value
fn resolve_or_default(self, context: TContext) -> TOutput;
fn resolve_or_default(&self, context: TContext) -> TOutput;
}

/// Trait to encapsulate behaviour where we need to resolve from a
Expand All @@ -19,39 +19,42 @@ pub(crate) trait ResolveOrDefault<TContext, TOutput> {
/// Will return a `None` if it unable to resolve.
pub(crate) trait MaybeResolve<T> {
/// Resolve a dimension that might be dependent on a context, with `None` as fallback value
fn maybe_resolve(self, context: T) -> T;
fn maybe_resolve(&self, context: T) -> T;
}

impl MaybeResolve<Option<f32>> for Dimension {
/// Converts the given [`Dimension`] into a concrete value of points
///
/// Can return `None`
fn maybe_resolve(self, context: Option<f32>) -> Option<f32> {
fn maybe_resolve(&self, context: Option<f32>) -> Option<f32> {
match self {
Dimension::Points(points) => Some(points),
Dimension::Points(points) => Some(*points),
// parent_dim * percent
Dimension::Percent(percent) => context.map(|dim| dim * percent),
_ => None,
Dimension::Auto | Dimension::Undefined => None,

#[cfg(feature = "std")]
Dimension::Calc(calc) => calc.maybe_resolve(context),
Comment on lines +36 to +37
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that Calc only work with the std feature enabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently yes, because it requires Box. Maybe alloc would be enough though.

}
}
}

impl MaybeResolve<Size<Option<f32>>> for Size<Dimension> {
/// Converts any `parent`-relative values for size into an absolute size
fn maybe_resolve(self, context: Size<Option<f32>>) -> Size<Option<f32>> {
fn maybe_resolve(&self, context: Size<Option<f32>>) -> Size<Option<f32>> {
Size { width: self.width.maybe_resolve(context.width), height: self.height.maybe_resolve(context.height) }
}
}

impl ResolveOrDefault<Option<f32>, f32> for Dimension {
/// Will return a default value of result is evaluated to `None`
fn resolve_or_default(self, context: Option<f32>) -> f32 {
fn resolve_or_default(&self, context: Option<f32>) -> f32 {
self.maybe_resolve(context).unwrap_or(0.0)
}
}

impl ResolveOrDefault<Size<Option<f32>>, Rect<f32>> for Rect<Dimension> {
fn resolve_or_default(self, context: Size<Option<f32>>) -> Rect<f32> {
fn resolve_or_default(&self, context: Size<Option<f32>>) -> Rect<f32> {
Rect {
start: self.start.resolve_or_default(context.width),
end: self.end.resolve_or_default(context.width),
Expand All @@ -62,7 +65,7 @@ impl ResolveOrDefault<Size<Option<f32>>, Rect<f32>> for Rect<Dimension> {
}

impl ResolveOrDefault<Option<f32>, Rect<f32>> for Rect<Dimension> {
fn resolve_or_default(self, context: Option<f32>) -> Rect<f32> {
fn resolve_or_default(&self, context: Option<f32>) -> Rect<f32> {
Rect {
start: self.start.resolve_or_default(context),
end: self.end.resolve_or_default(context),
Expand Down
58 changes: 58 additions & 0 deletions src/style/calc_dimension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
//! Dimensions that are have to be calculated while resolving.

use crate::resolve::MaybeResolve;

use super::Dimension;

/// An addition, subtraction, multiplication or division between [`Dimension`]s.
///
/// The values are calulated when the point values are resolved.
///
/// This type is needed, because not all calculations can be determined before resolving the actual values.
/// For example, when adding [`Dimension::Points`] and [`Dimension::Percent`] together,
/// the percentage first needs to be resolved to an absolute value to get the final result.
#[derive(Clone, PartialEq, Debug)]
pub enum CalcDimension {
/// Add two [`Dimension`]s together.
Add(Dimension, Dimension),

/// Subtract the right [`Dimension`] from the left one.
Sub(Dimension, Dimension),

/// Multiply a [`Dimension`] with a constant.
Mul(Dimension, f32),

/// Divide a [`Dimension`] by a constant.
Div(Dimension, f32),
}

impl MaybeResolve<Option<f32>> for CalcDimension {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Should this live in calc_dimension.rs or in resolve.rs? I think all other implementations of resolve is so far done in resolve.rs, but I don't know if that improves or reduces discoverability / organisation.

I don't feel strongly in either direction.

/// Resolve the calculations to concrete values.
///
/// If any side resolves to [`None`], [`None`] is also returned.
fn maybe_resolve(&self, context: Option<f32>) -> Option<f32> {
match self {
// Resolve both sides and add them together
// If either side resolves to `None`, return `None`
CalcDimension::Add(lhs, rhs) => lhs
.maybe_resolve(context)
.zip(rhs.maybe_resolve(context))
.map(|(lhs_value, rhs_value)| lhs_value + rhs_value),

// Resolve both sides and subtract them
// If either side resolves to `None`, return `None`
CalcDimension::Sub(lhs, rhs) => lhs
.maybe_resolve(context)
.zip(rhs.maybe_resolve(context))
.map(|(lhs_value, rhs_value)| lhs_value - rhs_value),

// Resolve the left side and multiply it by the factor
// If the left side resolves to `None`, return `None`
CalcDimension::Mul(lhs, factor) => lhs.maybe_resolve(context).map(|lhs_value| lhs_value * factor),

// Resolve the left side and divide it by the factor
// If the left side resolves to `None`, return `None`
CalcDimension::Div(lhs, divisor) => lhs.maybe_resolve(context).map(|lhs_value| lhs_value / divisor),
}
}
}
Loading