From ca80fe65eddffd554bd297581c31c7e936a87b6e Mon Sep 17 00:00:00 2001
From: Rostyslav Toch <RokaRostuk@gmail.com>
Date: Wed, 8 Dec 2021 20:09:53 +0000
Subject: [PATCH] Improve contributors example quality (#3258)

# Objective

Fixes #3181

## Solution

Refactored `contributors.rs` example:
- Renamed unclear variables
- Split setup system into two separate systems


Co-authored-by: CrazyRoka <rokarostuk@gmail.com>
---
 examples/2d/contributors.rs | 206 ++++++++++++++++++++++--------------
 1 file changed, 124 insertions(+), 82 deletions(-)

diff --git a/examples/2d/contributors.rs b/examples/2d/contributors.rs
index 74040446aadbe..1cd9a99eaeb7f 100644
--- a/examples/2d/contributors.rs
+++ b/examples/2d/contributors.rs
@@ -1,14 +1,15 @@
-use bevy::prelude::*;
+use bevy::{prelude::*, utils::HashSet};
 use rand::{prelude::SliceRandom, Rng};
 use std::{
-    collections::BTreeSet,
-    io::{BufRead, BufReader},
+    env::VarError,
+    io::{self, BufRead, BufReader},
     process::Stdio,
 };
 
 fn main() {
     App::new()
         .add_plugins(DefaultPlugins)
+        .add_startup_system(setup_contributor_selection)
         .add_startup_system(setup)
         .add_system(velocity_system)
         .add_system(move_system)
@@ -17,7 +18,8 @@ fn main() {
         .run();
 }
 
-type Contributors = BTreeSet<String>;
+// Store contributors in a collection that preserves the uniqueness
+type Contributors = HashSet<String>;
 
 struct ContributorSelection {
     order: Vec<(String, Entity)>,
@@ -52,19 +54,25 @@ const ALPHA: f32 = 0.92;
 
 const SHOWCASE_TIMER_SECS: f32 = 3.0;
 
-fn setup(
+const CONTRIBUTORS_LIST: &[&str] = &["Carter Anderson", "And Many More"];
+
+fn setup_contributor_selection(
     mut commands: Commands,
     asset_server: Res<AssetServer>,
     mut materials: ResMut<Assets<ColorMaterial>>,
 ) {
-    let contribs = contributors();
+    // Load contributors from the git history log or use default values from
+    // the constant array. Contributors must be unique, so they are stored in a HashSet
+    let contribs = contributors().unwrap_or_else(|_| {
+        CONTRIBUTORS_LIST
+            .iter()
+            .map(|name| name.to_string())
+            .collect()
+    });
 
     let texture_handle = asset_server.load("branding/icon.png");
 
-    commands.spawn_bundle(OrthographicCameraBundle::new_2d());
-    commands.spawn_bundle(UiCameraBundle::default());
-
-    let mut sel = ContributorSelection {
+    let mut contributor_selection = ContributorSelection {
         order: vec![],
         idx: 0,
     };
@@ -82,7 +90,7 @@ fn setup(
 
         let transform = Transform::from_xyz(pos.0, pos.1, 0.0);
 
-        let e = commands
+        let entity = commands
             .spawn()
             .insert_bundle((
                 Contributor { hue },
@@ -107,10 +115,17 @@ fn setup(
             })
             .id();
 
-        sel.order.push((name, e));
+        contributor_selection.order.push((name, entity));
     }
 
-    sel.order.shuffle(&mut rnd);
+    contributor_selection.order.shuffle(&mut rnd);
+
+    commands.insert_resource(contributor_selection);
+}
+
+fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
+    commands.spawn_bundle(OrthographicCameraBundle::new_2d());
+    commands.spawn_bundle(UiCameraBundle::default());
 
     commands.spawn_bundle((SelectTimer, Timer::from_seconds(SHOWCASE_TIMER_SECS, true)));
 
@@ -145,25 +160,23 @@ fn setup(
             },
             ..Default::default()
         });
-
-    commands.insert_resource(sel);
 }
 
 /// Finds the next contributor to display and selects the entity
 fn select_system(
     mut materials: ResMut<Assets<ColorMaterial>>,
-    mut sel: ResMut<ContributorSelection>,
-    mut dq: Query<&mut Text, With<ContributorDisplay>>,
-    mut tq: Query<&mut Timer, With<SelectTimer>>,
-    mut q: Query<(&Contributor, &Handle<ColorMaterial>, &mut Transform)>,
+    mut contributor_selection: ResMut<ContributorSelection>,
+    mut text_query: Query<&mut Text, With<ContributorDisplay>>,
+    mut timer_query: Query<&mut Timer, With<SelectTimer>>,
+    mut query: Query<(&Contributor, &Handle<ColorMaterial>, &mut Transform)>,
     time: Res<Time>,
 ) {
     let mut timer_fired = false;
-    for mut t in tq.iter_mut() {
-        if !t.tick(time.delta()).just_finished() {
+    for mut timer in timer_query.iter_mut() {
+        if !timer.tick(time.delta()).just_finished() {
             continue;
         }
-        t.reset();
+        timer.reset();
         timer_fired = true;
     }
 
@@ -171,26 +184,38 @@ fn select_system(
         return;
     }
 
-    let prev = sel.idx;
+    let prev = contributor_selection.idx;
 
-    if (sel.idx + 1) < sel.order.len() {
-        sel.idx += 1;
+    if (contributor_selection.idx + 1) < contributor_selection.order.len() {
+        contributor_selection.idx += 1;
     } else {
-        sel.idx = 0;
+        contributor_selection.idx = 0;
     }
 
     {
-        let (_, e) = &sel.order[prev];
-        if let Ok((c, handle, mut tr)) = q.get_mut(*e) {
-            deselect(&mut *materials, handle.clone(), c, &mut *tr);
+        let (_, entity) = &contributor_selection.order[prev];
+        if let Ok((contributor, handle, mut transform)) = query.get_mut(*entity) {
+            deselect(
+                &mut *materials,
+                handle.clone(),
+                contributor,
+                &mut *transform,
+            );
         }
     }
 
-    let (name, e) = &sel.order[sel.idx];
-
-    if let Ok((c, handle, mut tr)) = q.get_mut(*e) {
-        if let Some(mut text) = dq.iter_mut().next() {
-            select(&mut *materials, handle, c, &mut *tr, &mut *text, name);
+    let (name, entity) = &contributor_selection.order[contributor_selection.idx];
+
+    if let Ok((contributor, handle, mut transform)) = query.get_mut(*entity) {
+        if let Some(mut text) = text_query.iter_mut().next() {
+            select(
+                &mut *materials,
+                handle,
+                contributor,
+                &mut *transform,
+                &mut *text,
+                name,
+            );
         }
     }
 }
@@ -199,20 +224,25 @@ fn select_system(
 /// bring the object to the front and display the name.
 fn select(
     materials: &mut Assets<ColorMaterial>,
-    mat_handle: &Handle<ColorMaterial>,
-    cont: &Contributor,
-    trans: &mut Transform,
+    material_handle: &Handle<ColorMaterial>,
+    contributor: &Contributor,
+    transform: &mut Transform,
     text: &mut Text,
     name: &str,
 ) -> Option<()> {
-    let mat = materials.get_mut(mat_handle)?;
-    mat.color = Color::hsla(cont.hue, SATURATION_SELECTED, LIGHTNESS_SELECTED, ALPHA);
+    let material = materials.get_mut(material_handle)?;
+    material.color = Color::hsla(
+        contributor.hue,
+        SATURATION_SELECTED,
+        LIGHTNESS_SELECTED,
+        ALPHA,
+    );
 
-    trans.translation.z = 100.0;
+    transform.translation.z = 100.0;
 
     text.sections[0].value = "Contributor: ".to_string();
     text.sections[1].value = name.to_string();
-    text.sections[1].style.color = mat.color;
+    text.sections[1].style.color = material.color;
 
     Some(())
 }
@@ -221,24 +251,29 @@ fn select(
 /// the object to the back.
 fn deselect(
     materials: &mut Assets<ColorMaterial>,
-    mat_handle: Handle<ColorMaterial>,
-    cont: &Contributor,
-    trans: &mut Transform,
+    material_handle: Handle<ColorMaterial>,
+    contributor: &Contributor,
+    transform: &mut Transform,
 ) -> Option<()> {
-    let mat = materials.get_mut(mat_handle)?;
-    mat.color = Color::hsla(cont.hue, SATURATION_DESELECTED, LIGHTNESS_DESELECTED, ALPHA);
+    let material = materials.get_mut(material_handle)?;
+    material.color = Color::hsla(
+        contributor.hue,
+        SATURATION_DESELECTED,
+        LIGHTNESS_DESELECTED,
+        ALPHA,
+    );
 
-    trans.translation.z = 0.0;
+    transform.translation.z = 0.0;
 
     Some(())
 }
 
 /// Applies gravity to all entities with velocity
-fn velocity_system(time: Res<Time>, mut q: Query<&mut Velocity>) {
+fn velocity_system(time: Res<Time>, mut velocity_query: Query<&mut Velocity>) {
     let delta = time.delta_seconds();
 
-    for mut v in q.iter_mut() {
-        v.translation += Vec3::new(0.0, GRAVITY * delta, 0.0);
+    for mut velocity in velocity_query.iter_mut() {
+        velocity.translation += Vec3::new(0.0, GRAVITY * delta, 0.0);
     }
 }
 
@@ -248,78 +283,85 @@ fn velocity_system(time: Res<Time>, mut q: Query<&mut Velocity>) {
 /// velocity. On collision with the ground it applies an upwards
 /// force.
 fn collision_system(
-    wins: Res<Windows>,
-    mut q: Query<(&mut Velocity, &mut Transform), With<Contributor>>,
+    windows: Res<Windows>,
+    mut query: Query<(&mut Velocity, &mut Transform), With<Contributor>>,
 ) {
     let mut rnd = rand::thread_rng();
 
-    let win = wins.get_primary().unwrap();
+    let window = windows.get_primary().unwrap();
 
-    let ceiling = win.height() / 2.;
-    let ground = -(win.height() / 2.);
+    let ceiling = window.height() / 2.;
+    let ground = -(window.height() / 2.);
 
-    let wall_left = -(win.width() / 2.);
-    let wall_right = win.width() / 2.;
+    let wall_left = -(window.width() / 2.);
+    let wall_right = window.width() / 2.;
 
-    for (mut v, mut t) in q.iter_mut() {
-        let left = t.translation.x - SPRITE_SIZE / 2.0;
-        let right = t.translation.x + SPRITE_SIZE / 2.0;
-        let top = t.translation.y + SPRITE_SIZE / 2.0;
-        let bottom = t.translation.y - SPRITE_SIZE / 2.0;
+    for (mut velocity, mut transform) in query.iter_mut() {
+        let left = transform.translation.x - SPRITE_SIZE / 2.0;
+        let right = transform.translation.x + SPRITE_SIZE / 2.0;
+        let top = transform.translation.y + SPRITE_SIZE / 2.0;
+        let bottom = transform.translation.y - SPRITE_SIZE / 2.0;
 
         // clamp the translation to not go out of the bounds
         if bottom < ground {
-            t.translation.y = ground + SPRITE_SIZE / 2.0;
+            transform.translation.y = ground + SPRITE_SIZE / 2.0;
             // apply an impulse upwards
-            v.translation.y = rnd.gen_range(700.0..1000.0);
+            velocity.translation.y = rnd.gen_range(700.0..1000.0);
         }
         if top > ceiling {
-            t.translation.y = ceiling - SPRITE_SIZE / 2.0;
+            transform.translation.y = ceiling - SPRITE_SIZE / 2.0;
         }
         // on side walls flip the horizontal velocity
         if left < wall_left {
-            t.translation.x = wall_left + SPRITE_SIZE / 2.0;
-            v.translation.x *= -1.0;
-            v.rotation *= -1.0;
+            transform.translation.x = wall_left + SPRITE_SIZE / 2.0;
+            velocity.translation.x *= -1.0;
+            velocity.rotation *= -1.0;
         }
         if right > wall_right {
-            t.translation.x = wall_right - SPRITE_SIZE / 2.0;
-            v.translation.x *= -1.0;
-            v.rotation *= -1.0;
+            transform.translation.x = wall_right - SPRITE_SIZE / 2.0;
+            velocity.translation.x *= -1.0;
+            velocity.rotation *= -1.0;
         }
     }
 }
 
 /// Apply velocity to positions and rotations.
-fn move_system(time: Res<Time>, mut q: Query<(&Velocity, &mut Transform)>) {
+fn move_system(time: Res<Time>, mut query: Query<(&Velocity, &mut Transform)>) {
     let delta = time.delta_seconds();
 
-    for (v, mut t) in q.iter_mut() {
-        t.translation += delta * v.translation;
-        t.rotate(Quat::from_rotation_z(v.rotation * delta));
+    for (velocity, mut transform) in query.iter_mut() {
+        transform.translation += delta * velocity.translation;
+        transform.rotate(Quat::from_rotation_z(velocity.rotation * delta));
     }
 }
 
+enum LoadContributorsError {
+    IO(io::Error),
+    Var(VarError),
+    Stdout,
+}
+
 /// Get the names of all contributors from the git log.
 ///
 /// The names are deduplicated.
 /// This function only works if `git` is installed and
 /// the program is run through `cargo`.
-fn contributors() -> Contributors {
-    let manifest_dir = std::env::var("CARGO_MANIFEST_DIR")
-        .expect("This example needs to run through `cargo run --example`.");
+fn contributors() -> Result<Contributors, LoadContributorsError> {
+    let manifest_dir = std::env::var("CARGO_MANIFEST_DIR").map_err(LoadContributorsError::Var)?;
 
     let mut cmd = std::process::Command::new("git")
         .args(&["--no-pager", "log", "--pretty=format:%an"])
         .current_dir(manifest_dir)
         .stdout(Stdio::piped())
         .spawn()
-        .expect("`git` needs to be installed.");
+        .map_err(LoadContributorsError::IO)?;
 
-    let stdout = cmd.stdout.take().expect("`Child` should have a stdout.");
+    let stdout = cmd.stdout.take().ok_or(LoadContributorsError::Stdout)?;
 
-    BufReader::new(stdout)
+    let contributors = BufReader::new(stdout)
         .lines()
         .filter_map(|x| x.ok())
-        .collect()
+        .collect();
+
+    Ok(contributors)
 }