Am I setting up my game in the ideal way? (ie: pointers/.cpp/.header files)

[HailBee]

Just wanted to make sure im setting up my game in the ideal way.

From what I've learnt at Uni the last semester I figure using classes in separate .cpp/header files to get/set sprites is the way to go but any critique about my implementation methods is very welcomed!

Main Game Loop:

#include "SFML/Graphics.hpp"
#include "cmath"
#include "kf/kf_log.h"
#include "qgf2d/system.h"
#include "qgf2d/anim.h"
#include "hayleigh/player.h"
#include "hayleigh/levels.h"
#include "hayleigh/text.h"

using namespace std;
using namespace qgf;
using namespace kf;
using namespace sf;

int main()
{
        initDirectory();
        Log::getDefault().addCout();
        Log::getDefault().addFile("base.log");
        kf_log("Start Tute 2 program.");
        RenderWindow window(VideoMode(800, 600, 32), "Hayleigh_Blair GAM201 - Tutorial 2 - Animation");
        sf::Clock clock; //Setup delta time clock
        window.setMouseCursorVisible(false); //Hide default cursor

//Create Player
        Player *aPlayer = new Player();
        aPlayer->SetSprite();
        aPlayer->SetAnim();

//Load Levels
        Levels *theLevel = new Levels();
        theLevel->SetLevel1();

//Load Text
        TextHandler *printText = new TextHandler();
        printText->SetFont();

//Begin Game Loop
        while (window.isOpen())
        {
                float deltaT = clock.restart().asSeconds(); //reset clock, deltaT = clock data 'as Seconds'.
                aPlayer->Update();
                printText->BlendText(); //Fade in/out alpha & Color range
                theLevel->HandleCursor(window); //Draw circle to cursor

                Event ev;
                while (window.pollEvent(ev))
                {
               
                        if ((ev.type == Event::Closed) || ((ev.type == Event::KeyPressed) && (ev.key.code == Keyboard::Escape)))
                        {
                                window.close();
                                break;
                        }

                }

//Play Idle Animation
        aPlayer->Idle();

//Detect Input for aPlayer
        aPlayer->move();       

//Draw to Window
                window.clear();
                window.draw(theLevel->GetBG());
                window.draw(theLevel->GetBGfill());
                window.draw(theLevel->GetShapes());
                window.draw(aPlayer->GetSprite());
                window.draw(printText->GetLevelName(), sf::BlendAlpha);
                window.draw(theLevel->GetCirc());
                window.display();
        }
        return 0;
}

Player .cpp

#include "hayleigh/player.h"
//Get Sprite (used by window.draw)
sf::Sprite Player::GetSprite() const
        {
                return playSprite;
        };
//Set Sprite
void Player::SetSprite()
        {
        float speed_y = 0;
        player = onGround;
        playTexture.loadFromFile("data/gabe.png");
        playSprite = sf::Sprite(playTexture);
        playSprite.setPosition(sf::Vector2f(100,500));
        playSprite.setOrigin(23, 63);
        };     
//Set Animation
void Player::SetAnim()
{
        playAnim = qgf::Anim (&playSprite, 0, 0, 46, 64, 6, 2);
        playAnim.addSequence("idle", 0, 5, 2.0f, true);  
        playAnim.addSequence("run", 6, 11, 0.8f, true);
}
//Start Idle animation
void Player::Idle()
{
        playAnim.play("idle", false);
        playAnim.update(deltaT);
}
//Update(Sync delta)
void Player::Update()
        {
                deltaT = clock.restart().asSeconds();
        }
//Update animation
void Player::UpdateAnim()
        {
                playAnim.play("run", false);
                playAnim.update(deltaT);
        }
//Move Player
void Player::move()
        {
        //Move Left
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::A))
                        {
                        kf::Vector2 position = playSprite.getPosition();
                        playSprite.setScale(-1,1);
                        position.x -= 120.0f * deltaT;
                        UpdateAnim();
                        playSprite.setPosition(position);
                        }
        //Move Right
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::D))
                        {
                        kf::Vector2 position = playSprite.getPosition();
                        playSprite.setScale(1,1);
                        position.x += 120.0f * deltaT;
                        UpdateAnim();
                        playSprite.setPosition(position);
                        }
        //Jump
                switch(player)
                {
                        case onGround:
                        if (sf::Keyboard::isKeyPressed(sf::Keyboard::Space))
                                {
                                       
                                        kf::Vector2 position = playSprite.getPosition();
                                        if (position.y <= 460)
                                        {
                                                player = falling;
                                        }
                                        else if (position.y > 460)
                                        {
                                        position.y -= 120.0f * deltaT;
                                        UpdateAnim();
                                        playSprite.setPosition(position);
                                        }
                                        break;
                                }
                        case falling:
                                        kf::Vector2 position = playSprite.getPosition();
                                        if(position.y > 500)
                                                {
                                                player = onGround;
                                                }
                                        else if(position.y < 500)
                                                {
                                                position.y += 120.0f * deltaT;
                                                UpdateAnim();
                                                playSprite.setPosition(position);
                                                }
                                        break;
                }      
        }
 

Player Header

#ifndef PLAYER_H
#define PLAYER_H

#include "SFML/Graphics.hpp"
#include "cmath"
#include "qgf2d/system.h"
#include "qgf2d/anim.h"

class Player
{
public:
        sf::Sprite Player::GetSprite() const;
        void Player::SetSprite();
        void Player::Update();
        void Player::move();
        void Player::Idle();
        void Player::SetAnim();
        void Player::UpdateAnim();
         
private:
         float deltaT;
         enum playerState {onGround, falling} player;
         sf::Texture playTexture;
         sf::Sprite playSprite;
         sf::Clock clock;
         qgf::Anim playAnim;
};

#endif PLAYER_H

[The Hatchet]

Well, everyone codes differently but I will say this:

1.  using multiple namespaces is a BAD idea.  if you have multiple libraries with the same class or method and have declared both libraries as a namespace you are gonna have bad conflicts.

2.  in your header file there is no need for the Player:: before the methods

3.  your GetSprite() function is returning a full copy of the sprite and not a pointer to that specific sprite object.

4.  Using 'new' without using 'delete' is bad, bad, bad, bad, bad.  If you don't 'delete' any 'new' you've done that memory doesn't get freed up and you have memory leaks.  Basically you want to avoid using 'new' as much as you can and if you have to use 'new' then use c++11 <unique_ptr> (smart pointers) since you don't have to call 'delete' when it goes out of scope.  If you can't compile with c++11 features and absolutely have to use 'new' then you darn well make sure to 'delete' every single 'new' instance of the object you make.

[HailBee]

Well, everyone codes differently but I will say this:

1.  using multiple namespaces is a BAD idea.  if you have multiple libraries with the same class or method and have declared both libraries as a namespace you are gonna have bad conflicts.

2.  in your header file there is no need for the Player:: before the methods

3.  your GetSprite() function is returning a full copy of the sprite and not a pointer to that specific sprite object.

4.  Using 'new' without using 'delete' is bad, bad, bad, bad, bad.  If you don't 'delete' any 'new' you've done that memory doesn't get freed up and you have memory leaks.  Basically you want to avoid using 'new' as much as you can and if you have to use 'new' then use c++11 <unique_ptr> (smart pointers) since you don't have to call 'delete' when it goes out of scope.  If you can't compile with c++11 features and absolutely have to use 'new' then you darn well make sure to 'delete' every single 'new' instance of the object you make.

Awesome thanks for the feedback!! In response,

1. My tutor had our template SFML project setup with these but ill get rid of them as I agree ill end up running into problems and namespaces are really just lazy coding.

2. Good to know I dont need 'Player::' , I think that ended up there from copy/pasting functions from the .cpp to quickly reference in the header.

3. That's interesting and im guessing waste of memory so ill look at referencing the sprite instead of creating a new one.

4. Good to know, as we have to use Visual Studio 2010 edition(Uni has yet to upgrade =P) ill add some delete calls in the 'escape/close window event'

[OniLinkPlus]

4. Good to know, as we have to use Visual Studio 2010 edition(Uni has yet to upgrade =P) ill add some delete calls in the 'escape/close window event'

Don't even do that. Just flat-out stop using new. Unless you are working with low-level programming ala driver-space or kernel-space, the odds you will need to use new/delete is 0.000001%. Every time you use it here is unnecessary and BAD BAD BAD.

[kloffy]

1.  using multiple namespaces is a BAD idea.  if you have multiple libraries with the same class or method and have declared both libraries as a namespace you are gonna have bad conflicts.

1. My tutor had our template SFML project setup with these but ill get rid of them as I agree ill end up running into problems and namespaces are really just lazy coding.

Just to make this clear in case it was not: The Hatchet was referring to the "using namespace"-statements, and not the namespaces themselves. Namespaces are a good practice.

[The Hatchet]

That is indeed what I meant, using multiple libraries and namespaces is fine, but declaring "using namespace xxx" for multiple namespaces is lots of trouble.  I don't even use "using namespace std" just so I don't fall into the habit as it can lead to lazy coding.

A little extra typing now can save a lot of big headaches later

[MorleyDev]

My first concern is the overuse of comments, and the size of the functions. Comments mean the code doesn't explain itself adequately, so really should only be used in situations where you've had to make a weird decision, to explain that weird decision.

The current placement of the comments also serves as a good indicator where the main could be broken down into smaller and easier to understand functions. Remember, whilst a good coder can keep the entire system they've written in their head, a great coder writes the system in such a way that they don't need to.

Also you can scope using namespace statements to a function call, which personally I recommend if you insist on not just writing std:: and similar (which is what I'd recommend first), to limit the declaring "using namespace xxx" problem.

A few more things, I'd say the player knows too much. Namely: It knows when the keyboard is telling it to move itself.  I'd recommend breaking that logic out of the player, and having something else tell the when player to move.