WIP: Refactor the LayoutTree trait

[nicoburns]

Objective

Fix #28 by adding child measurement functions to the LayoutTree trait.

• Add measure_child_size and perform_child_layout methods to the LayoutTree trait, and trampoline all calls to size children through the LayoutTree trait (fixes Support multiple layout algorithms #28)
• Remove parent method (unused - we only ever traverse down the tree)
• Remove mark_dirty method (unused - layout algorithms never need to mark nodes as dirty)
• Remove layout method (unused - we only ever use layout_mut)
• Remove the is_childless method (no need to make implementors implement another method when we can just use child_count() == 0)
• Refactor such that impl LayoutTree represents a single node rather an entire hierarchy. Add child_style and child_layout_mut methods.
• Make ChildId an associated generic type
• Remove needs_measure and measure_child methods (if you're implementing LayoutTree then you don't need measure_funcs or the Leaf layout more).

Remaining Issues

• Caching. Currently the LayoutTree trait still has a cache_mut method, but the standalone Flexbox and CSS Grid implementations don't use it (in the high-level API caching is implemented in the generic method that switches between the modes). There are a couple of options here:

  • We could remove the cache_mut method from the trait. This simplifies the trait, but requires that implementors implement caching themselves. This is entirely possible with the current caching strategy, and we could provide an standalone cache type that encapsulates the storage and actual cache logic for people who want to use it. But it does preclude things like storing more of the internal state of the Flexbox/Grid algorithms in future (although we could of course just reintroduce a cache method to the trait at that point).
  • Alternatively we could move caching into the individual algorithms. This adds complexity, but might be a good idea from a perspective of having maximum information available when choosing what to cache. However, it would be quite hard to design a general purpose caching strategy. For example, in the Taffy struct where we are in control of the whole tree it doesn't make sense for the flexbox implementation to cache the sizes of it's children, because those children can cache their own sizes and this avoids us doubling up on caches. I suppose perhaps we could have nodes only cache their children, but that relies on making sure we have stable node references (reordering children shouldn't end up with them getting each other's cached values!)

  I'm definitely leaning towards removing caching entirely for those implementing LayoutTree themselves for the time being (on the basis that it's better to have no abstraction than a leaky abstraction), but I'd value other perspectives on this.

• Performance. I'm currently seeing a 10%-16% decrease in performance from this PR. I'm not sure what's causing the slowdown, but I feel like I'd be willing to accept this for the increased flexibility (thoughts?). For the "deep tree" benchmarks we're still quite a bit faster than Taffy 0.2 due to earlier optimisations that have already been merged to main (for the "wide tree" benchmarks we seem to be a bit slower - I'm not sure why, but I'm not going to worry too much about this as a single node with 1000 direct (non-absolutely positioned) children doesn't seem like a very likely use case).

• Children/Child methods. There are a few related issues here:

  • The ChildIter associated type and children method aren't strictly necessary. We could make do with child_count and child. This would simplify the implementation of the trait, but would make things slightly less efficient. In some cases we're using child_count and child anyway to get around borrow checking restrictions.
  • The ChildId associated type and child methods are not strictly necessary. We could just always refer to children by index. Again, this would be less efficient (for some storage methods that actually have a meaningful child key - like Taffy's own storage), but would simplify the trait implementation.

• Reference Nesting. The layout algorithms currently take &mut impl LayoutTree like they did before this PR. However, prior to this PR the impl LayoutTree was an instance of the Taffy struct (so there was only one level of indirection), but in the new implementation impl LayoutTree is an instance of TaffyNodeRef<'a>, which is a small struct which itself contains an &mut Taffy and a node id (so there are now two levels of indirection). It's possible to remove the outer layer by making the layout algorithms take impl LayoutTree instead of &mut LayoutTree, and I've implemented this in https://github.com/nicoburns/taffy/commits/reborrow-layout-tree (see most recent 2 commits).

  In theory this should be more efficient (assuming that struct implementing LayoutTree are as small as the two I've created so far). However:

  • It requires implementers of LayoutTree to implement an additional reborrow method
  • It didn't seem to make much difference in benchmarks.

  I'm currently leaning towards leaving this as it is.

• Naming. The trait is called LayoutTree, and that makes sense in the current version of Taffy because it represents a whole tree. But with the changes in this PR it only represents a single node in the tree (and it's direct children). As such, I'm thinking it makes sense to rename the trait. Some options:

  • LayoutNode
  • LayoutNodeRef
  • Node
  • NodeRef

  The "ref" bit is because in both the implementations of the new LayoutTree I have written so far (the one in this PR, and the one in iced_taffy), the type implementing LayoutTree has ended up being a wrapper around a reference back to the

Context

See #28 (comment)
Blocked on #325, which is blocked on #320.

New LayoutTree trait

pub trait LayoutTree {
    /// Type of an id that represents a child of the current node
    /// This can be a usize if you are storing children in a vector
    type ChildId: Copy + PartialEq + Debug;

    /// Type representing an iterator of the children of a node
    type ChildIter<'a>: Iterator<Item = Self::ChildId>
    where
        Self: 'a;

    // Current node methods

    /// Get the [`Style`] for this Node.
    fn style(&self) -> &Style;

    /// Modify the node's output layout
    fn layout_mut(&mut self) -> &mut Layout;

    // Child methods

    /// Get the list of children IDs for the given node
    fn children(&self) -> Self::ChildIter<'_>;

    /// Get the number of children for the given node
    fn child_count(&self) -> usize;

    /// Get a specific child of a node, where the index represents the nth child
    fn child(&self, index: usize) -> Self::ChildId;

    /// Get the [`Style`] for this child.
    fn child_style(&self, child_node_id: Self::ChildId) -> &Style;

    /// Modify the child's output layout
    fn child_layout_mut(&mut self, child_node_id: Self::ChildId) -> &mut Layout;

    /// Compute the size of the node given the specified constraints
    fn measure_child_size(
        &mut self,
        child_node_id: Self::ChildId,
        known_dimensions: Size<Option<f32>>,
        parent_size: Size<Option<f32>>,
        available_space: Size<AvailableSpace>,
        sizing_mode: SizingMode,
    ) -> Size<f32>;

    /// Perform a full layout on the node given the specified constraints
    fn perform_child_layout(
        &mut self,
        child_node_id: Self::ChildId,
        known_dimensions: Size<Option<f32>>,
        parent_size: Size<Option<f32>>,
        available_space: Size<AvailableSpace>,
        sizing_mode: SizingMode,
    ) -> SizeAndBaselines;

    /// Perform a hidden layout (mark the node as invisible)
    fn perform_child_hidden_layout(&mut self, child_node_id: Self::ChildId, order: u32);
}

[nicoburns]

I've made a proof of concept integration with Iced based on this PR (see: https://github.com/nicoburns/iced_taffy). I had to change quite a few bits to get it to work, and it's still not perfect (but some of that needs to be fixed on the Iced side), but it's super nice to see the result of the layout in visual form. And it was also nice to confirm that Taffy's grid implementation is usable for real world UIs.

[Weibye]

I do like the changes, it does feel like taffy is becoming more modular and opening up to more different ways of using it as an end user.

If I understand it correctly, so far there is no changes to the end user yet, in terms of how they interact with taffy as a library? As in you would still be running the layouting on the full tree at once and not node-by-node?

[Weibye]

We should have some examples showing how (and when) to use the various methods on the trait, especially if it will be up to the end user to run the layout on each of the node (as opposed to calling perform_layout on the tree as a whole)

[Weibye]

That may not need to be in this PR, but it would be useful when reviewing to see the impact of the refactor on user-facing code

[nicoburns]

We should have some examples showing how (and when) to use the various methods on the trait, especially if it will be up to the end user to run the layout on each of the node

Agreed. Also, I've just noticed that this comment (about stable indexes) is no longer accurate. I want to remove a few more methods from the LayoutTree trait first though!

[Weibye]

I assume horizontal baselines are only relevant when supporting vertical text directions?

[nicoburns]

Ah, well I'd phrase it as "vertical baselines are only relevant when supporting vertical text directions", but I suspect that makes the short answer "yes".

[nicoburns]

If I understand it correctly, so far there is no changes to the end user yet, in terms of how they interact with taffy as a library? As in you would still be running the layouting on the full tree at once and not node-by-node?

That depends. There are effectively two ways to use taffy. Either:

• You can use the Taffy struct: add nodes with styles and measure_funcs and then call compute_layout to compute the entire tree. That API is unchanged by this PR (although the implementation has changed under the hood).
• Or you can use your own node storage, implement the LayoutTree trait and call the algorithm entry points directly. That has API is changed dramaticly by this PR:
  • There are now perform_child_layout and measure_child_size methods on the LayoutTree trait, which you are expected to implement (effectively making this api node-by-node). This could call straight back into Taffy, but might also call into other layout algorithms, or proxy the call into Taffy via a method in your framework (like https://github.com/nicoburns/iced_taffy does)
  • The LayoutTree implementation is now only expected to provide access to styles/layout/cache for a single node and it's immediate descendants at once. Recursing down the tree works by calling perform_child_layout and measure_child_size which are expected to create a new instance of impl LayoutTree (or mutate the existing one) and call back into taffy with that new instance.

[nicoburns]

@alice-i-cecile @Weibye @geom3trik @jkelleyrtp @Demonthos @TimJentzsch

I'm not currently intending for this to be merged until after a 0.3 release (so 0.3 would contain CSS Grid, and this change would go into a 0.4 release). However it is now at a point that I consider it ready for review, and as this represents a significant change to Taffy's API (albeit a hitherto little-used part of it), I'm keen to get as many eyes on it as possible and to give people advance warning so that there is plenty of time for feedback and revisions.

If you are reviewing this:

• You may wish to read Support multiple layout algorithms #28 (comment) as background / an overview of the design.
• You should probably concentrate on the changes to tree.rs, node.rs and lib.rs. The changes in tree.rs and lib.rs are the big API changes, and the changesnode.rs allow you to see how this has impacted our own internal implementation of that interface. The rest of the changes are mostly mechanical changes to make our code work with the new interface.
• You may wish to refer to https://github.com/nicoburns/iced_taffy/blob/main/src/grid.rs as a second example of an implementation of the new trait (it implements a Taffy-based Grid widget for the Iced GUI framework). This implementation is quite different to the internal one in Taffy, and demonstrates how the new version of the LayoutTree trait can be implemented without having access to the entire layout tree at once.
• In addition to the code changes, any thoughts/opinions/input on the issues in the "remaining issues" section of the PR description above would be very helpful!

[ealmloff]

Other than needing more documentation around when certain functions should be called, I think the API here looks good. The performance penalty is unfortunate, but I think it is worth the tradeoff. Dioxus' native renderer uses a tree similar to Taffy's tree, so the child ids help the performance there as well. I think Leaving the caching up to the implementor makes sense. It gives them more flexibility to allow caching of different steps in whatever internal layout algorithms they implement.

[nicoburns]

Closing in favour of smaller separate PRs:

• Remove unused methods from the LayoutTree trait #425
• Use struct NodeId(u64) instead of slotmap::DefaultKey in all public APIs #426
• Trampoline child measurement/layout through the LayoutTree trait #427
• Remove cache_mut, needs_measure and measure_node methods from LayoutTree trait #431
• Move cache logic to own module #432
• Make low-level layout algorithm functions public #434

The new set of PRs differs from this PR in two ways:

• It uses a newtype around u64 for node ids instead of a generic type
• It doesn't include the refactor such that impl LayoutTree represents a single node rather an entire hierarchy as this currently has perf concerns, and may also make implement block/subgrid layout awkward.