3
\$\begingroup\$

So I've read numerous tutorials on handling game states in XNA, and to each tutorial I've been read, it was different from the others. Now I want to ask which way is more efficient? I've created my own ScreenManager and what not, but it wasn't satisfying to me because it was inefficient since I didn't unload any unused content.

My question is: What is an efficient way to manage your screens and its content? Is using an enum fine? Or...?

Here's my current code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.Xna.Framework;
using Microsoft.Xna.Framework.Content;
using Microsoft.Xna.Framework.Graphics;

namespace Menu
{
    public class ScreenManager
    {
        // Variables
        List<GameScreen> gameScreens = new List<GameScreen>();

        // Constructors
        public ScreenManager()
        {
            gameScreens.Add(new Splashscreen());
            gameScreens.Add(new Mainscreen());
            gameScreens.Add(new Playscreen());
            gameScreens.Add(new Pausescreen());
        }

        // Public Properties
        public List<GameScreen> Screens
        {
            get { return gameScreens; }
            set { gameScreens = value; }
        }

        // Public Methods
        public void Add(GameScreen screen)
        {
            gameScreens.Add(screen);
        }

        public void LoadContent(ContentManager Content)
        {
            foreach (GameScreen screen in gameScreens)
            {
                screen.LoadContent(Content);
            }
        }

        public void Update(GameTime gameTime)
        {
            foreach (GameScreen screen in gameScreens)
            {
                screen.Update(gameTime);
            }
        }

        public void Draw(SpriteBatch spriteBatch)
        {
            switch (ScreenState.Instance.CurrentState)
            {
                case State.Splashscreen:
                    gameScreens[0].Draw(spriteBatch);
                    break;
                case State.Menu:
                    gameScreens[1].Draw(spriteBatch);
                    break;
                case State.Play:
                    gameScreens[2].Draw(spriteBatch);
                    break;
                case State.Pause:
                    gameScreens[3].Draw(spriteBatch);
                    break;
            }
        }
    }
}
\$\endgroup\$
0
2
\$\begingroup\$

I can't be sure of this (I need to foster my XNA knowledge), but I would probably derive that class from DrawableGameComponent so that the Update and Draw methods would be overrides (not just methods that happen to be named "Update" and "Draw"), but deriving from that class might have other consequences that I'm not aware of; again, my XNA is fairly basic - it just seems natural to me for a class that has Update and a Draw methods, to derive from DrawableGameComponent: these method names are not an accident.

Update

Your Update() method is looping through each GameScreen and calling Update() on them all:

foreach (GameScreen screen in gameScreens)
{
    screen.Update(gameTime);
}

I think doing this incurs non-necessary processing: why would you bother updating the splash screen and pause screen at every single iteration? You're wasting cycles here.

Draw

Your Draw() method is more selective:

public void Draw(SpriteBatch spriteBatch)
{
    switch (ScreenState.Instance.CurrentState)
    {
        case State.Splashscreen:
            gameScreens[0].Draw(spriteBatch);
            break;
        case State.Menu:
            gameScreens[1].Draw(spriteBatch);
            break;
        case State.Play:
            gameScreens[2].Draw(spriteBatch);
            break;
        case State.Pause:
            gameScreens[3].Draw(spriteBatch);
            break;
    }
}

I like that you're using an enum here, but I don't like that you've hard-coded the gameScreens indices. Let's see.. you're storing the screens in a List, here:

List<GameScreen> gameScreens = new List<GameScreen>();

public ScreenManager()
{
    gameScreens.Add(new Splashscreen());
    gameScreens.Add(new Mainscreen());
    gameScreens.Add(new Playscreen());
    gameScreens.Add(new Pausescreen());
}

Allow me to open a nitpicking parenthesis here:

Nitpick

Instead of List<GameScreen> gameScreens = new List<GameScreen>(); and then adding the screens in the list at construction, I would have used an interface here, and initialized everything in a single statement:

private readonly IList<GameScreen> _gameScreens;

public ScreenManager()
{
    _gameScreens = new List<GameScreen> 
                           {
                               new SplashScreen(),
                               new MainScreen(),
                               new PlayScreen(),
                               new PauseScreen()
                           };
}

This allows the _gameScreens private field to be readonly, which prevents the list's reference to be tampered with.

The mechanics work, but could work better if you leveraged IDictionary and turned List<GameScreen> gameScreens into a IDictionary<State, Action<SpriteBatch>>:

private readonly IDictionary<State, Action<SpriteBatch>> _gameScreenRenderer;
private readonly GameScreen _splashScreen = new SplashScreen();
private readonly GameScreen _mainScreen = new MainScreen();
private readonly GameScreen _playScreen = new PlayScreen();
private readonly GameScreen _pauseScreen = new PauseScreen();

public ScreenManager()
{
    _gameScreenRenderer = new Dictionary<State, Action<SpriteBatch>>
                            {
                                { State.SplashScreen, sb => _splashScreen.Draw(sb) },
                                { State.MainScreen, sb => _mainScreen.Draw(sb) },
                                { State.PlayScreen, sb => _playScreen.Draw(sb) },
                                { State.PauseScreen, sb => _pauseScreen.Draw(sb) }
                            };
}

An Action<SpriteBatch> is a delegate that points to any method that returns void and takes a SpriteBatch parameter; here the method is anonymous.

Each GameScreen becomes a field of the ScreenManager class, there's no need to have them in a list of any kind. Doing this turns your Draw implementation into something like this:

private void Draw(SpriteBatch spriteBatch)
{
    _gameScreenRenderer[ScreenState.Instance.CurrentState](spriteBatch);
}

Implement a similar mechanism for your Update method and you'll only be updating and rendering the screen for your CurrentState.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thank you so much for pointing out the flaws of this code. Thank you, thank you, thank you. \$\endgroup\$ – Brandon Dec 29 '13 at 0:07
  • 1
    \$\begingroup\$ You're welcome! Thank you for asking here on CodeReview! Come back anytime, there's always someone around here to review C# code :) \$\endgroup\$ – Mathieu Guindon Dec 29 '13 at 3:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.