Putting my game through a public design review.

[Critkeeper]

Introduction:

Studying as a software engineer has taught me that design reviews are crucial to producing good robust designs that can be extended and maintained easily. In the interest of making sure that the games I will be making will share a common robust design, I want to put up this design up for public scrutiny. No design should be totally hooked to a library if the API is clean, but nevertheless I want to make sure I'm using SFML to its maximum potential, and that is a valid design criteria. I can't think of any other community that knows how to use SFML better than this one!

The primary goal of this thread is to either verify that the design doesn't need changes, or hopefully to circulate ideas about how to improve the design so that it can be better. A second goal is to demonstrate how SFML can be used in an application. The design will be communicated in 3 ways: a uml class diagram made with http://www.umlet.com/ attached at the bottom, code, and an explanation to fill in the blanks. Keep in mind that this is still in the early design phase, so much of the code remains unwritten and many methods are empty and it certainly isn't complete enough to compile-- hopefully the explanation should fill in the holes.

"What kind of game" in some ways is a fair question, but in other ways is not. If the design for the game is entirely contingent on the type of the game, then no code can be recycled if you decide to write a different kind of game-- and that is obviously nonsense. I am early enough in the design phase that I am writing code that will be recycled again and again, so it behooves me to make sure the design is robust, extendable, versatile and maintainable.

In order to make sure that subsequent revisions to the design can be made, any time a large revision is made I will update the design and post it as a reply to this thread, and I will edit the post you are reading to include a hyperlink to the latest revision.


UML class diagram:




Code:

Audible:

Code: [Select]

#include "Audible.h"

void
Audible::
play() const
{
    playThis();
    for ( Node* child : children )
    {
        Audible* audible = dynamic_cast<Audible*>( child );
        if ( audible )
            audible->play( target, states );
    }
}

Background:

Code: [Select]

#include "Background.h"

Background::
Stage()
{
}

void
Background::
updateThis( sf::Time dt )
{

}

void
Background::
drawThis( sf::RenderTarget& target, sf::RenderStates states )
{

}

void
Background::
playThis()
{

}

Collidable:

Code: [Select]

void
Collidable::
collide() const
{
    collideThis();
    for ( Node* child : children )
    {
        Collidable* collidable = dynamic_cast<Collidable*>( child );
        if ( collidable )
            collidable->collide();
    }
}


Command:

Code: [Select]

#ifndef COMMAND_H
#define COMMAND_H

#include "SFML/System/Time.hpp"
#include <functional>
#include <list>


template < typename Sender, typename Receiver >
class
Command
{
    public:
        sf::Time
        activate;

        sf::Time
        deactivate;

        sf::Time
        sent;

        sf::Time
        received;

        sf::Time
        started;

        sf::Time
        completed;

        std::function< void (Receiver&, sf::Time) >
        operation;

        Sender*
        sender;

        Receiver*
        receiver;
};

#endif // COMMAND_H

CommandResolver:

Code: [Select]

#include "CommandResolver.h"


void
CommandResolver::
addCommand(Command< Sender, Receiver >*)
{
   
}

void
CommandResolver::
resolveCommands()
{
   
}

Game:

Code: [Select]

#include "Game.h"

Game::
Game()
{
}

void
Game::
attachMode( Mode* mode )
{
    this->attachNode( mode );
}

void
Game::
updateThis( sf::Time dt )
{

}

Mode:

Code: [Select]

#include "Mode.h"

Mode::
Mode()
{
}

void
Mode::
updateThis( sf::Time dt )
{
    for ( Node* child : children )
    {
        Stage* stage = dynamic_cast<Stage*>( child );
        if ( stage )
        {
            stage->draw( target, states );
            stage->play();
        }
    }
}

Node:

Code: [Select]

#include "Node.h"
#include <algorithm>
#include <cassert>
#include <queue>

void
Node::
attachNode( Node* child )
{
    children.push_back( child );
}

Node*
Node::
detachNode( const Node& child )
{
    auto found = std::find_if
    (
        children.begin(),
        children.end(),
        [&] ( Node* p ) -> bool { return p == &child; }
    );
    assert( found != children.end() );

    Node* result = found;
    children.erase( found );
    return result;
}

void
Node::
queueCommand( Command* command )
{
    commands.push( command );
}

void
Node::
update( sf::Time dt )
{
    for( int i = 0; i < commands.size(); i++ )
    {
        commands.front()->operation( this, dt );
        commands.pop();
    }
    updateThis(dt);
    for( const auto& child : children )
        child->update(dt);
}

Renderable:

Code: [Select]

#include "Renderable.h"
#include <SFML/Graphics/Transform.hpp>
#include <SFML/Graphics/RenderStates.hpp>
#include <SFML/Graphics/RenderTarget.hpp>
#include <memory>


void
Renderable::
draw( sf::RenderTarget& target, sf::RenderStates states ) const
{
    transformFromRoot = states.transform *= getTransform();
    drawThis(target, states);
    for ( Node* child : children )
    {
        Renderable* renderable = dynamic_cast<Renderable*>( child );
        if ( renderable )
            renderable->draw( target, states );
    }
}

sf::Vector2f
Renderable::
getDisplacementFromRoot() const
{
    return transformFromRoot * sf::Vector2f();
}

Resources:

Code: [Select]

#ifndef RESOURCES_H_INCLUDED
#define RESOURCES_H_INCLUDED

#include <map>
#include <memory>
#include <cassert>
#include <stdexcept>


/** \brief A template for creating resource management classes.
 *
 *  The resources are handled with shared pointers.
 *  It is possible to keep a resource in memory, even if no shared pointers exist which point to it, by using member keep.
 *  Unless a resource is kept, it will be reloaded distinctly every time it is requested, which can waste memory if there is no intention of modifying the data of that resource.
 *  Therefore using the shared pointer returned by the get member to generate more shared pointers is recommended if the data for the resource does not need to be instanced.
 *  The target user of this class template is other classes, particularly constructors which can make a copy the shared pointer provided by the member get.
 *
 * \param Resource defines the type of the resource to be loaded.
 * \param Identifier defines an alias by which the resource can be referred to.
 *
 */

template < typename Resource, typename Identifier >
class
Resources
{
public:
    void
    keep( Identifier id );

    template < typename Parameter >
    void
    keep( Identifier id, Parameter secondParam );

    void
    unkeep( Identifier id );

    std::shared_ptr< Resource >
    get( Identifier id );

    const std::shared_ptr< Resource >
    get( Identifier id ) const;

    template < typename Parameter >
    std::shared_ptr< Resource >
    get( Identifier id, Parameter secondParam );

    template < typename Parameter >
    const std::shared_ptr< Resource >
    get( Identifier id, Parameter secondParam ) const;

    void
    file( Identifier id, std::string filename );
private:
    std::map< Identifier, std::shared_ptr<Resource> >
    memory;

    std::map< Identifier, std::string filename >
    disk;

    std::shared_ptr< Resource >
    load( Identifier id );

    template < typename Parameter >
    std::shared_ptr< Resource >
    load( Identifier id, const Parameter& secondParam );
};

/** \brief Keeps the resource in memory so that it does not have to be reloaded from the disk when requested even if the last shared pointer to it was previously deleted.
 *
 */

template < typename Resource, typename Identifier >
void
Resources < Resource, Identifier >::
keep( Identifier id )
{
    // do nothing if the resource is already kept, otherwise load into it into memory and store a shared pointer to it
    auto found = memory.find( id );
    if( found != memory.end() )
        return;
    else
        memory[ id ] = load( id );
}

/** \brief Identical to the overloaded method of the same name but additionally passes a second parameter to the loadFromFile member of the resource being loaded.
 *
 */

template < typename Resource, typename Identifier >
template < typename Parameter >
void
Resources < Resource, Identifier >::
keep( Identifier id, Parameter secondParam )
{
    // do nothing if the resource is already kept, otherwise load into it into memory and store a shared pointer to it
    auto found = memory.find( id );
    if( found != memory.end() )
        return;
    else
        memory[ id ] = load( id, secondParam );
}

/** \brief Undoes keep.
 *
 */

template < typename Resource, typename Identifier >
void
Resources < Resource, Identifier >::
unkeep( Identifier id )
{
    // do nothing if the resource is not kept, otherwise remove from memory the shared pointer to it
    auto found = memory.find( id );
    if( found != memory.end() )
        return;
    else
        memory.erase( id );
}

/** \brief Acquires a pointer to the specified resource.
 *
 *  Loads a resource of a type specified by the class template parameter Resource.
 *  The resource must implement a loadFromFile member that takes a string filename as an argument.
 *  The Identifier class template parameter is used to alias the resources, so that they can be referred to and specified efficiently.
 *  Enum class or enum are good identifiers for most purposes, for example.
 *
 *  If the shared pointer is in memory this member will return it.
 *  Otherwise it will load the resource from file and return a brand new shared pointer without storing a copy of the pointer in memory.
 *  Unless an order to keep the resource in memory is made, the resource will be removed from memory the moment the last shared pointer to it is deleted.
 *
 * \param The alias of the resource being requested is id.
 * \return Returns a shared pointer to the specified resource.
 *
 */
template < typename Resource, typename Identifier >
std::shared_ptr< Resource >
Resources < Resource, Identifier >::
get( Identifier id )
{
    auto found = memory.find( id );
    if( found != memory.end() )
        return memory[id];
    else
    {
        // if this fails then a resource was requested without defining where it resides on disk
        found = disk.find( id );
        assert( found != disk.end() );

        return load( id );
    }
}

 /** \brief Identical to the overloaded method of the same name but passes a second parameter to the loadFromFile member of the resource being loaded.
  *
  *  The type of the additional parameter is provided in the function template parameter labeled Parameter.
  *  the value of the additional parameter is provided as an argument labeled secondParam.
  *
  */

template < typename Resource, typename Identifier >
template < typename Parameter >
std::shared_ptr< Resource >
Resources < Resource, Identifier >::
get( Identifier id, secondParam )
{
    auto found = memory.find( id );
    if( found != memory.end() )
        return memory[id];
    else
    {
        // if this fails then a resource was requested without defining where it resides on disk
        found = disk.find( id );
        assert( found != disk.end() );

        return load <Parameter>( id, secondParam );
    }
}

/** \brief Identical to the overloaded method of the same name but with different qualifiers
 *
 */

template < typename Resource, typename Identifier >
std::shared_ptr< Resource >
Resources < Resource, Identifier >::
get( Identifier id ) const
{
    auto found = memory.find( id );
    if( found != memory.end() )
        return memory[id];
    else
    {
        // if this fails then a resource was requested without defining where it resides on disk
        found = disk.find( id );
        assert( found != disk.end() );

        return load( id );
    }
}

/** \brief Identical to the overloaded method of the same name but with different qualifiers
 *
 */

template < typename Resource, typename Identifier >
template < typename Parameter >
std::shared_ptr< Resource >
Resources < Resource, Identifier >::
get( Identifier id, secondParam ) const
{
    auto found = memory.find( id );
    if( found != memory.end() )
        return memory[id];
    else
    {
        // if this fails then a resource was requested without defining where it resides on disk
        found = disk.find( id );
        assert( found != disk.end() );

        return load<Parameter>( id, secondParam );
    }
}

template < typename Resource, typename Identifier >
std::shared_ptr< Resource >
Resources < Resource, Identifier >::
load( Identifier id )
{
    std::shared_ptr< Resource > resource( new Resource() );

    // if this fails then a resource was requested that is not accessible on disk
    if( not resource->loadFromFile( disk[id] ) )
        throw std::runtime_error( "Resources::load - Failed to load " + disk[id] );
}

// Specifically passes a second parameter of type Parameter to resource->loadFromFile
// the type of the resource is provided in the class template and must implement two argument loadFromFile
template < typename Resource, typename Identifier >
template < typename Parameter >
std::shared_ptr< Resource >
Resources < Resource, Identifier >::
load( Identifier id, const Parameter& secondParam )
{
    std::shared_ptr< Resource > resource( new Resource() );

    // if this fails then a resource was requested that is not accessible on disk
    if( not resource->loadFromFile( disk[id], secondParam ) )
        throw std::runtime_error( "Resources::load - Failed to load " + filename );
}

template < typename Resource, typename Identifier >
void
Resources < Resource, Identifier >::
file( Identifier id, std::string filename )
{
    disk[id] = filename;
}
#endif // RESOURCES_H_INCLUDED


Stage:

Code: [Select]

#include "Stage.h"
#include <SFML/Graphics/RenderStates.hpp>
#include <SFML/Graphics/RenderTarget.hpp>

Stage::
Stage()
{
}

void
Stage::
attachBackground( Background* background )
{
    this->attachNode( background );
}

void
Stage::
attachCollidable( Collidable* collidable )
{
    this->attachNode( collidable );
}

void
Stage::
updateThis( sf::Time dt )
{

}

void
Stage::
drawThis( sf::RenderTarget& target, sf::RenderStates states ) const
{

}

void
Stage::
playThis() const
{

}

Vessel:

Code: [Select]

#include "Vessel.h"
#include <SFML/Graphics/RenderStates.hpp>
#include <SFML/Graphics/RenderTarget.hpp>

Vessel::Vessel()
{
}

void
Vessel::
updateThis( sf::Time dt )
{

}

void
Vessel::
drawThis( sf::RenderTarget& target, sf::RenderStates states ) const
{

}

void
Vessel::
playThis() const
{

}



Explanation:

One major design criteria is the amount of effort required to make large structural changes to the game. I would strongly prefer to have a design that allows me to make sweeping structural changes with little effort. One way to do this is to use a sceneGraph type pattern to ensure that the structure of the game is reflected by a data structure, and is the pattern I have chosen to adapt.

A concrete example of what I mean follows. From a top down point of view I want to be able to erect the framework for a game by saying something like the following:

Code: [Select]

Root root = new Root();
Game shmup = new Game( /* parameters */ );
Mode singlePlayer = new Mode();
Mode pause = new Mode();
Vessel player = new Vessel();
/* etc */

root.attachGame( shmup );
shmup.attachMode( singlePlayer );
singlePlayer.attachStage( firstStage );
firstStage.attachBackground( space );
background.attachSpecialEffects( twinklingStars);
firstStage.attachCollidable( enemy );
firstStage.attachCollidable( player );
player.attachTurret( laser );
player.attachTurret( missiles );
/* more stuff for the first stage */
singlePlayer.attachStage( secondStage );
/* stuff for the second stage */
/* etc */
shmup.attachMode( pause );
pause.attachStage( pauseMainScreen );
statScreen.attachBackground( halfTransparent );
statScreen.attachCollidable( cursor );
statScreen.attachCollidable( inventoryTab );
statScreen.attachCollidable( questsTab );
/* etc */
statScreen.attachCollidable( unpauseButton);
pause.attachStage( pauseInventoryScreen );
/* stuff for the inventory screen */
pause.attachStage( pauseQuestsScreen );
/* stuff for the quests screen */
/* and so on */

Then I would simple call update on the root node with root.update().

The update will propagate through the child nodes as with most scene graphs. Unlike other implementations, in my design the entire functionality of the game must be captured by the structure of the sceneGraph and the messages which are passed between the elements within the graph. Messages in my game are simply commands.

A command can be followed or it can be ignored. It is up to the receiver of the command to ignore the message or follow the message. For example, a bomb may damage everything in an area so it broadcasts a command to all nearby entities by pushing those commands into the entity with addCommand( blowUp ). If an player is nearby and recently stepped on spikes and is invulnerable, it would simply ignore the command explicitly since it knows about this command by name.

One point of fair criticism is that the player entity would have to know about this command by name, and so it defeats the point of encapsulation. However, the underlying player class itself is still encapsulated and so is the bomb. The command itself is defined using functional members that belong to the player, namely takeDamage( whatever ). So encapsulation is actually preserved, but the structure of the relationships between the classes are defined by HOW the commands are used.

By ensuring that the structure of the relationships between all the entities in the game isn't hard coded by inheritance or object oriented methodologies, I can quickly alter those relationships by changing only a few lines of code-- namely what command is being broadcast or ignored and under what condition. The design cost is that I must predefine what commands exist-- in effect a command becomes an atomic entity that serves a syntactic purpose, like a basic operation.

This has a tradeoff. I gain the ability to define the language in which i want to "explain" to the compiler what my game is and what its rules are, the language's tokens are the names of the commands. I lose readability because anyone reading the program must also understand the new language I have created.


Design Revisions:

None yet!

[eXpl0it3r]

I wish you good luck finding someone who will actually "review" this huge amount of information. It will probably take that person equally long to understand it as it took you to create it.

The most important advice I can give you is: Write Games, Not Engine
Writing and planing an engine can be an interesting project, however if your goal is to create games, you should start by writing games instead of an huge engine which will never really fit the games you later on want to write with it.

Studying as a software engineer has taught me...

Big design plans etc. are very useful when working in a team, since everyone will be up to date and work load can be split onto different teams. Having an overview of your possible classes and can be useful, but it also often leads to over engineering. If your class X receives a breaking API change, you don't have to inform three different teams about it, but instead you can simply apply the change everywhere.
An often seen practice in game/software development in small teams is KISS (Keep It Simple Stupid) and YAGNI (You Ain't Gonna Need It). Implement things in the most simple way and only implement what you really need.

If you're writing your first game or your first bigger game, you usually lack the experience of what you actually need and which part can/should be generalized. When thinking about all theoretically you'll make a lot of wrong assumptions and end up having to change a lot of your initial plan.

No design should be totally hooked to a library if the API is clean

That's in my experience often a wrong assumption. There's rarely any gain in abstracting everything away. Sure if you do a very rough design it doesn't matter, you'd have just very vague description of what happens, but once you start writing actual interfaces, there's no reason to hide what's being used.


I suggest you take a look at Tank's Game Development Design articles. Especially for your command pattern you might find the message bus article interesting.

[Spirro]

I agree a lot with eXploit3r on this one.  I did read through your post text, but skipped a lot of the diagram as, imo, it's way overly thought out for something that hasn't even begin.  Planning is important, but to me you are trying to plan in to specific a manor and by doing so will end up hurting yourself by trying to live up to your plans and then saying the hell with it and stopping.

Thing I've done have been incremental, but I'm looking to expand soon so I am definately planning, but not in the detail you are.  While looking up resources I did come across something interesting that I think may apply here and others may think is noteworthy.  There currently is a AAA programmer developing a game and he is broadcasting it's development(via twitch) and archiving feeds(via youtube).  I will be crossplatform, but currently is windows only.  It's very interesting to me for many reasons, but a couple of big ones is he does Q&A before and after live feeds.  He's given rough outlines for the project, which he started November of last year and will take the rest of this year and a little more to complete in his estimation.

The site you can find information about the project is https://handmadehero.org/.  There are project outlines and archives up to present of the video feeds.  His planning on the project is really very basic, but in practice he is doing some nice techniques that I have learned a bit from.  It may be worth you looking at a bit as well to see how your planning is vs his.  I only say this because his experience is listed and it's quite extensive.

[Critkeeper]

Thankyou both for your contributions. I will change my schedule to set aside a couple of hours each day for the next couple of weeks in order to read the Optank and watch the Handmade hero and I am certain I will learn many new things that will invigorate me and help me to self-review with new insights.