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

Draft: Discussion about builder pattern #10833

Closed
wants to merge 1 commit into from

Conversation

TotalKrill
Copy link
Contributor

DO NOT MERGE:

 // Builder pattern ( 8 lines )
 commands.spawn(
    NodeBundle::default().style(
         Style::default()
            .width(Val::Percent(100.0))
            .height(Val::Percent(100.0))
            .justify_content(JustifyContent::SpaceBetween),
    ),
 );
  // Struct pattern (9 lines)
 commands.spawn(NodeBundle {
     style: Style {
        width: Val::Percent(100.0),
        height: Val::Percent(100.0),
        justify_content: JustifyContent::SpaceBetween,
         ..default()
    },
      ..default()
 });

Just wanted to start a discussion here, because implementing a builder pattern into bevy feels quite trivial to implement.

Pros:

  • Less lines of code
  • Some people prefer this
  • The spawn().with_children() and many other parts of the code is a kind of builder pattern, but towards bevys runtime
  • the ..default() pattern makes refactoring code that is Bundle::default() a bit more annoying
  • the ..default() patterns breaks frequently with cargo fmt on save ( such as for me, a chronical terminal text editor user)

Cons:

  • Initial Line gets longer Bundle { ... vs Bundle::default()
  • Some people dislike this
  • Would allow two ways of specifying a struct....
  • Probabl something else

Objective

Try out how builder pattern feels, compared to the struct pattern

Solution

derive setters for member methods, and try out how creating UI nodes feel with that.

@cart
Copy link
Member

cart commented Dec 1, 2023

I've been pretty strongly against this for the lifetime of the project:

  1. Its one more thing people need to derive, adding boilerplate
  2. It generates more code, increasing compile times for each component
  3. Not everyone will do it so it fractures the ecosystem.

The current Default/spread approach is simple, legible, and reasonably ergonomic. For those that really want to reduce typing, I think something like BSN is the move.

@TotalKrill
Copy link
Contributor Author

I don't feel that builder Patterns necessarily competes with a scene system such as Bsn.

More advanced structs really could do with some hand crafted builder methods. Looking at the [Style] component, that has members that have multiple internal interactions.

Looking at [Transform] and the setters and initializers really shine when creating them where the looking_at methods and such are absolutely critical, and basically follows a bit of a builder pattern (not really I know, since they modify the underlying data, instead of consuming or creating it as builder patterns does)

I do not believe allowing setter methods on bevy Bundles is going to fracture the ecosystem, rather the opposite, since we are already using similar methods here and there. Using them everywhere would make it more uniform.

Regarding boilerplate and compile time, it's mostly for first time compilations, iterative compilation time would not be affected right?

If I could have added this as a standalone crate, that would have been better, unfortunately that is not possible afaik

@bushrat011899
Copy link
Contributor

Personally, I think ..default() is fine, and I think components with a large number of fields (like Style) should be broken up, which is the primary area you'd have this discussion in anyway.

It's a bit tangential to this actual discussion, but I've been mulling over proposing a change that would remove the Style component fields, and push them into their own components. It feels antithetical to an ECS to create the component which has everything inside it, as opposed to breaking it up along access patterns (e.g., layout, post-processing, fonts, etc.)

Back on topic, I think the builder pattern is best for complex structures (like App), but components should be simpler structures.

@TotalKrill
Copy link
Contributor Author

Back on topic, I think the builder pattern is best for complex structures (like App), but components should be simpler structures.

I agree that components should be simpler structures, but builder patterns can still help, such as with Transform.

Bundles are complex structures, it's their entire purpose, for which builder patterns are a great fit.

I have a feeling that the hard stance on builder patterns are a bit detrimental. For example builder patterns for PbrBundle could have created helper functions to spawn in 3D models or simple shape. But the feeling that it is "wrong" might make people avoid it, instead of discussing how they would be most ergonomic to use.

And even with that said, it doesn't feel like it would go against something like the Bsn concept. (which is a derive macro, that increases the learning curve since those become a Micro-language in themself, and have small caveats and things that differ from regular Rust).

@bushrat011899
Copy link
Contributor

And even with that said, it doesn't feel like it would go against something like the Bsn concept.

I think the argument is because BSN will be built around the struct itself (I don't believe there are plans for the BSN to include support for arbitrary code generating components/bundles, but I could be wrong!), so a focus on the builder pattern would make one or the other a second class citizen in the API.

I think the builder pattern is excellent, and it's definitely encouraged in Bevy, App is a perfect example of the builder pattern IMO.

@alice-i-cecile alice-i-cecile marked this pull request as draft December 4, 2023 18:17
@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets S-Blocked This cannot move forward until something else changes C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Dec 4, 2023
@alice-i-cecile
Copy link
Member

It's a bit tangential to this actual discussion, but I've been mulling over proposing a change that would remove the Style component fields, and push them into their own components. It feels antithetical to an ECS to create the component which has everything inside it, as opposed to breaking it up along access patterns (e.g., layout, post-processing, fonts, etc.)

See #5513 for a previous attempt :)

Otherwise, I definitely agree with Cart here: this pattern is quite hard to maintain, and fills the same sort of space as BSN. I think, for bundles and so on, we should avoid it.

@nicoburns
Copy link
Contributor

@bushrat011899

It's a bit tangential to this actual discussion, but I've been mulling over proposing a change that would remove the Style component fields, and push them into their own components. It feels antithetical to an ECS to create the component which has everything inside it, as opposed to breaking it up along access patterns (e.g., layout, post-processing, fonts, etc.)

This is actually already the case with Style. Style contains the properties used during layout. Which are typically all accessed together.

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 S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants