game_menu example: pull common patterns into methods #12008

awwsmm · 2024-02-20T17:56:36Z

Objective

examples/games/game_menu has a decent amount of boilerplate
this hides the logic and bloats this simple example to > 800 lines

Solution

pull common patterns into methods for reusability, better adherence to DRY
allows newbies to focus on the differences between different menus, ignoring similarities

awwsmm · 2024-02-20T17:57:07Z

I also made a FSM diagram for this example, though I'm unsure of where the best place for this would be in the repo

awwsmm · 2024-02-20T17:58:49Z

examples/games/game_menu.rs

@@ -45,10 +45,62 @@ fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
 }

+// This outermost node encompasses the entire Window (100% width and height)
+// and centers all content vertically (align_items) and horizontally (justify_content)
+fn outer_node(


This is a pattern common to every screen of this example, and was previously repeated verbatim in 6 different places

awwsmm · 2024-02-20T17:59:25Z

examples/games/game_menu.rs

+}
+
+// This node is always a direct child of the `outer` node, and arranges contents in a column, from top to bottom
+fn inner_node<T: Into<BackgroundColor>>(


This pattern was previously used on every single screen except for the Splash screen, and so was repeated in five places

awwsmm · 2024-02-20T18:01:26Z

examples/games/game_menu.rs

-    fn main_menu_setup(mut commands: Commands, asset_server: Res<AssetServer>) {
-        // Common style for all buttons on the screen
-        let button_style = Style {
+    fn button_style() -> Style {


This pattern was previously repeated verbatim in four different places. The only difference between the Main menu and the Main Settings menu was that the buttons were 50px wider on the Main menu. I've made the Main Settings menu buttons 50px wider in order to not have to repeat this configuration

awwsmm · 2024-02-20T18:01:57Z

examples/games/game_menu.rs

+        }
+    }
+
+    fn button_text_style() -> TextStyle {


This pattern was previously repeated verbatim in six places

Have you considered extracting it on the root as a function? A similar pattern is used in game_setup, we could re-use the same code.

fn default_font(font_size: f32) -> TextStyle { TextStyle { font_size, color: TEXT_COLOR, ..default() } }

awwsmm · 2024-02-20T18:02:12Z

examples/games/game_menu.rs

+    }
+
+    // In the main menu and in the main settings menu, we create a column of buttons
+    fn column_of_buttons(


This pattern was previously repeated in two places

awwsmm · 2024-02-20T18:03:11Z

Net -160 lines makes it easier to see what's important and what's not on each screen, without having to mentally filter out all of the boilerplate

nicopap

I really like this set of changes! It suddenly makes the example much less overwhelming.

I suggest a few changes to even reduce more the number of lines, but also to make the code a bit more in line with rust conventions. (The curly braces surrounding closures are especially problematic)

You don't need to manually change anything! I opened a PR on your bevy fork. See the link: awwsmm#1

examples/games/game_menu.rs

nicopap · 2024-02-20T18:50:18Z

examples/games/game_menu.rs

+        }
+    }
+
+    fn button_text_style() -> TextStyle {


Have you considered extracting it on the root as a function? A similar pattern is used in game_setup, we could re-use the same code.

fn default_font(font_size: f32) -> TextStyle { TextStyle { font_size, color: TEXT_COLOR, ..default() } }

examples/games/game_menu.rs

nicopap · 2024-02-20T19:12:28Z

Regarding the state machine, I think it would work as ASCII art as module doc. Here is a wonderful draft

      Start
        v
GameState::Splash
        |
  |---------------------------------------------------------------------|
  |     |                       GameState::Menu                         |
  |     |         v------back-------<                                   |
  |     v         v                 |                                   |
  |  MenuState::Main >-settings-> MenuState::Settings <--------------<  |
  |   v    ^                        |       |                        |  |
  |   |    |                      sound  display                     |  |
  |  play  |                        v       v                        |  |
  |   |    |   MenuState::SettingsSound   MenuState::SettingsDisplay |  |
  |   |    |                     |              |                    |  |
  |   |    |                     >-------------->--------------------^  |
  |---+----+------------------------------------------------------------|
      |    |
      |  timer
      v    ^
   GameState::Game

nicopap

So the one thing to ask is: does this make the example harder to read? And I think it doesn't. In fact, it introduces some nice hints at good practices for UI dev for the current bevy UI state.

awwsmm · 2024-02-21T15:29:36Z

So the one thing to ask is: does this make the example harder to read? And I think it doesn't. In fact, it introduces some nice hints at good practices for UI dev for the current bevy UI state.

I also considered this. These changes definitely introduce a bit more indirection, but they also eliminate quite a lot of duplication. I think the net effect is improved readability and understandability.

benfrankel · 2024-02-26T10:34:22Z

I made another version of the diagram. There are no edge labels, but they should be easy to infer hopefully.

              START                                                             
                │                                                               
      ┌─────────▼─────────┐                                                     
      │GameState::Splash  │                                                     
      │MenuState::Disabled│  ┌───────────────────┐  ┌──────────────────────────┐
      └─────────┬─────────┘  │                   ├─►│GameState::Menu           │
      ┌─────────▼─────────┐  │                   │◄─┤MenuState::SettingsDisplay│
END ◄─┤GameState::Menu    ├─►│GameState::Menu    │  └──────────────────────────┘
      │MenuState::Main    │◄─┤MenuState::Settings│  ┌──────────────────────────┐
      └────────┬─▲────────┘  │                   ├─►│GameState::Menu           │
      ┌────────▼─┴────────┐  │                   │◄─┤MenuState::SettingsSound  │
      │GameState::Game    │  └───────────────────┘  └──────────────────────────┘
      │MenuState::Disabled│                                                     
      └───────────────────┘

benfrankel

Approving because this is an overall improvement. Just left a few nits.

examples/games/game_menu.rs

nicopap · 2024-02-26T17:57:41Z

FYI, to rebase, you just need to rename Color to LegacyColor.

To avoid some headache, I would rebase and ignore the upstream changes. Here is how it would look like:

Rename Color to LegacyColor
Commit the change
Run git checkout -b example-game_menu-backup && git checkout - (to avoid nasty surprises)
Run git fetch && git rebase upstream/main
Run git checkout --theirs . && git add -u && git rebase --continue
Make sure everything works by running cargo run --example game_menu

awwsmm · 2024-03-12T12:38:27Z

I made another version of the diagram. There are no edge labels, but they should be easy to infer hopefully.

              START                                                             
                │                                                               
      ┌─────────▼─────────┐                                                     
      │GameState::Splash  │                                                     
      │MenuState::Disabled│  ┌───────────────────┐  ┌──────────────────────────┐
      └─────────┬─────────┘  │                   ├─►│GameState::Menu           │
      ┌─────────▼─────────┐  │                   │◄─┤MenuState::SettingsDisplay│
END ◄─┤GameState::Menu    ├─►│GameState::Menu    │  └──────────────────────────┘
      │MenuState::Main    │◄─┤MenuState::Settings│  ┌──────────────────────────┐
      └────────┬─▲────────┘  │                   ├─►│GameState::Menu           │
      ┌────────▼─┴────────┐  │                   │◄─┤MenuState::SettingsSound  │
      │GameState::Game    │  └───────────────────┘  └──────────────────────────┘
      │MenuState::Disabled│                                                     
      └───────────────────┘

Remade this using labels, and making the diagram tall, rather than wide.

//!          START                                 
//!            │                                   
//! ┌──────────▼──────────┐ ┌─────────────────────┐
//! │  GameState::Splash  │ │   GameState::Game   │
//! │ MenuState::Disabled │ │ MenuState::Disabled │
//! └───┬─────────────────┘ └─▲─────┬─────────────┘
//!     │                     │     │              
//! ┌───┼─────────────────────┼─────┼────────┐     
//! │   │                     │     │        │     
//! │ (timer)               Play  (timer)    │     
//! │   │                     │     │        │     
//! │ ┌─▼─────────────────────┴─────▼─┐      │     
//! │ │     MenuState::Main           ├─Quit─┼─►END
//! │ └─┬───────────────────────────▲─┘      │     
//! │   │                           │        │     
//! │ Settings           BackToMainMenu      │     
//! │   │                           │        │     
//! │ ┌─▼───────────────────────────┴──────┐ │     
//! │ │     MenuState::Settings            │ │     
//! │ └─┬───┬─────────────────────────▲──▲─┘ │     
//! │   │   │                         │  │   │     
//! │   │ SettingsDisplay     BackToSettings │     
//! │   │   │                         │  │   │     
//! │   │ ┌─▼─────────────────────────┴┐ │   │     
//! │   │ │ MenuState::SettingsDisplay │ │   │     
//! │   │ └────────────────────────────┘ │   │     
//! │   │                                │   │     
//! │ SettingsSound                      │   │     
//! │   │                                │   │     
//! │ ┌─▼────────────────────────────────┴─┐ │     
//! │ │     MenuState::SettingsSound       │ │     
//! │ └────────────────────────────────────┘ │     
//! │                                        │     
//! │            GameState::Menu             │     
//! │                                        │     
//! └────────────────────────────────────────┘

Wide diagrams are sometimes soft-wrapped in IDEs (RustRover, for example).

Diagram is available here

https://asciiflow.com/#/share/eJzFVt1KwzAUfpWYK4XBQPCvd46JVxO1vQxIdGEG22ysKWyMvYXsYYZP45OYtiZL16ZJ1cxwYFlOvnPyfeckdAUZTggM4ET8PCWEZbAHY7wkc7G2QnCBYHB1dt5DcClmp5cXYsbJgos%2FCPb7R0CNMLp%2BjIBtIMQqKAA%2B3z%2BsKIUTmy222Vr3OERxsN2JALgV4oUccxIE4SzG6WtJq6SmOfNpSVlDj4To3%2F4hTfFzTMYKbfA5q%2BFgMoqDcHY12goq12v%2Bv2TzcxmqJ%2BnGowF%2FzGlC5id72HtxtYDu85BZ1tODTBtdLEPuYknr2xGmrMpB2ENGuRZ2e3M3rDPwYbLLjQxsD1K79iHhnLJJqgEG%2BOUtmuYq5KJ4yrvH7gDmWP0GQYAZ7deUOqoJWurQVo3CV91WiyCJi2d7Vlx61QpKEo%2FZ%2FfWDQ%2Bam%2BksZ3CJ4MltmyzCiJclwmrFxR%2BzvMvurs0VD13uvS2JGH9ia6%2BA02pHap97usXdCdsv5j1aeBK7h%2Bgu9D%2FUw)

awwsmm · 2024-03-12T13:11:37Z

Tested the version on this branch locally and it works now as it does before. All button colours and actions are as expected.

awwsmm · 2024-03-12T13:20:48Z

Ready for re-review @nicopap , @benfrankel

Thanks in advance!

examples/games/game_menu.rs

…(), renamed column_of_buttons() to button(), 'go to' -> 'to go'

awwsmm · 2024-03-12T21:49:58Z

☝🏻 updated main on my fork and rebased

benfrankel

Looks good to me :)

mockersf · 2024-03-26T07:45:57Z

I'm mostly opposed to those changes. This example is on purpose boilerplate heavy as UI in Bevy is boilerplate heavy, and is a good way to show potential improvements.

Solving it in the example rather than in Bevy is not the good way, and will make it harder to showcase those improvements later. It's also a very good candidate for scene driven UI, and that's made harder by having functions around.

awwsmm · 2024-03-26T16:36:30Z

I'm mostly opposed to those changes. This example is on purpose boilerplate heavy as UI in Bevy is boilerplate heavy, and is a good way to show potential improvements.

Solving it in the example rather than in Bevy is not the good way, and will make it harder to showcase those improvements later. It's also a very good candidate for scene driven UI, and that's made harder by having functions around.

Thanks for your feedback, François. How would you use scene-driven UI here? Is there an example of that in the Bevy repo?

Anyway, here are my counterpoints:

Going through the examples/ "is currently the best way to learn Bevy's features and how to use them", and the recommended method for beginners to learn Bevy. This example is 800+ lines and repetitive. It is not very approachable for beginners. You can read how I (a beginner) struggled with it over 4+ hours here, here, and here.

Not improving this example because that would make future UI improvements appear less impactful is not, in my opinion, a good reason to leave an example in its current state. If anything, that biases any future comparison, because you started with an example which was purposefully not streamlined when it could have been.

Even if you do not wish to accept these changes wholesale, surely there are some sections which you would approve of including in the example, like the diagram (or the functions which return styles, rather than the ones which spawn things). If you let me know which improvements, I'd be happy to extract only those out and trash the other ones.

awwsmm commented Feb 20, 2024

View reviewed changes

nicopap added C-Examples An addition or correction to our examples A-UI Graphical user interfaces, styles, layouts, and widgets labels Feb 20, 2024

nicopap mentioned this pull request Feb 20, 2024

Make the game_menu example even drier awwsmm/bevy#1

Merged

nicopap reviewed Feb 20, 2024

View reviewed changes

nicopap approved these changes Feb 21, 2024

View reviewed changes

benfrankel approved these changes Feb 26, 2024

View reviewed changes

examples/games/game_menu.rs Outdated Show resolved Hide resolved

examples/games/game_menu.rs Outdated Show resolved Hide resolved

examples/games/game_menu.rs Outdated Show resolved Hide resolved

examples/games/game_menu.rs Outdated Show resolved Hide resolved

nicopap added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 26, 2024

awwsmm force-pushed the example-game_menu-DRY branch 2 times, most recently from 2b2df07 to 8775459 Compare March 12, 2024 13:09

benfrankel suggested changes Mar 12, 2024

View reviewed changes

examples/games/game_menu.rs Outdated Show resolved Hide resolved

examples/games/game_menu.rs Outdated Show resolved Hide resolved

examples/games/game_menu.rs Outdated Show resolved Hide resolved

awwsmm and others added 5 commits March 12, 2024 17:49

game_menu example: pull common patterns into methods

ec82521

remove unnecessary &mut / mut

7c91676

Make the game_menu example even drier

905c0d5

add state diagram to docs

3e6d1e7

added new diagram, created const RED, moved default_font() below main…

a110a22

…(), renamed column_of_buttons() to button(), 'go to' -> 'to go'

awwsmm added 3 commits March 12, 2024 17:49

rebased on latest main

05904f6

cargo fmt --all

f6f32df

Ben's comments

4dc25de

awwsmm force-pushed the example-game_menu-DRY branch from 221ef5a to 4dc25de Compare March 12, 2024 21:49

benfrankel approved these changes Mar 12, 2024

View reviewed changes

awwsmm added 4 commits March 21, 2024 22:20

Merge branch 'main' into example-game_menu-DRY

bae4b7b

Merge branch 'main' into example-game_menu-DRY

2ac488f

Merge branch 'main' into example-game_menu-DRY

4b36402

Merge branch 'main' into example-game_menu-DRY

6d492f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

game_menu example: pull common patterns into methods #12008

game_menu example: pull common patterns into methods #12008

awwsmm commented Feb 20, 2024

awwsmm commented Feb 20, 2024

awwsmm Feb 20, 2024

awwsmm Feb 20, 2024

awwsmm Feb 20, 2024

awwsmm Feb 20, 2024

nicopap Feb 20, 2024

awwsmm Feb 20, 2024

awwsmm commented Feb 20, 2024

nicopap left a comment •

edited

Loading

nicopap Feb 20, 2024

nicopap commented Feb 20, 2024

nicopap left a comment

awwsmm commented Feb 21, 2024

benfrankel commented Feb 26, 2024 •

edited

Loading

benfrankel left a comment

nicopap commented Feb 26, 2024

awwsmm commented Mar 12, 2024

awwsmm commented Mar 12, 2024

awwsmm commented Mar 12, 2024

awwsmm commented Mar 12, 2024

benfrankel left a comment

mockersf commented Mar 26, 2024

awwsmm commented Mar 26, 2024

game_menu example: pull common patterns into methods #12008

Are you sure you want to change the base?

game_menu example: pull common patterns into methods #12008

Conversation

awwsmm commented Feb 20, 2024

Objective

Solution

awwsmm commented Feb 20, 2024

awwsmm Feb 20, 2024

Choose a reason for hiding this comment

awwsmm Feb 20, 2024

Choose a reason for hiding this comment

awwsmm Feb 20, 2024

Choose a reason for hiding this comment

awwsmm Feb 20, 2024

Choose a reason for hiding this comment

nicopap Feb 20, 2024

Choose a reason for hiding this comment

awwsmm Feb 20, 2024

Choose a reason for hiding this comment

awwsmm commented Feb 20, 2024

nicopap left a comment • edited Loading

Choose a reason for hiding this comment

nicopap Feb 20, 2024

Choose a reason for hiding this comment

nicopap commented Feb 20, 2024

nicopap left a comment

Choose a reason for hiding this comment

awwsmm commented Feb 21, 2024

benfrankel commented Feb 26, 2024 • edited Loading

benfrankel left a comment

Choose a reason for hiding this comment

nicopap commented Feb 26, 2024

awwsmm commented Mar 12, 2024

awwsmm commented Mar 12, 2024

awwsmm commented Mar 12, 2024

awwsmm commented Mar 12, 2024

benfrankel left a comment

Choose a reason for hiding this comment

mockersf commented Mar 26, 2024

awwsmm commented Mar 26, 2024

nicopap left a comment •

edited

Loading

benfrankel commented Feb 26, 2024 •

edited

Loading