Refactor Style component into many smaller pieces
#5513
Conversation
Weibye
added
A-UI
alice-i-cecile
added
the
C-Usability
alice-i-cecile
changed the title
Style component into many smaller pieces
There was a problem hiding this comment.
This looks good in my eyes, I particularly like the changed filter!
I do share your concern about example-migrating. That being said, are there one relatively simple example we could migrate while still in draft-phase so we can better see the actual implication on user-code?
|
I'd suggest only migrating the |
There was a problem hiding this comment.
I'm not sure how I feel about the general concept yet. I need to see it in action first, but I'm a bit concerned that it will reduce discoverability of all the style parameters. Fortunately we could have a builder type thing so it might be fine.
I like that it flattens styling a bit and puts color at the same level as everything else. I also like the rename to Layout.
My main criticism right now is that all instances of Flexbox should be renamed to Flex
|
This should probably be done in a future PR just to not let this one grow too much, but doing this for TextStyle should probably be done too. |
alice-i-cecile
force-pushed
the
style-fracture
branch
from
87f0f7b to
a6dc7a9
Compare
We can group the components under a module, so they are listed in rendered documentation. |
alice-i-cecile
force-pushed
the
style-fracture
branch
from
be47f70 to
c5a319e
Compare
|
Been thinking a bit about this recently, so here's some deeper reasoning and context: Let's consider the use case of us implementing borders: (#3991)
Option 1
This has the downside of splitting concerns across multiple components without providing consistency. Option 2
The end-user experience is better in this situation, but it does fragment Option 3
End user experience is much more verbose, but this is technically closer to how CSS actually operates. I think option 2 or 3 is the best way to go as they provide much more consistency, and they pave the way to unlock more style functionality that isn't related to layouting. Option 3 is probably the best if, and only if we manage to build higher level ergonomics on top, but that is something that has to come later. We can start with path 2, then go to 3 later. My conclusion is that we need this merged to unlock the way forward. |
|
How does one handle reusable styles when its fragmented into multiple components like this? Perhaps a separate monolithic style bundle? 🤔Internally I'm still debating if styles should be attached to entities or if they should be treated more like materials that live inside of the asset system. |
Here's how I'm picturing it at a high level:
The heavy lifting will be done by the theming-plugin, and we should try to aim for a design that separates theme from style. |
|
I would suggest to reorder the migration table a bit to have all flexbox layout stuff together. |
The "store each style as an entity" model in bevyengine/rfcs#1 would work well with more scattered styling components. It's also fully extensible to user-defined style properties. We already have issues with this; other things you might want to share (such as color) live outside of the |
There was a problem hiding this comment.
My initial impression is very positive, this seems to be much more natural for ECS. It also facilitates reuse across different application areas and avoids having "unneded" style definitions for entities that only need a part of them.
Where exactly to make the split is probably the hard part, for example it's a bit weird from a user perspective to define the border size and the border color in separate places. But we already partially had this problem before.
I also assume that not every layout system will need all of these components, so maybe it's even a bit more efficient.
For now, I left some doc comments
Weibye
force-pushed
the
style-fracture
branch
from
b4b7b0b to
4f9ed1b
Compare
There was a problem hiding this comment.
I have:
- Brought this up to speed on
main - Migrated examples
LayoutStrategy -> Layout
Some of these new structs lend itself very well to various builder patterns like Spacing while others are not so clear how to best expand and improve, but we don't have to solve all of that now we can continually improve as we go :)
Weibye
requested review from
TimJentzsch and
oceantume
and removed request for
TimJentzsch and
oceantume
| /// Creates a [`UiRect`] where all sides are [`Val::Undefined`] | ||
| pub const UNDEFINED: UiRect = UiRect::all(Val::Undefined); | ||
|
|
||
| /// Creates a [`UiRect`] where all sides are [`Val::Auto`] | ||
| pub const AUTO: UiRect = UiRect::all(Val::Auto); |
There was a problem hiding this comment.
Do we have it documented somewhere what the difference between undefined and auto is?
There was a problem hiding this comment.
@TimJentzsch In general the meaning of Auto depends on the property being set. For a Rect, the main property where Auto is relevant is margin where Auto means "take up remaining available space" and Undefined resolves to zero. Currently Taffy and (bevy_ui) also accept Auto for padding and border where it is no different to Undefined. Taffy 0.3 (unreleased) is more strict about this and introduces a new type with no Auto variant for padding and border. I'm unsure as whether bevy_ui will follow Taffy in this regard.
There was a problem hiding this comment.
I'm unsure as whether bevy_ui will follow Taffy in this regard.
I would be strongly in favour of doing so.
There was a problem hiding this comment.
At some point we should probably have that in the doc comments as well, of the respective components.
|
|
||
| /// The inset of a UI node from its base position. | ||
| /// | ||
| /// Layouting is performed by the Flexbox layout algorithm where the value of [`Inset`] is considered. |
There was a problem hiding this comment.
How is it considered?
IMO this needs a bit more explanation what this actually does, especially because this is not a property in CSS (to my knowledge)
There was a problem hiding this comment.
Inset is what we until now has (incorrectly) been calling position. It works the same way, it just has been renamed.
https://developer.mozilla.org/en-US/docs/Web/CSS/inset
Better docs never hurt though, so will look into that
There was a problem hiding this comment.
It is a property in CSS. It's the shorthand name for the top, left, right, and bottom properties. Taffy previously called this position (and what is now position was called position_type). They have been renamed to inset and position to match CSS. https://developer.mozilla.org/en-US/docs/Web/CSS/inset
There was a problem hiding this comment.
I've updated the doc-comment a bit, hopefully to be a bit clearer.
There was a problem hiding this comment.
Oops, looks like my search was off.
I like that follow CSS terminology!
Weibye
force-pushed
the
style-fracture
branch
from
4ca1ef0 to
2c3a5e6
Compare
|
The way the constraints data is structured seems wrong to me. pub fn text_constraint(min_size: Val, size: Val, max_size: Val, scale_factor: f64) -> f32 {
match (min_size, size, max_size) {
(_, _, Val::Px(max)) => scale_value(max, scale_factor),
// ..
}
}called with: text_constraint(
layout.size_constraints.min.width,
layout.size_constraints.suggested.width,
layout.size_constraints.max.width,
scale_factor,
)With the constraints data stored per-axis: pub struct LengthConstraint {
pub min: Val,
pub max: Val,
pub suggested: Val,
} it's much more natural: pub fn text_constraint(length_constraint: LengthConstraint, scale_factor: f64) -> f32 {
match length_constraint {
LengthConstraint { max: Val::Px(max), .. } => scale_value(max, scale_factor),
// ..
}
}text_constraint(layout.size_constraints.width, scale_factor)In Bevy that is the worst it gets at the moment, but in Taffy there are lots of tangled sections like: if constants.node_inner_size.main(constants.dir).is_none() && constants.is_row {
child.target_size.set_main(
constants.dir,
child.size.main(constants.dir).unwrap_or(0.0).maybe_clamp(
child.resolved_minimum_size.main(constants.dir).into(),
child.max_size.main(constants.dir),
),
);
} else {
child.target_size.set_main(constants.dir, child.hypothetical_inner_size.main(constants.dir));
}where most of the complexity would vanish with some sort of per-axis interface. |
|
I agree with that. @Weibye, do you think we should implement that now, or fix it upstream first? |
If possible I'd prefer getting this merged in its current state, then tackle that in a separate PR so that discussion can take place more focused, and we can get some movement on these refactors. I do think we should fix the axis separation upstream first, then fix it here after. |
I'm not even completely convinced that this would an improvement for Taffy. It's true that accessing lots of values in a single axis is awkward atm. But equally it makes accessing both components of the same value easy, and that does happen a lot too. For example, applying aspect ratio is a single method call on |
Weibye
force-pushed
the
style-fracture
branch
from
f9a2171 to
fbb7c67
Compare
| pub control: LayoutControl, | ||
| /// Defines how this node's layout should be. | ||
| pub layout: FlexContainer, | ||
| /// Defines how this node should behave as a child of a node. | ||
| pub child_layout: FlexItem, |
There was a problem hiding this comment.
As a general pattern we call Bundle fields the same thing as their component. Calling them something different creates a conceptual disconnect. Ex: layout: FlexContainer should be flex_container: FlexContainer. Creating a new concept / name just adds one more term for people to contend with.
| /// The strategy used to position this node. | ||
| pub position: Position, | ||
| /// The inset of this UI node, relative to its default position | ||
| pub inset: Inset, |
There was a problem hiding this comment.
Why the extra type wrapper here? It doesn't add any functionality and just makes it harder to set and access these fields.
| /// Core controls for layouting of this node. | ||
| /// | ||
| /// See: [`Display`](crate::Display), [`Position`](crate::Position), [`Inset`](crate::Inset), [`Overflow`](crate::Overflow). | ||
| pub control: LayoutControl, |
There was a problem hiding this comment.
control feels like a pretty ambiguous term in this context, given that it is a verb. What does it control? What if we later have some other XControl thing? It also feels like the wrong thing to focus on given that LayoutControl is about "layout", not "control".
| /// Grouping of core control parameter of the layout for this node. | ||
| #[derive(Component, Copy, Clone, PartialEq, Debug, Serialize, Deserialize, Reflect)] | ||
| #[reflect(PartialEq, Serialize, Deserialize)] | ||
| pub struct LayoutControl { |
There was a problem hiding this comment.
If this is the "universal layout configuration component", maybe we can shorten the name to Layout? Like Display is really the "layout method selector", but we're keeping it aligned with CSS terminology, so that frees up the Layout term. This component encapsulates the "canonical layout method selector", so Layout feels like fair game.
| color: Color::BLACK, | ||
| }; | ||
|
|
||
| commands |
There was a problem hiding this comment.
This example is broken for me in this PR:
| .spawn(( | ||
| NodeBundle { | ||
| style: Style { | ||
| size_constraints: SizeConstraints::FILL_PARENT, |
There was a problem hiding this comment.
This example is broken for me in this PR:
| /// Grouping of core control parameter of the layout for this node. | ||
| #[derive(Component, Copy, Clone, PartialEq, Debug, Serialize, Deserialize, Reflect)] | ||
| #[reflect(PartialEq, Serialize, Deserialize)] | ||
| pub struct LayoutControl { |
There was a problem hiding this comment.
I think theres a strong argument to put all layout related things under a single Layout component (aka dont break things up, just rename Style to Layout). Outside of the more granular change detection (which doesn't matter for the actual Taffy layout impl and will likely rarely come up in user code), I don't think breaking this up meaningfully improves UX. And it actively degrades it in a number of places:
- Introduces more "virtual" concept nesting users need to contend with: Spacing (not a css term), Flex Container/Item (a css term, but not one users directly interact with in flex apis), SizeConstraints (not a CSS term,
size_constraints.mindiverges frommin_size), inset (a css term, but not one commonly used, and never aslayout.inset.top) - Discoverability is more challenging: users setting layouts now need to know how to access 5 components instead of 1. Additionally, having one component means you can discover all fields via autocomplete.
- Ergonomics downgrade: a user that wants to switch a node to "flex", set the flex container fields, and set the flex item fields now needs to write
Query<(&mut LayoutControl, &mut FlexContainer, &mut FlexItem)>. That is a pretty big mouthful compared toQuery<&mut Layout>.// centralized fn system(query: Query<&mut Layout>) { let mut layout = query.single(); layout.display = Display::Flex; layout.flex_direction = Direction::Row; layout.grow = 0.5; } // broken up fn system(query: Query<(&mut LayoutControl, &mut FlexContainer, &mut FlexItem)>) { let (mut layout_control, mut flex_container, mut flex_item) = query.single(); layout_control.display = Display::Flex; flex_container.direction = Direction::Row; flex_item.grow = 0.5; }
- Ergonomics downgrade: more nesting and verticality during instantiation:
// centralized commands.spawn(NodeBundle { position: Position::Absolute, flex_direction: Direction::Row, grow: 0.5, ..default() }) // broken up commands.spawn(NodeBundle { control: LayoutControl { position: Position::Absolute, ..default() }, flex_container: FlexContainer { direction: Direction::Row, ..default() }, flex_item: FlexItem { grow: 0.5, ..default() }, ..default() })
In theory, breaking things up into components could make things more modular. But in practice, Taffy is monolithic and a good portion of these concepts are deeply interrelated. And the centralized Display enum and "black box" nature of taffy from a user perspective prevents people from inserting new layout approaches into this meaningfully.
If I ask myself "will users be happy about these changes", in the current state I think the answer is likely "no". Maybe I'm wrong, but I'm not seeing the wins here.
There was a problem hiding this comment.
I'm not sure what others' motivations for this change are (I rather agree that it's not great from an ergonomics point of view, but one key benefit would be lower memory usage in the common case that not all styles are set. Taffy's Style struct is currently around 250 bytes. It's up to 300+ with CSS Grid support in Taffy 0.3, and is only likely to increase in future when add support for things like Morphorm layout and RTL text/flow-relative styles. Granted, we're probably only talking about 5-10mb here for realistic UI node counts of ~10000 (I'm not sure how big a deal that would be considered).
There was a problem hiding this comment.
Are we going to support other layout algorithms in Bevy, e.g. grids?
In that case I think it might make sense to keep them separated
There was a problem hiding this comment.
Yes, that's my ultimate hope.
There was a problem hiding this comment.
A bit late to the party here:
In theory, breaking things up into components could make things more modular.
The original intent behind this was to break everything up so that things like Margin, Padding, Display, Direction would be top level components in order to get close to the "behaviour" of CSS.
Then over time it morphed to what you see here which is due to multiple good arguments & various pushbacks.
I do agree with closing this particular PR, but I keep having the feeling that the original idea would be worthwhile still but it will require a different execution.
There was a problem hiding this comment.
It seems to me that the intent of the proposal is to make NodeBundle into a flat namespace. But currently that would entail each style being it's own bevy_ecs Component, which is considered undesirable for perf reasons. Perhaps a possble alternative would be some way of mapping bundles to components that didn't just assume that each property of the bundle struct is it's own component, but allows a custom mapping.
There was a problem hiding this comment.
It seems to me that the intent of the proposal is to make NodeBundle into a flat namespace. But currently that would entail each style being it's own bevy_ecs Component, which is considered undesirable for perf reasons.
Yeah, you sum it up very well here. The idea was to achieve increased ergonomics and more granular change detection but I'm starting to get the feeling that this is trying to solve the symptom instead of the root cause.
Ergonomic UI in Bevy is IMO more likely to be achieved with finding a better way to structure data-flow and high level components instead of just changing the api surface of what is already there. In short: We need a working design :)
The naive implementation I was thinking of is where the main and cross functions would return a value (a reference to the whole Style struct I guess) wrapped in an enum: enum Axis<T> {
Row(T)
Column(T)
}and then the chained functions won't need an axis parameter, they can just match against the axis enum. An alternative with Size might be to replace the min_size, max_size and max_size fields with: pub struct LengthConstraints<T> {
pub min: T,
pub max: T,
pub length: T,
}
pub struct Style {
pub size: Size<LengthConstraints<Dimension>>,
// .. rest of style properties
}and then directly implement all the math, clamping, resolve, etc traits on |
|
There should probably be a different type for border and padding. It doesn't make sense to assign either of them auto or undefined values. |
|
Closing out: this isn't going to be worth rebasing. I still think we should do this, but it's not sufficiently motivated until we have multiple layout algorithms. |
16 participants
Note for release notes: this PR is co-authored with @Weibye <3
Objective
Stylecomponent is monolithic and poorly named (it only controls layout). This makes it hard to work with and reason about.Styleinto multiple components #5511.Changelog
StyleUI component into many small componentsFlexboxLayoutQuery,FlexboxLayoutQueryItemandFlexboxLayoutChangedhelper types to make recomputing layouts easierMigration Guide
Up to date as of 76557be
The
Style-component has been removed. In its place, there are several new components:LayoutControlFlexContainerFlexItemSizeConstraintsSpacingStyle.direction: DirectionStyle.position_type: PositionTypeLayoutControl.position: PositionStyle.position: UiRectLayoutControl.inset: Inset(UiRect)Style.overflow: OverflowLayoutControl.overflow: OverflowStyle.display: DisplayLayoutControl.display: DisplayStyle.flex_direction: FlexDirectionFlexContainer.direction: DirectionStyle.flex_wrap: FlexWrapFlexContainer.wrap: WrapStyle.align_items: AlignItemsFlexContainer.align_items: AlignItemsStyle.align_content: AlignContentFlexContainer.align_content: AlignContentStyle.justify_content: JustifyContentFlexContainer.justify_content: JustifyContentStyle.gap: Gap(UiRect)FlexContainer.gap: Gap(UiRect)Style.align_self: AlignSelfFlexItem.align_self: AlignSelfStyle.flex_grow: f32FlexItem.flex_grow: f32Style.flex_shrink: f32FlexItem.flex_shrink: f32Style.flex_basis: ValFlexItem.flex_basis: ValStyle.margin: UiRectSpacing.margin: UiRectStyle.padding: UiRectSpacing.padding: UiRectStyle.border: UiRectSpacing.border: UiRectStyle.size: SizeSizeConstraints.suggested: SizeStyle.min_size: SizeSizeConstraints.min: SizeStyle.max_size: SizeSizeConstraints.max: SizeStyle.aspect_ratio: Option<f32>SizeConstraints.aspect_ratio: Option<f32>Queries
Any queries that previously queried all of
Styleshould now instead use theFlexLayoutQuery.Most of the time however, you'd likely want to query for the more specific layout components you need.
Change detection
If you previously relied on detecting when something on
Stylechanged you can now instead use theFlexLayoutChangedtype.Small related changes
Various helper methods on
TextBundlehave been removed for consistency with other bundles in the engine. Construct these explicitly, or use a trait extension method to redefine them if you prefer this style.