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

Remove the style component from ui widgets #6886

Closed
wants to merge 14 commits into from

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Dec 8, 2022

Objective

Widgets aren't flex containers and shouldn't have a style component.

fixes: #6879. #6717, #4009, #2249
related: #6807, #6743, #6724. #5502

Solution

Remove the style component from the UI widget bundles.

Changelog

  • removes the text_constraint function
  • removes the style field from TextBundle, ImageBundle, and ButtonBundle
  • changes to flex_node_system + text_system to support unstyled nodes
  • changes text example to use new TextBundle
  • adds a ui layout example (produces the screenshot below)

Additional Information

Progress

Capture

Bevy UI was using the MeasureFunc that preserves aspect ratio for both images and text.
This meant that the cross-axis of flex items containing text would be calculated incorrectly sometimes
depending on how the aspect ratio of the text compared to the size of the containing node.

changes:
    * Added parameter preserve_aspect_ratio to the upsert_leaf function.
    * The MeasureFunc only preserves the aspect ratio when preserve_aspect_ratio is true.
    * flex_node_system queries for UiImage before it calls upsert_leaf
    and sets preserve_aspect_ratio appropriately.
…aspect_ratio field to CalculatedSize which is

set to true for image nodes by update_image_calculated_size_system.
@mockersf
Copy link
Member

mockersf commented Dec 8, 2022

The style component is not just for flex containers, it's also for items. Currently it contains both. Removing it from nodes like image and friends would mean adding an extra layer of nodes each time to specify those properties, I don't see that as a good idea?

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Dec 8, 2022

The style component is not just for flex containers, it's also for items. Currently it contains both. Removing it from nodes like image and friends would mean adding an extra layer of nodes each time to specify those properties, I don't see that as a good idea?

Yes the extra boiler plate is a really annoying downside. It will be a lot easier with with_child though, then you just have:

commands.spawn(Nodebundle {
  style: Style {
    min_size: Size::new(Val::Px(100., 100.),
    ..Default::default()
  },
  ..Default::default()
})
.with_child(ImageBundle::new(image_handle));

which isn't so bad.

At the moment the style component doesn't work correctly for the image and text bundles, and there are PRs that fix some of those problems with specific hacks. And it definitely can all be made to work. But then if there are more widget bundles added then there probably would need to be more hacks for those too etc.

@mockersf
Copy link
Member

mockersf commented Dec 8, 2022

I would prefer to wait for #5513 and split flex settings between flex container and flex items then only have the relevant component, than removing style completely and adding an extra layer everywhere

@ickshonpe
Copy link
Contributor Author

I would prefer to wait for #5513 and split flex settings between flex container and flex items then only have the relevant component, than removing style completely and adding an extra layer everywhere

Think you might be right about splitting the style settings, and after messing around with this I think I have another way to fix the padding issue. Putting it aside for now.

@ickshonpe ickshonpe closed this Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text and Images can't be given padding
2 participants