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

Add const builder methods to bevy_ui #5529

Closed
alice-i-cecile opened this issue Aug 1, 2022 · 2 comments
Closed

Add const builder methods to bevy_ui #5529

alice-i-cecile opened this issue Aug 1, 2022 · 2 comments
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

Constants are useful when manually defining UIs to get compile-time guarantees that your style / layout won't unexpectedly change at runtime.

However, many of the existing builder methods are not const, when they could be.

What solution would you like?

  1. Look through bevy_ui, and make any methods that you can const. This is particularly important for methods that return a new struct of the type that the impl block is for (a "builder method").
  2. Ensure that all of the essential structs have a const equivalent to Default::default(). This should be a DEFAULT associated constant. Use these new constants in the default trait implementations where feasible.

What alternative(s) have you considered?

Use a const fn new(). This is less clear, doesn't display the actual value in docs, and may have indirection overhead (IIRC this varies based on the optimization level). These are simple values, they should be represented as such.

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 1, 2022
@james-j-obrien
Copy link
Contributor

Two quick questions:

  1. Should this happen after Refactor Style component into many smaller pieces #5513 in order to prevent adding work to that PR? If 5513 isn't going to be merged for a while and we want to have this in I can implement now.
  2. There are a large amount of enums involved in styling and these would also all need const defaults to allow things like Button or Text bundles to have const defaults. The way to do this would be removing the derived Default and adding an assciated constant and then impl Default manually.
#[derive(Copy, Clone, PartialEq, Eq, Debug, Default, Serialize, Deserialize, Reflect)]
#[reflect_value(PartialEq, Serialize, Deserialize)]
pub enum PositionType {
    /// Relative to all other nodes with the [`PositionType::Relative`] value
    #[default]
    Relative,
    /// Independent of all other nodes
    ///
    /// As usual, the `Style.position` field of this node is specified relative to its parent node
    Absolute,
}

becomes:

/// The strategy used to position this node
#[derive(Copy, Clone, PartialEq, Eq, Debug, Serialize, Deserialize, Reflect)]
#[reflect_value(PartialEq, Serialize, Deserialize)]
pub enum PositionType {
    /// Relative to all other nodes with the [`PositionType::Relative`] value
    Relative,
    /// Independent of all other nodes
    ///
    /// As usual, the `Style.position` field of this node is specified relative to its parent node
    Absolute,
}

impl PositionType {
    const DEFAULT: Self = Self::Relative;
}

impl Default for PositionType {
    fn default() -> Self {
        Self::DEFAULT
    }
}

We lose the convenience and visibility of the macro but this is otherwise fine and could then be removed if/when rust gets const defaults.
As long as this trade off is ok I have no problem quickly running through and implementing this.

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Aug 1, 2022

  1. As the author of that PR, I'd be happy to see this occur first.
  2. I think this is likely worth the tradeoff. Frustrating, but the ability to define constant layout and styles using struct update syntax / for helper methods is very valuable IMO.

CC @Weibye @TimJentzsch; we'll likely want to pursue this upstream in taffy as well if Bevy ends up going this route.

@bors bors bot closed this as completed in df3673f Jan 4, 2023
alradish pushed a commit to alradish/bevy that referenced this issue Jan 22, 2023
# Objective
- Fixes bevyengine#5529 

## Solution
- Add assosciated constants named DEFAULT to as many types as possible
- Add const to as many methods in bevy_ui as possible

I have not applied the same treatment to the bundles in bevy_ui as it would require going into other bevy crates to implement const defaults for structs in bevy_text or relies on UiImage which calls HandleUntyped.typed() which isn't const safe.

Alternatively the defaults could relatively easily be turned into a macro to regain some of the readability and conciseness at the cost of explicitness.
Such a macro that partially implements this exists as a crate here: [const-default](https://docs.rs/const-default/latest/const_default/derive.ConstDefault.html) but does not support enums.

Let me know if there's anything I've missed or if I should push further into other crates.

Co-authored-by: Carter Anderson <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective
- Fixes bevyengine#5529 

## Solution
- Add assosciated constants named DEFAULT to as many types as possible
- Add const to as many methods in bevy_ui as possible

I have not applied the same treatment to the bundles in bevy_ui as it would require going into other bevy crates to implement const defaults for structs in bevy_text or relies on UiImage which calls HandleUntyped.typed() which isn't const safe.

Alternatively the defaults could relatively easily be turned into a macro to regain some of the readability and conciseness at the cost of explicitness.
Such a macro that partially implements this exists as a crate here: [const-default](https://docs.rs/const-default/latest/const_default/derive.ConstDefault.html) but does not support enums.

Let me know if there's anything I've missed or if I should push further into other crates.

Co-authored-by: Carter Anderson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants