bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Add const builder methods to bevy_ui

Open alice-i-cecile opened this issue 2 years ago • 2 comments

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 avatar Aug 01 '22 15:08 alice-i-cecile

Two quick questions:

  1. Should this happen after #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.

james-j-obrien avatar Aug 01 '22 19:08 james-j-obrien

  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.

alice-i-cecile avatar Aug 01 '22 19:08 alice-i-cecile