Add calculated expressions to `Dimension` #232

TimJentzsch · 2022-09-29T18:45:15Z

Objective

Closes #225, closes #229.

This PR will add calculated expressions to Dimension, similar to calc() in CSS.
This allows things like adding a percentage value to a point value, which can only be calculated when resolving the values (because then we know what the percentage values stand for).

Solution

To accomplish this, a new Calc variant has been added to Dimension.
This is itself an enum, with the Add, Sub, Mul and Div variants.

The Add and Sub variants work with two Dimensions (which have to be Boxed, to have the size known at compile-time).

The Mul and Div variants work with one Dimension (also Boxed) and one f32, similar to calc().

Additionally, the Add, Sub, Mul and Div traits have been implemented on Dimension. This makes it a lot more convenient and intuitive to do the operations.

If possible, the operations will try to calculate the final result already, for example when adding two point values together. If this is not possible (e.g. adding percentage and point values), then the appropriate Calc variant is constructed.

If any of these operations encounter Dimension::Undefined, the result will be Dimension::Undefined, similarly for Dimension::Auto.
This might be a potential footgun for users, but I'm not sure if there's a better way to handle this.

A rather big, unfortunate side-effect of these changes is that Dimension and FlexboxLayout can no longer derive Copy (because Box doesn't implement Copy).
For Dimension this introduces a lot more Clone in the code, I'm not yet sure how much that will effect performance/ergonomics. I'm also not sure if there's a way around this.

The first couple commits are a bit of refactoring to split up the big style module. I recommend to review them separately.

Context

MDN: calc() in CSS
Feature request on Bevy's side: Support calc bevy::ui::Val bevyengine/bevy#5893

Feedback wanted

How bad is that we loose the Copy derives? Is this bad for performance? How about ergonomics?
Is this the best way to handle Dimension::Undefined and Dimension::Auto?
Is it better to define the trait operators for Dimension or &Dimension? Or perhaps both?

TimJentzsch · 2022-10-12T15:05:35Z

This is now ready for a first review pass.

The thing I'm most uncertain about is if it is worth it that we lose Copy on Dimension. That could be annoying when using the library.

Otherwise, I think this gives us some nice functionality and being able to use the +, -, * and / operators is a nice quality of life change.

src/style/calc_dimension.rs

src/style/dimension.rs

alice-i-cecile

The loss of Copy hurts ergonomically :(

Overall I think that this is a good, if somewhat implicit, approach to a gnarly problem. Thankfully the docs are very thorough: issues arising here are not going to be fun to debug.

The other approach we could use is a try_add approach, as we've been experimenting with in Bevy. I worry that will lead to excessive verbosity and inconsistent handling of edge cases though.

nicoburns · 2022-10-12T15:18:56Z

The thing I'm most uncertain about is if it is worth it that we lose Copy on Dimension. That could be annoying when using the library.

I'm not sure if this makes any sense, but we could use a slotmap for storing the actual calculation (requiring you to manually allocate it with taffy ahead of time), and then just include an id reference to it in Dimension similar to how we do nodes. It would be nice if we could avoid .clone() having to do a deep clone of the nested box structure.

alice-i-cecile · 2022-10-12T15:29:50Z

I'd also like to benchmark for performance regressions before merging this PR; those clones + the large enum variant are making me nervous. Ideally we would report memory usage in the benchmarks too.

nicoburns · 2022-10-12T15:23:40Z

src/flexbox.rs

-            let child_style = self.nodes[child].style;
+            let child_style = self.nodes[child].style.clone();


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

nicoburns · 2022-10-12T15:25:51Z

src/style/calc_dimension.rs

+    /// Multiply a [`Dimension`] with a constant.
+    Mul(Box<Dimension>, f32),
+
+    /// Divide a [`Dimension`] by a constant.
+    Div(Box<Dimension>, f32),


Why do Mul and Div not take Box<Dimension> for both arguments?

I tried to align with the CSS calc() here, which requires one side to be a <number>: https://developer.mozilla.org/en-US/docs/Web/CSS/calc#syntax

I think that I would prefer double boxed here: that seems like an arbitrary limitation.

Are there any use-cases for multiplying a Dimension::Points with Dimension::Percentage, for example?

I think multiplying with a simple factor is all you really need and then easier to write.

If there are use-cases I can change it though :)

Thinking about it, I don’t think there are any use cases for a non constant factor here. I’m CSS I can imagine you might sometimes want to do something like 20px * (2 + 8), but with Taffy this can easily be handled before passing the resultant integer in.

Although for that matter I find it hard to imagine a real use case for div and mul at all. Pixels and percentages can both easily be pre-scaled. I guess maybe you might want to scale the result of an addition or subtraction

Yes, I think there is still value for something like (10% + 10px) * 2 (although it's probably a very rare use-case.

nicoburns · 2022-10-12T15:26:35Z

src/style/calc_dimension.rs

+#[derive(Clone, PartialEq, Debug)]
+pub enum CalcDimension {
+    /// Add two [`Dimension`]s together.
+    Add(Box<Dimension>, Box<Dimension>),


Should this use a different enum rather than Dimension here? Not all Dimension variants will be valid in nested position (Undefined and Auto won't be)

Hmm, maybe. It will still work for Undefined and Auto, but the result will always be the same. I'd like to avoid duplicating some of the enum values, but I guess the current approach is also not ideal.

src/style/dimension_ops.rs

src/style/mod.rs

nicoburns · 2022-10-12T15:32:27Z

src/flexbox.rs

@@ -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)),


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?

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.

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.

nicoburns · 2022-10-12T15:32:52Z

src/flexbox.rs

@@ -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();


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

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

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.

nicoburns · 2022-10-13T11:15:52Z

src/style/dimension.rs

+    /// The actual result is then calculated when resolving the absolute values.
+    /// See [`Dimension::maybe_resolve`].
+    #[cfg(feature = "std")]
+    Calc(Box<CalcDimension>),


Another thought on this. Would it make sense to make this an Arc rather than a Box? That way we'd get cheaper clones...

Weibye

The implementation of Calc looks good and overall this is a useful feature!

My limited expertise makes it hard to comment on if losing copy is acceptable or not, or how that can be resolved.

Weibye · 2022-11-19T09:52:30Z

RELEASES.md

+- `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`.


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.

Weibye · 2022-11-19T09:56:47Z

src/style/calc_dimension.rs

+    Div(Dimension, f32),
+}
+
+impl MaybeResolve<Option<f32>> for CalcDimension {


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.

Weibye · 2022-11-19T10:05:18Z

src/resolve.rs

+            #[cfg(feature = "std")]
+            Dimension::Calc(calc) => calc.maybe_resolve(context),


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

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

TimJentzsch · 2022-11-22T12:41:22Z

After #246 is merged I'll try to rebase and take another stab at this.
Maybe I can get rid of a few more .clones.

TimJentzsch · 2023-04-08T11:08:59Z

I think at this point it will be easier to start a new PR instead of rebasing this one.
But this can probably still serve as a reference for the implementation :)

nicoburns · 2023-04-09T16:17:45Z

@TimJentzsch A couple of notes if you're thinking of taking another stab at this:

I think the size of the Calc variant of Dimension should be at most 64 bits (/usize sized). Concerns have already been expressed about even this size, but IMO calc is important enough a feature that it trumps concerns about memory usage. This implies that the entire Calc expression should be boxed in some way.
Dimension remaining Copy probably isn't super important, but Dimension remaining cheap to clone is. That implies the Calc variant being an Arc, Rc, or a slotmap key rather than a Box. Possibly we could use an opaque newtype around one of the above to allow to change the underlying implementation in future.
The order of operations is important. And the implementation should make sure to respect the standard order of operations for maths operators.
As well as basic mathematical operators, CSS also supports functions like min, max, clamp and round. We don't need to support these right away, but it would be a good if a design supported expanding to support them in future.

TimJentzsch mentioned this pull request Sep 29, 2022

Allow sum of percent and points in Dimension #229

Open

TimJentzsch added enhancement New feature or request breaking-change A change that breaks our public interface controversial This work requires a heightened standard of review due to implementation or design complexity labels Sep 29, 2022

jkelleyrtp force-pushed the main branch from d295d9b to c83608d Compare October 10, 2022 06:11

TimJentzsch added 7 commits October 11, 2022 18:28

Move style to module folder

5271156

Add CalcDimension to calculate dimensions

ec5803e

Fix errors caused by Dimension not being Copy

20710d2

Implement maybe_resolve for CalcDimension

ed1ee24

Move dimension to separate module

a8c4952

Implement numerical operations for Dimension

6da4f75

Simplify match statements for Mul and Div implementations

abdeb59

TimJentzsch force-pushed the 225-calc-expressions branch from ab82e88 to abdeb59 Compare October 11, 2022 16:29

TimJentzsch added 4 commits October 12, 2022 16:44

Improve documentation for calculated dimensions

86e8987

Fix errors with style not being copy in tests

aeb06df

Fix calculated dimensions breaking no-std

675623d

Add value calculation to release notes

fa28a28

TimJentzsch requested review from alice-i-cecile, Weibye and jkelleyrtp October 12, 2022 15:03

TimJentzsch marked this pull request as ready for review October 12, 2022 15:03

alice-i-cecile reviewed Oct 12, 2022

View reviewed changes

src/style/calc_dimension.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Oct 12, 2022

View reviewed changes

src/style/dimension.rs Show resolved Hide resolved

alice-i-cecile reviewed Oct 12, 2022

View reviewed changes

Improve documentation for CalcDimension

d6aeabf

alice-i-cecile mentioned this pull request Oct 12, 2022

Taffy points actually seem to represent pixel sizes #236

Closed

nicoburns requested changes Oct 12, 2022

View reviewed changes

TimJentzsch added 2 commits October 12, 2022 17:52

Don't clone Dimensions in FlexboxLayout methods

d234d35

Box CalcDimension instead of operation paramters

b87f83d

nicoburns reviewed Oct 13, 2022

View reviewed changes

Weibye reviewed Nov 19, 2022

View reviewed changes

nicoburns added this to the 0.4 milestone Jan 8, 2023

nicoburns removed this from the 0.4 "Make taffy easier to embed" milestone Mar 20, 2023

TimJentzsch closed this Apr 8, 2023

TimJentzsch mentioned this pull request Apr 9, 2023

Support Calc val #225

Open

TimJentzsch mentioned this pull request Jun 17, 2023

Don't take self by value if we don't have to #498

Closed

kokoISnoTarget mentioned this pull request Jun 21, 2024

Add CSS calc #668

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add calculated expressions to `Dimension` #232

Add calculated expressions to `Dimension` #232

TimJentzsch commented Sep 29, 2022 •

edited

Loading

TimJentzsch commented Oct 12, 2022

alice-i-cecile left a comment

nicoburns commented Oct 12, 2022 •

edited

Loading

alice-i-cecile commented Oct 12, 2022

nicoburns Oct 12, 2022

nicoburns Oct 12, 2022 •

edited

Loading

TimJentzsch Oct 12, 2022

alice-i-cecile Oct 12, 2022

TimJentzsch Oct 12, 2022

nicoburns Oct 12, 2022

TimJentzsch Nov 22, 2022

nicoburns Oct 12, 2022

TimJentzsch Oct 12, 2022

nicoburns Oct 12, 2022

TimJentzsch Oct 12, 2022

nicoburns Oct 12, 2022

nicoburns Oct 12, 2022

TimJentzsch Oct 12, 2022

nicoburns Oct 12, 2022

nicoburns Oct 13, 2022

Weibye left a comment

Weibye Nov 19, 2022

Weibye Nov 19, 2022

Weibye Nov 19, 2022

TimJentzsch Nov 22, 2022

TimJentzsch commented Nov 22, 2022 •

edited

Loading

TimJentzsch commented Apr 8, 2023

nicoburns commented Apr 9, 2023 •

edited

Loading

		let child_style = self.nodes[child].style;
		let child_style = self.nodes[child].style.clone();

		#[cfg(feature = "std")]
		Dimension::Calc(calc) => calc.maybe_resolve(context),

Add calculated expressions to Dimension #232

Add calculated expressions to Dimension #232

Conversation

TimJentzsch commented Sep 29, 2022 • edited Loading

Objective

Solution

Context

Feedback wanted

TimJentzsch commented Oct 12, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

nicoburns commented Oct 12, 2022 • edited Loading

alice-i-cecile commented Oct 12, 2022

Choose a reason for hiding this comment

nicoburns Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Weibye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimJentzsch commented Nov 22, 2022 • edited Loading

TimJentzsch commented Apr 8, 2023

nicoburns commented Apr 9, 2023 • edited Loading

Add calculated expressions to `Dimension` #232

Add calculated expressions to `Dimension` #232

TimJentzsch commented Sep 29, 2022 •

edited

Loading

nicoburns commented Oct 12, 2022 •

edited

Loading

nicoburns Oct 12, 2022 •

edited

Loading

TimJentzsch commented Nov 22, 2022 •

edited

Loading

nicoburns commented Apr 9, 2023 •

edited

Loading