taffy icon indicating copy to clipboard operation
taffy copied to clipboard

Add calculated expressions to `Dimension`

Open TimJentzsch opened this issue 3 years ago • 3 comments

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

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 avatar Sep 29 '22 18:09 TimJentzsch

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.

TimJentzsch avatar Oct 12 '22 15:10 TimJentzsch

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.

nicoburns avatar Oct 12 '22 15:10 nicoburns

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.

alice-i-cecile avatar Oct 12 '22 15:10 alice-i-cecile

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 avatar Nov 22 '22 12:11 TimJentzsch

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 :)

TimJentzsch avatar Apr 08 '23 11:04 TimJentzsch

@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.

nicoburns avatar Apr 09 '23 16:04 nicoburns