Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute content size, padding and border for each node #573

Merged
merged 44 commits into from
Nov 21, 2023

Conversation

nicoburns
Copy link
Collaborator

@nicoburns nicoburns commented Oct 29, 2023

Objective

Changes made

  • Overflow::Clip variant added to the Overflow enum. Layout with Clip behaves the same as Visible, except the overflowing contents don't contribute to the ancestor nodes' content sizes.
  • content_size feature flag added
  • content_size field added to Layout struct (stored on Node) and LayoutOutput (returned to parent) structs
  • border and padding fields added to the Layout struct
  • Implement content_size computation for all layout modes
  • Add scroll_width and scroll_height methods to Layout and add tests for scroll width/height
  • Remove caching run_mode exception for leaf layout. We now need to run leaf layout

Todo

  • Feature flag the block layout implementation
  • Feature flag the leaf layout implementation (if appropriate)

Context

Useful for any UI toolkit wanting to implement scrolling (e.g. bevyengine/bevy#8104). Although it's worth noting that this implementation assumes that every node is a scroll node, and it may be possible to do this cheaper if you can avoid that assumption at a higher level.

Feedback wanted

  • General code review
  • Should this be merged as-is (causes 5-20% performance hit). Or can anyone think of any way to reduce to reduce the performance impact - as far as I can tell is the actually computation of the content sizes from the sizes of the child nodes?
  • If it should be merged, should it be enabled by default?

@nicoburns nicoburns added the enhancement New feature or request label Oct 29, 2023
@nicoburns nicoburns added this to the 0.4 milestone Oct 29, 2023
Copy link
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit, it's becoming quite hard for me to grasp all the layout logic as it's becoming more and more complex.
I wonder if we can improve this somehow, maybe with more inline docs or with better abstractions or if the complexity of Flexbox and the performance considerations make this a necessity.

Regarding the performance, it would of course be great if we only had to pay this cost if scrolling is enabled, but I assume this would be difficult to implement right now.
I assume Bevy and other clients will always have this feature enabled, so it would be great to reduce the performance cost somehow.
If the feature is enabled by default or not is less important IMO.

scripts/gentest/src/main.rs Outdated Show resolved Hide resolved
scripts/gentest/test_helper.js Outdated Show resolved Hide resolved
src/compute/flexbox.rs Outdated Show resolved Hide resolved
src/compute/flexbox.rs Outdated Show resolved Hide resolved
src/compute/flexbox.rs Show resolved Hide resolved
src/compute/flexbox.rs Outdated Show resolved Hide resolved
src/compute/taffy_tree.rs Outdated Show resolved Hide resolved
@nicoburns nicoburns force-pushed the output-content-size branch 2 times, most recently from 257ec2a to 0e74c94 Compare October 30, 2023 00:06
src/style/mod.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be merged as-is (causes 5-20% performance hit). Or can anyone think of any way to reduce to reduce the performance impact - as far as I can tell is the actually computation of the content sizes from the sizes of the child nodes?

I'm okay to merge as is: I think this is fundamentally a necessary feature for the spec.

If it should be merged, should it be enabled by default?

Hmm. So ultimately our users are UI crates that consume taffy. Some of them will be simple, and won't support scrolling, others are more complex and do. Unless they make a feature flag to mirror this, that control won't be exposed to users.

In Bevy for example, I would probably push for a mirror of this feature flag, and then enable it by default in Bevy.

On the balance, I think we should keep this on by default, to match our existing policy of enabling default features for multiple different layout algorithms, even if not all of them are useful to all UI crates.

@nicoburns nicoburns force-pushed the output-content-size branch 3 times, most recently from a51ed92 to d8d0700 Compare November 15, 2023 21:31
@nicoburns nicoburns changed the title Compute content size for each node Compute content size, padding and border for each node Nov 21, 2023
@nicoburns
Copy link
Collaborator Author

There's still a performance hit with this, but it's much better since #579 (now more like 5-8%). So I think I'm going to enable this by default. I suspect we may also need to tweak the computed size in future (as we discover bugs), but I think this is good enough as a first cut at the feature.

@nicoburns nicoburns merged commit 64f8aa0 into DioxusLabs:main Nov 21, 2023
22 checks passed
@nicoburns nicoburns deleted the output-content-size branch November 21, 2023 22:43
osiewicz added a commit to zed-industries/zed that referenced this pull request May 10, 2024
We reverted bump to taffy 0.4.3 following an issue spotted by
@maxdeviant where chat text input was not being rendered correctly:

![image](https://github.com/zed-industries/zed/assets/24362066/9d7e6444-47b1-4ac2-808f-bf10404377c0)
This was an issue with the previous attempt to upgrade to taffy 0.4.0 as
well. We bail early in `compute_auto_height_layout` due to a missing
width:
https://github.com/zed-industries/zed/blob/df190ea84621837c44fa50c62837bdbea04b9e22/crates/editor/src/element.rs#L5266
The same issue is visible in story for auto-height editor (or rather,
the breakage is visible - the editor simply does not render at all
there).

I tracked down the breakage to
DioxusLabs/taffy#573 ; it looks like it
specifically affects editors with auto-height. In taffy <0.4 which we
were using previously, we'd eventually get a proper width for
auto-height EditorElement after having initially computed the size. With
taffy 0.4 however (and specifically that PR mentioned earlier), we're
getting `Size::NONE` in layout phase [^1].
I've noticed though that even with taffy <0.3, the
`known_dimensions.width` was always equal to `available_space.width` in
layout phase. Hence, I went with falling back to `available_space.width`
when it is a definite value and we don't have a
`known_dimensions.width`.
Done this way, both chat input and auto-height story render correctly.
/cc @as-cii 
Related:
#11606
#11622
#7868
#7896

[^1]: This could possibly be related to change in what gets passed in
https://github.com/DioxusLabs/taffy/pull/573/files#diff-60c916e9b0c507925f032cecdde6ae163e41b84b8e4bc0a6c04f7d846b0aad9eR133
, though I'm not sure if editor is a leaf node in this case

Release Notes:

- N/A
osiewicz added a commit to RemcoSmitsDev/zed that referenced this pull request May 18, 2024
We reverted bump to taffy 0.4.3 following an issue spotted by
@maxdeviant where chat text input was not being rendered correctly:

![image](https://github.com/zed-industries/zed/assets/24362066/9d7e6444-47b1-4ac2-808f-bf10404377c0)
This was an issue with the previous attempt to upgrade to taffy 0.4.0 as
well. We bail early in `compute_auto_height_layout` due to a missing
width:
https://github.com/zed-industries/zed/blob/df190ea84621837c44fa50c62837bdbea04b9e22/crates/editor/src/element.rs#L5266
The same issue is visible in story for auto-height editor (or rather,
the breakage is visible - the editor simply does not render at all
there).

I tracked down the breakage to
DioxusLabs/taffy#573 ; it looks like it
specifically affects editors with auto-height. In taffy <0.4 which we
were using previously, we'd eventually get a proper width for
auto-height EditorElement after having initially computed the size. With
taffy 0.4 however (and specifically that PR mentioned earlier), we're
getting `Size::NONE` in layout phase [^1].
I've noticed though that even with taffy <0.3, the
`known_dimensions.width` was always equal to `available_space.width` in
layout phase. Hence, I went with falling back to `available_space.width`
when it is a definite value and we don't have a
`known_dimensions.width`.
Done this way, both chat input and auto-height story render correctly.
/cc @as-cii 
Related:
zed-industries#11606
zed-industries#11622
zed-industries#7868
zed-industries#7896

[^1]: This could possibly be related to change in what gets passed in
https://github.com/DioxusLabs/taffy/pull/573/files#diff-60c916e9b0c507925f032cecdde6ae163e41b84b8e4bc0a6c04f7d846b0aad9eR133
, though I'm not sure if editor is a leaf node in this case

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output "scroll width/height" for each node Expose border calculated value
3 participants