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

Fix flexbox baseline alignment #325

Merged
merged 9 commits into from
Jan 16, 2023

Conversation

nicoburns
Copy link
Collaborator

@nicoburns nicoburns commented Jan 12, 2023

Objective

  • Fix flexbox baseline alignment implementation (fixes baseline alignment tests imported from yoga)
  • Refactor flexbox baseline alignment implementation so that it doesn't require directly accessing the layout of descendant nodes other than direct children (required to make it's baseline alignment interoperate with Grid and other layout modes properly)
  • Optimise baseline alignment computation so that child baseline are only calculated when necessary

Context

Note: This PR depends on #320 which in turn depends on #317 both now merged!

I did a quick run of the benchmarks vs. main to check for regressions, and actually I'm seeing mild performance wins (10% - 30% on some benchmarks) on quite a few of the benchmarks. There is also a ~10% regression on a couple, but those are the very smallest ones that are completing in single-digit microseconds anyway, so I'm not going to worry too much about those (we probably ought to remove those super small ones at some point - I don't think they're very representative of real-world performance).

Feedback wanted

Does changing each algorithm's perform_layout function to return:

struct SizeAndBaselines {
   size: Size<f32>,
   first_baseline: Point<Option<f32>>,
}

instead of Size<f32> as I have done in this PR seem reasonable? This is instead of the 3rd layout method proposed in #28 (comment)

@nicoburns nicoburns added bug Something isn't working usability Make the library more comfortable to use breaking-change A change that breaks our public interface blocked Cannot be advanced until something else changes controversial This work requires a heightened standard of review due to implementation or design complexity labels Jan 12, 2023
@nicoburns nicoburns mentioned this pull request Jan 12, 2023
87 tasks
@nicoburns nicoburns force-pushed the baseline-alignment branch 3 times, most recently from 1dc8683 to ef8e73d Compare January 12, 2023 21:23
@nicoburns nicoburns changed the title WIP: Fix baseline alignment + Add layout methods to LayoutTree trait WIP: Fix baseline alignment Jan 12, 2023
@nicoburns nicoburns removed usability Make the library more comfortable to use breaking-change A change that breaks our public interface controversial This work requires a heightened standard of review due to implementation or design complexity labels Jan 12, 2023
This was referenced Jan 12, 2023
@nicoburns nicoburns removed the blocked Cannot be advanced until something else changes label Jan 13, 2023
@nicoburns nicoburns changed the title WIP: Fix baseline alignment Fix flexbox baseline alignment Jan 14, 2023
@nicoburns nicoburns marked this pull request as ready for review January 15, 2023 23:53
@nicoburns
Copy link
Collaborator Author

This still doesn't implement baseline alignment for CSS Grid, but I now think I'd like to merge this without that to unblock #326. These changes (fixing baseline alignment for flexbox, and refactoring the flexbox implementation so that it doesn't require accessing the layout of descendant nodes other than direct children) are worthwhile on their own, and baseline alignment for CSS Grid can follow in another PR.

@@ -130,14 +158,14 @@ struct AlgoConstants {
}

/// Computes the layout of [`LayoutTree`] according to the flexbox algorithm
pub fn compute(
pub fn compute_generic(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This name isn't very clear.

/// Grid layout algorithm
/// This consists of a few phases:
/// - Resolving the explicit grid
/// - Placing items (which also resolves the implicit grid)
/// - Track (row/column) sizing
/// - Alignment & Final item placement
pub fn compute(
pub fn compute_generic(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto on the name here.

src/compute/leaf.rs Outdated Show resolved Hide resolved
src/compute/leaf.rs Outdated Show resolved Hide resolved

/// The public interface to a generic algorithm that abstracts over all of Taffy's algorithms
/// and applies the correct one based on the `Display` style
pub struct GenericAlgorithm;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. I'm not very happy with this naming / docs. Maybe something like UnifyingLayoutAlgorithm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This struct actually disappears in my latest version of #326, as I've made the "generic" algorithm and the "leaf" algorithm operate on &mut Taffy rather than &mut impl LayoutTree (so they don't fit the LayoutAlgorithm trait anymore). This allows us to remove measure_funcs from the LayoutTree trait and make them an implementation detail of our Taffy storage implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good I'm fine to wait.

src/layout.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.

Some feedback on structure and docs. I would like to get this merged quickly though, so feel free to defer some of the work (e.g. the trait refactor) to a follow-up PR.

@nicoburns
Copy link
Collaborator Author

@alice-i-cecile I've renamed the compute_generic functions back to compute, which is what they were already called. So I think it can now safely be said that the interface is no worse than it was. In any case, as these traits/functions are currently private, I'm not going to worry about too much (we can easily change this later if we want to). I suggest we bikeshed this in #326 (which does make the LayoutAlgorithm trait public) and also makes major changes to the public interface exposed through the LayoutTree trait.

Is there anything else blocking this?

@alice-i-cecile
Copy link
Collaborator

Nope, this now LGTM.

@alice-i-cecile alice-i-cecile merged commit b91874d into DioxusLabs:main Jan 16, 2023
@nicoburns nicoburns added this to the 0.3 "CSS Grid" milestone Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants