`MeasureFunc` is confusing and hard to use

andriyDev commented 2 years ago

I'm new to Rust so this might be a basic problem. I couldn't find an example of MeasureFuncs in stretch.

I'm trying to create a text leaf node. I wrote a font loading mechanism which returns Font, and I wrote a basic text layout algorithm which takes a borrow to Font (lets call this function layout(in_bounds: Size<Number>, f: &Font) -> Size<f32>, where the returned vector is the bounds of the text. It also takes the bounds where we should layout text into). I'd like to be able to call this layout function inside the MeasureFunc, but no matter what I try it says font does not live long enough. borrowed value does not live long enough.

let mut stretch = stretch2::node::Stretch::new();
let font = load_font(...);

let text_node = stretch.new_leaf(
  Style { ..Default::default() },
  MeasureFunc::Boxed(Box::new(|size|{
    layout(size, &f)
  }))).unwrap();
stretch.compute_layout(text_node, Size{ width: Number::Defined(500), height: Number::Undefined }).unwrap();

I tried boxing and Rc-ing the font with no luck.

let mut stretch = stretch::node::Stretch::new();
let font = load_font(...);

let rc_font = Rc::new(font);

let text_node = stretch.new_leaf(
  Style { ..Default::default() },
  MeasureFunc::BoxedBox::new(|size|{
    layout(size, rc_font.as_ref())
  }))).unwrap();
stretch.compute_layout(text_node, Size{ width: Number::Defined(500), height: Number::Undefined }).unwrap();

The reason I want this is because the text layout should change based on the size of the container. What's the proper way to do this? An example of this in the future would be immensely useful!

TimJentzsch commented 2 years ago

Thank you for opening this issue!

May I suggest to use syntax highlighting in your code blocks, like this?

```rust
// Your code
```

This makes it easier to read your examples. Thank you!

andriyDev commented 2 years ago

Fixed, sorry about that!

TimJentzsch commented 2 years ago

In your use-case, would it be possible to clone the Font? That would eliminate the reference and the lifetime errors.

I definitely agree that we need more examples and more documentation in general. I think that will be one of our focus points going forward :)

andriyDev commented 2 years ago

I'm not really sure where to clone though. I can clone in the MeasureFunc, but that still complains about font not living long enough, which makes sense since it still has to refer to the original value to clone.

let mut stretch = stretch2::node::Stretch::new();
let font = load_font(...);

let text_node = stretch.new_leaf(
  Style { ..Default::default() },
  MeasureFunc::Boxed(Box::new(|size|{
    layout(size, f.clone())
  }))).unwrap();
stretch.compute_layout(text_node, Size{ width: Number::Defined(500), height: Number::Undefined }).unwrap();

andriyDev commented 2 years ago

Here's my actual super-simple example code:

use stretch2::{
  node::MeasureFunc,
  prelude::{Number, Size, Style},
};

#[derive(Clone)]
struct Font {
  x: f32,
  y: f32,
}

fn load_font() -> Font {
  Font { x: 27.0, y: 50.0 }
}

fn layout(_: Size<Number>, f: &Font) -> Size<f32> {
  Size { width: f.x, height: f.y }
}

fn main() {
  let mut stretch = stretch2::node::Stretch::new();
  let font = load_font();

  let text_node = stretch
    .new_leaf(
      Style { ..Default::default() },
      MeasureFunc::Boxed(Box::new(|size| layout(size, &font))),
    )
    .unwrap();
  stretch
    .compute_layout(
      text_node,
      Size { width: Number::Defined(500.0), height: Number::Undefined },
    )
    .unwrap();
}

You can just immediately set this into a new project with stretch2 installed.

TimJentzsch commented 2 years ago

For me the following compiles:

use stretch2::{
    node::MeasureFunc,
    prelude::{Number, Size, Style},
};

#[derive(Clone)]
struct Font {
    x: f32,
    y: f32,
}

fn load_font() -> Font {
    Font { x: 27.0, y: 50.0 }
}

fn layout(_: Size<Number>, f: &Font) -> Size<f32> {
    Size {
        width: f.x,
        height: f.y,
    }
}

fn main() {
    let mut stretch = stretch2::node::Stretch::new();

    let text_node = stretch
        .new_leaf(
            Style {
                ..Default::default()
            },
            MeasureFunc::Boxed(Box::new(|size| layout(size, &load_font()))),
        )
        .unwrap();
    stretch
        .compute_layout(
            text_node,
            Size {
                width: Number::Defined(500.0),
                height: Number::Undefined,
            },
        )
        .unwrap();
}

or if you don't want the boxing you could also do:

MeasureFunc::Raw(|size| layout(size, &load_font()))

Would that work for you?

andriyDev commented 2 years ago

Not really... That means that every time my layout refreshes, the font needs to be reloaded. This can be pretty expensive!

twop commented 2 years ago

I recently integrated text measurements and working with the current API was quite confusing.

Specifically there are two challenges with it

Challenges

1. External state that needs to be captured in the closure

Usually you have some external means to measure a leaf node (such as text), thus accessing that "external" thing (such as parsed Font) requires at least an Rc and some "magic" to move it into the measure function.

  let some_data = Rc::new(5.);
  let cloned_data_pointer = some_data.clone(); // <-- clone in order to move

  let leaf = stretch.new_leaf(
      Style {
          ..Default::default()
      },
      stretch2::node::MeasureFunc::Boxed(Box::new(move |size| {
          let external_data = *cloned_data_pointer; // <-- accessing external state

          Size {
              width: 10. * external_data, // <-- use it to calc result
              height: 10. * external_data,
          }
      })),
  )?;

2. You need to know what to measure

A naive way to solve it: clone() needed data for measuring (such as text) or wrap with Rc.

let text = "I need to be measured".to_string();

let cloned_text = text.clone(); // <-- clone data in order to "move" below

let leaf = stretch.new_leaf(
    Style {
        ..Default::default()
    },
    stretch2::node::MeasureFunc::Boxed(Box::new(move |size| {
        let bytes_cnt = cloned_text.len() as f32;

        Size {
            width: 10. * bytes_cnt,
            height: 10.,
        }
    })),
)?;

Proposed solutions

Here are my thoughts how to approach it.

We know exactly when measurements take place, e.g. in compute_layout call.
That means that we can borrow rather than move the external state for the duration of compute_layout.
We don't know the type of "Measure context object", thus needs to be parametrized by T.
It would be awesome if we can attach metadata to the leaf node and then pass it to MeasureFunc. I think that might be a bit of a stretch (pun intended) in terms of API complexity because you have to also parametrize it with type U.
If the point above is "too much", there is an alternative way to solve this: pass Node to the MeasureFunc, in that case the library user is responsible for keeping track of Map<Node, MyNodeData>.

Solution 1: Borrowed context + storing custom metadata

struct Stretch<TMeasureCtx, TLeafData> {...}

enum MeasureFunc<TMeasureCtx, TLeafData> {
    Raw(fn(Size<Number>) -> Size<f32>),
    Boxed(sys::Box<dyn Fn(&TMeasureCtx, &TLeafData, Size<Number>) -> Size<f32>>),
}

fn new_leaf(&mut self, style: Style, measure: MeasureFunc<TMeasureCtx, TLeafData>, data: TLeafData) {...}

Note that the library needs to store TLeafData internally.

Solution 2: Borrowed context + passing `Node` as an argument

struct Stretch<TMeasureCtx> {...}

enum MeasureFunc<TMeasureCtx> {
    Raw(fn(Size<Number>) -> Size<f32>),
    Boxed(sys::Box<dyn Fn(&TMeasureCtx, Node, Size<Number>) -> Size<f32>>),
}

Note that Node is passed as an argument to identify what is actually being measured. Note2 that I omitted the signature change for Raw variant, but the idea is applicable there too.

In both of these cases the signature of compute_layot is

pub fn compute_layout(&mut self, node: Node, size: Size<Number>, measure: &TMeasureCtx) -> Result<(), Error> {

This avoids move and other "shenanigans" needed to measure leaf nodes today

P.S.

Happy to contribute this change and or discuss it further

twop commented 2 years ago

Oh, forgot to mention. I think both of the proposed solutions are technically "breaking" changes, thus it might be a good opportunity to take advantage of the new project name and introduce them now.

There might be an ergonomic way to keep the existing compute_layout signature alongside the one with measure context (although the function names have to be different, right?). This might be beneficial for examples or in other simple cases.

Something like (pseudocode)

struct NoMeasure {}

struct Stretch<T> {}

type SimpleStretch = Stretch<NoMeasure>;

impl SimpleStretch {
    fn new() -> Self {
        Self { d: PhantomData }
    }

    pub fn compute_layout(&mut self, node: Node, size: Size<Number>) -> Result<(), Error> {
        // calls internally the other version
        compute_layout_with_measure(other_args, NoMeasure {})
    }
}

Also both of the solutions are likely to be beneficial for performance, either in terms of memory consumptions and/or the amount of dynamic dispatches to call the measure functions. Partially because in most cases Raw variant is probably going to be enough

alice-i-cecile commented 2 years ago

I think both of the proposed solutions are technically "breaking" changes, thus it might be a good opportunity to take advantage of the new project name and introduce them now.

I intend to keep introducing breaking changes for a long time coming :) That said, I'd like the very initial release of sprawl to be as non-breaking as we can, to enable easy migration.

twop commented 2 years ago

to be as non-breaking as we can, to enable easy migration.

One potential solution to that is to add a variant(s) to measure functions vs replacing them.

Like so

enum MeasureFunc<TMeasureCtx = NoMeasure, TLeafData = NoLeafData> {
    Raw(fn(Size<Number>) -> Size<f32>),
    Boxed(sys::Box<dyn Fn(Size<Number>) -> Size<f32>>),

    // just add a new variant
    WithCtx(fn(&TMeasureCtx, &TLeafData, Size<Number>) -> Size<f32>)
    // I think that the Boxed version is not actually needed
}

Note, that I'm horrible at naming things, I bet there is a better name than WithCtx

I'm not 100% sure but with this approach the existing code that uses the library will compile, right?

alice-i-cecile commented 2 years ago

@twop now that things are a bit cleaner I'd love to see an implementation of these ideas.

twop commented 2 years ago

Sure, incoming soon

jkelleyrtp commented 2 years ago

I pattern I use a lot in my projects is passing state into methods rather than storing them in the structures directly.

So we could have compute_measured_layout that passes a dyn callback. This would save us the hassle of Send/Sync and also allow temporaries and borrowed data with no overhead.

jkelleyrtp commented 2 years ago

I would like to tackle removing measurefunc from being built into Taffy, but rather passed in via a more specific compute_layout.

This method would be generic over function so we get 100% performance with no dynamic dispatch.

The function would look something like:

fn compute_measured_layout(&mut self, func: impl Fn(Node) -> Option<Size>);

If the provided measure func can produce a size for this node (IE it needs to be handled specially, like a paragraph), then Taffy would use that. If no size is returned, then taffy just ignores it. It would be up to the implementor to look up the right UI node from the Taffy node, measure it, and optionally return a size.

The current callback API is very reminiscent of JavaScript. However, it does not translate into Rust and trying to maintain the JS API will likely lead to headache.

Edit: We should probably pass some context to the function like its NodeData so the implementor can find the original UI node during the measure process.

alice-i-cecile commented 2 years ago

I'm on board; I'm looking forward to seeing what this looks like in practice.

nicoburns commented 2 years ago

A few notes from my work on CSS grid that relates to MeasureFunc:

For Grid, we will want to be able to pass a "sizing constraint" (MinContent or MaxContent) as an alternative to a size (or in addition to an indefinite size).
CSS calls the result of it's equivalent of a "MeasureFunc" an item's "intrinsic size" when sized under such constraints
Finally, perhaps obvious to some, but MeasureFuncs only ever apply to leaf nodes and are ignored on Flex nodes. We may wish to make this clearer in the API and/or docs!

nicoburns commented 1 year ago

Closing this as completed as we now:

Support borrowing external context in measure functions
Have an example (https://github.com/DioxusLabs/taffy/blob/main/examples/measure.rs) of how to use measure functions which demonstrates how to do this.

DioxusLabs / taffy