Abstract flexbox over LayoutTree by jkelleyrtp · Pull Request #202 · DioxusLabs/taffy

jkelleyrtp · 2022-07-18T02:11:00Z

This PR makes the flexbox algorithm generic over a LayoutTree trait.

At the time of filing this PR, the LayoutTree trait mostly abstracts over the broadest set of functions that the Taffy struct satisfies. I don't know or think this is the best and most general solution to flexboxing: Taffy internally does caching of calculations that a user's LayoutTree shouldn't really be concerned about. Perhaps some of the state - like caching - could remain in a Taffy struct, but this level of specificity intrinsically ties a user's UI system to Taffy due to dependent data structures.

Generally, since Taffy uses its own types, most implementations will be tied to Taffy anyways, so perhaps this linking of user code to Taffy is not a big deal.

Also - this trait supplies a method that returns a FlexboxLayout (not the final layout, despite the confusing name) which might not be exactly a trait implementation would return the "current state" of the DOM Node. For instance, in Dioxus, we are using the CSS Cascade to supply the "current state" of the DOM node's style. I think before publishing out these changes, it might be worth further abstracting the trait to supply single-level Taffy attributes. Perhaps a "TaffyAttribute" enum could be supplied so use cases like Dioxus' cascading CSS could be implemented efficiently.

Closes #182

Swift bindings

Implement js bindings

twop · 2022-07-19T15:20:48Z

src/tree.rs

+    fn style(&self, node: Node) -> &FlexboxLayout;
+
+    /// Get the node's output "Final Layout"
+    fn layout(&self, node: Node) -> &Layout;


Unsolicited thoughts.

I was thinking about the change and would love to share some thoughts:

If I understand correctly this approach aims to solve the fundamental decision with Taffy: who owns the notion of visual hierarchy? From my experience, you kinda have to have your own notion of visual hierarchy and then somehow keep your hierarchy and Taffy's in sync, this approach might help with that.

As a side benefit of this approach you can have dynamic FlexboxLayout based on some external context (like in CSS). Although I'm curious if you can mark only the root node with mark_dirty or go through the entire subtree and call mark_dirty on each node recursively if you want to "cascade" new font size through the entire hierarchy (example: body {font-size: 14 -> 16}).

Assuming that you want to implement your own LayoutTree, I wonder what kind of restrictions need to be applied to Node? It feels like SlotMap is very restrictive, but feels "right" for this use case, so I'm curious what a custom implementation might look like in regards to generating Nodes

It feels to me that the methods in this trait can be roughly split into two categories: functions that deal with things that created outside of the library, and functions that deal with things that created inside of the library. Let me elaborate a bit, parent -> child relationships and FlexboxLayout are created and managed in user's code, but caching and Layout seem to be originated and used inside the library itself. So I wonder if this trait perhaps should be split in two: layout hierarchy and layout computation, note that "layout computation" can potentially be in the "core" Taffy and not a trait.

As a general thought: this approach seems to introduce more cognitive complexity and I wonder if the benefits out-weight the cost for the broader audience. For example: for Dioxus use-case this trait saves X amount of memory and makes it Y% faster, but will these benefits to be as pronounced for the broader audience, if yes, what would it take to unleash the full potential?

Thanks for reading!

Consider thoughts of this nature solicited by default! This is great feedback.

nicoburns · 2022-07-22T19:41:21Z

src/tree.rs

+    // for reference, CSS cascades require context, and storing a full flexbox layout for each node could be inefficient
+    //
+    /// Get the [`FlexboxLayout`] for this Node.
+    fn style(&self, node: Node) -> &FlexboxLayout;


I would like to suggest that FlexboxLayout is renamed to FlexboxStyle: the idea being to establish a naming convention that *Style is input and *Layout is output. FlexboxLayout is already in a file called style.rs and returned from a method called style, so this seems to improve naming consistency.

This makes a lot of sense, it is what it should have been all along.

I'm on board with that. IMO we should do it in a small seperate PR though.

I have thought a bit about this.

CSS cascade is a bit on the complex side, but when you need it, you need it. That means that there shouldn't be 2 implementations of it: one for the Taffy properties (the flexbox data) and one for the rest of the UI properties (color, font, whatever). The former should be derived from the latter.

A proper CSS implementation probably shouldn't store the full properties for each node but some kind of diff to its parent, to let the whole cascade (with selectors, etc.) work properly and not to take huge amount of memory.

So I like this style() method and I approve the FlexboxStyle name.

and one for the rest of the UI properties (color, font, whatever).

This is out of scope for this library :) I'd like to keep this focused on pure layout, and let consumers (like bevy_ui or Dioxus) define their own tools for working with these sort of non-layout properties.

Yes it is, but my point is that the user will have to implement it anyway, so it probably should be reused; so having a style() method to ask the user how it resolved a particular Node's properties looks good to me.

nicoburns · 2022-07-22T19:44:06Z

Is it worth thinking about how this will interact with supporting multiple layout algorithms? From some preliminary reading of the code, I believe we will probably want to split out the Taffy struct into something which simply holds data (i.e. something that implements LayoutTree here, and an entirely separate struct/module (possibly stateless) that implements the flexbox algorithm.

EDIT: I've been working with this code more closely, and I've realised that this PR more or less already implements this.

geom3trik · 2022-07-23T23:11:45Z

On the subject of supporting multiple layout algorithms, since I know that Morphorm is being looked at as a candidate, this isn't too far away from what Morphorm does with its Hierarchy trait. The main difference being that Morphorm requires the user to provide iterators over the whole tree while this approach simply queries nodes for their parent and children, which I think is much better. I'm going to see if I can adapt Morphorm to work in the same way. The tricky part for integrating multiple algorithms is going to be the mismatch between the style properties that each is using I think.

alice-i-cecile · 2022-07-23T23:28:27Z

The tricky part for integrating multiple algorithms is going to be the mismatch between the style properties that each is using I think.

Agreed. We should start with an MVP where we can't mix styles in the same tree. Then once that's built and we have a couple candidates, we can start looking to Flutter to figure out if there's a good interface by which we can let them talk.

bestouff · 2022-08-23T15:16:55Z

src/tree.rs

+    /// Taffy caches results of computations for nodes to not need re-caculating nodes it already knows
+    ///
+    /// When a node does not have a cache, Taffy will layout that node appropriately.
+    fn primary_cache(&mut self, node: Node) -> &mut Option<Cache>;


~~I do not understand how this cache works (when is it invalidated, etc.).~~

Nevermind I get it; all nodes have to contain the space for 2 Cache structs.

bestouff · 2022-08-23T15:37:18Z

src/tree.rs

+/// remains the same between re-layouts.
+pub trait LayoutTree {
+    /// Get the list of children IDs for the given node
+    fn children(&self, node: Node) -> &[Node];


Maybe here you restrict too much the type of the underlying children set (which now has to be a Vec or something like that) and it's be better to have something like

fn children(&self, node: Node) -> impl Iterator<Item=Node>;

(which also has a trivial implementation). Now if the LayoutTree implementor uses e.g. a BTreeMap as the underlying storage it still can trivially implement children().

Definitely agree with this :) This sort of flexibility is really nice for public traits.

We should probably use IntoIterator not Iterator though; as that's the trait that's implemented directly on collections.

Wouldn't IntoIterator consume the collection (into_iter() takes self by value) ? I think we just want using iter() here.

bestouff · 2022-08-23T16:59:42Z

[...] I know that Morphorm is being looked at as a candidate [...]

You made me have a look at it. It's really cool (even if the API is a bit clunky at times) but in my small test it doesn't implement wrapping; wouldn't that be required for Taffy to use it ?

bestouff · 2022-08-23T19:21:05Z

Note for later: TaffyECS should probably be conditionally compiled, so somebody that only needs the LayoutTree trait could get rid of unneeded dependencies (e.g. slotmap). But this doesn't need to be in that PR.

nicoburns · 2022-08-23T19:49:29Z

[...] I know that Morphorm is being looked at as a candidate [...]

You made me have a look at it. It's really cool (even if the API is a bit clunky at times) but in my small test it doesn't implement wrapping; wouldn't that be required for Taffy to use it ?

I also looked into this recently, and it does seem cool. I think Taffy would use it as another alternative algorithm (e.g. a Display::Morphorm mode) rather than an implementation detail for the existing one. So lacking features wouldn't preclude it. It's earlier, but I think taffy aims to be somewhat of a "one stop shop" for layout.

Note for later: TaffyECS should probably be conditionally compiled, so somebody that only needs the LayoutTree trait could get rid of unneeded dependencies (e.g. slotmap). But this doesn't need to be in that PR.

This makes sense.

nicoburns · 2022-10-10T22:28:21Z

@jkelleyrtp What's the status of this PR? I've been basing my work off of it, and I'm concerned that it's now drifting further and further from main over time. My own take is that the changes in this PR are broadly good, and that while we might want to further tweak the LayoutTree trait abstraction, it might be best to get this merged and iterate on it later.

(side note: I believe that the recent force-push to main has broken existing pull requests and that they will need to be rebased on top of the latest main).

alice-i-cecile · 2022-10-10T22:36:45Z

My own take is that the changes in this PR are broadly good, and that while we might want to further tweak the LayoutTree trait abstraction, it might be best to get this merged and iterate on it later.

This is my position as well :) I'd like to avoid long-lived branches, especially for foundational data structures.

geom3trik · 2022-10-10T23:06:47Z

You made me have a look at it. It's really cool (even if the API is a bit clunky at times) but in my small test it doesn't implement wrapping; wouldn't that be required for Taffy to use it ?

Somehow missed this before. You're right that Morphorm doesn't currently support wrapping but it's something I'm planning to add for a rewrite that I'm working on at the moment (there's already a working but not very well tested implementation).

Once I've finished this rewrite then I think Morphorm will be in a better position to be integrated with taffy in some way if that's still desired.

alice-i-cecile · 2022-10-10T23:07:44Z

Once I've finished this rewrite then I think Morphorm will be in a better position to be integrated with taffy in some way if that's still desired.

It is!

nicoburns · 2022-10-25T10:29:55Z

Closing as this work was merged via a rebased version of this branch (#238)

Emil Sjölander added 30 commits April 25, 2019 10:07

make API more swifty

651299d

setup circleci

549d74a

Add test to scheme

176a0b8

Fix circleci

6dccd83

Fix circleci

8a9bdcd

Fix circleci

b4997bf

Fix circleci

ad06e26

Fix circleci

27d0aeb

Merge pull request #26 from vislyhq/swift-bindings

f3206bf

Swift bindings

Implement js bindings

a33184b

fix circleci

6051e2d

fix clippy and fmt

97049ea

fix circleci

b0ae50b

fix circleci

8a057b5

Merge pull request #27 from vislyhq/js-bindings

1a66123

Implement js bindings

publish to npm

c855a48

Add badges to readme

ec2360c

Update README.md

b5029ac

Update README.md

7f040bc

Port closed-source landing page over to gatsby + github pages

efaad5d

Basic docs setup

452b6de

Rust docs

24057fa

Finish rust docs

6a60a7d

Add docs to circleci

1272c7a

Fix circle config

1aab843

Fix circle config

9348214

Fix circle config

5fd124c

Fix circle config

2620fa5

Update banner

3c83806

Update README.md

47d456c

jkelleyrtp added 4 commits July 15, 2022 21:10

feat: move measurefunc out of nodedata

dbfcd63

feat: extract out some measure requirements

ae74ae5

wip: clean up generalization a bit

19a24a3

feat: pass all tests

8d82de1

alice-i-cecile added enhancement New feature or request code quality Make the code cleaner or prettier. controversial This work requires a heightened standard of review due to implementation or design complexity labels Jul 18, 2022

alice-i-cecile requested review from TimJentzsch and Weibye July 18, 2022 02:17

twop reviewed Jul 19, 2022

View reviewed changes

nicoburns reviewed Jul 22, 2022

View reviewed changes

nicoburns mentioned this pull request Jul 24, 2022

Implement CSS Grid #205

Merged

nicoburns mentioned this pull request Aug 22, 2022

Return iterator instead of vector of children #222

Closed

bestouff reviewed Aug 23, 2022

View reviewed changes

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

nicoburns mentioned this pull request Oct 13, 2022

Abstract flexbox over LayoutTree (rebased on top of latest main) #238

Merged

nicoburns closed this Oct 25, 2022

nicoburns deleted the jk/abstract-over-storage branch February 8, 2023 14:04

Conversation

jkelleyrtp commented Jul 18, 2022

Uh oh!

twop Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bestouff Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoburns commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geom3trik commented Jul 23, 2022

Uh oh!

alice-i-cecile commented Jul 23, 2022

Uh oh!

bestouff Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bestouff commented Aug 23, 2022

Uh oh!

bestouff commented Aug 23, 2022

Uh oh!

nicoburns commented Aug 23, 2022

Uh oh!

nicoburns commented Oct 10, 2022

Uh oh!

alice-i-cecile commented Oct 10, 2022

Uh oh!

geom3trik commented Oct 10, 2022

Uh oh!

alice-i-cecile commented Oct 10, 2022

Uh oh!

nicoburns commented Oct 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

twop Jul 19, 2022 •

edited

Loading

bestouff Aug 23, 2022 •

edited

Loading

nicoburns commented Jul 22, 2022 •

edited

Loading

bestouff Aug 23, 2022 •

edited

Loading