Player Movement with Classes.

[guitarmatt99]

Hello, I am still learning SFML and have created a basic program where you can move a character around the screen with an animation. You can also sprint which also speeds up the animation!

I wanted to ask if I have done everything correctly and you can correct me if not.

main.cpp

#include <SFML/Graphics.hpp>
#include <iostream>
#include "player.h"

int main()
{
    //Window
    sf::RenderWindow window(sf::VideoMode(1200, 700), "Testing");
    window.setKeyRepeatEnabled(false);

    //Load Player Texture
    sf::Texture playerTexture;
    if(!playerTexture.loadFromFile("IMAGE/playerSprite.png"))
        std::cout << "Texture Error" << std::endl;

    //OBJECTS
    Player player(playerTexture); //Player

    //Game Loop
    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        window.clear(); //Clear Window

        //PLAYER MOVEMENT
        if (sf::Keyboard::isKeyPressed(sf::Keyboard::W)) //Move Up
            player.moveUp();

        else if(sf::Keyboard::isKeyPressed(sf::Keyboard::S)) //Move Down
            player.moveDown();

        else if(sf::Keyboard::isKeyPressed(sf::Keyboard::D)) //Move Right
            player.moveRight();

        else if(sf::Keyboard::isKeyPressed(sf::Keyboard::A)) //Move Left
            player.moveLeft();

        //SPRINT
        if(sf::Keyboard::isKeyPressed(sf::Keyboard::LShift))
            player.setSpeed(0.35, sf::milliseconds(50));
        else
            player.setSpeed(0.20, sf::milliseconds(80));

        window.draw(player.getSprite()); //Draw Player Sprite
        window.display(); //Display Window
    }

    return 0;
}
 

player.h

#ifndef PLAYER_H
#define PLAYER_H

class Player
{
    public:
        Player(); //Constructor
        Player(sf::Texture&); //Overload Constructor
        ~Player(); //Destructor

        //PLAYER MOVEMENT FUNCTIONS
        void moveUp();
        void moveDown();
        void moveRight();
        void moveLeft();
        void setSpeed(float, sf::Time);

        //ACCESSOR FUNCTIONS
        sf::Sprite getSprite() const;

    private:
        sf::Sprite _Sprite; //Declare Player Sprite
        sf::Vector2i _Source; //Declare Source (Sprite Sheet Crop)
        enum _Direction{ Down, Left, Right, Up }; //Declare Direction Enumeration

        //ANIMATION DATA
        float _Speed; //Player Speed
        sf::Clock _AnimClock; //Player Animation Clock
        sf::Time _AnimTime; //Player Animation Time
};

#endif // PLAYER_H
 

player.cpp

#include <SFML/Graphics.hpp>
#include <iostream>
#include "player.h"

Player::Player() //Constructor
{

}

Player::Player(sf::Texture& TEMP_Texture) //Overload Constructor
{
    //PLAYER SPRITE
    _Sprite.setTexture(TEMP_Texture); //Set Sprite Texture
    _Sprite.setPosition(600 - 32, 350 - 32); //Set Sprite Position (Centre)
    _Sprite.setScale(1.5f, 1.5f); //Sprite Scale
    sf::Vector2i _Source(1, Down); //Default Sprite Sheet Crop
    _Sprite.setTextureRect(sf::IntRect(_Source.x * 32, _Source.y * 32, 32, 32)); //Crop Sprite Sheet (Default Crop)

    //PLAYER / ANIMATION SPEED
    _AnimTime = sf::milliseconds(80); //Animation Speed
    _Speed = 0.20; //Player Speed
}

Player::~Player() //Destructor
{
}

//ACCESSOR FUNCTIONS
sf::Sprite Player::getSprite() const //Player Sprite
{
    return _Sprite;
}

//PLAYER MOVEMENT
void Player::moveUp()
{
    _Source.y = Up; //Set '_Source.y' Equal To 'Up' (_Direction Enum)
    _Sprite.move(0, -_Speed); //Move Player Sprite

    if(_AnimClock.getElapsedTime() > _AnimTime)
    {
        _Sprite.setTextureRect(sf::IntRect(_Source.x * 32, _Source.y * 32, 32, 32)); //Crop Sprite Sheet

        //Animation
        _Source.x++;
        if(_Source.x * 32 >= _Sprite.getTexture()->getSize().x)
        {
            _Source.x = 0;
        }
        _AnimClock.restart();

    }

}

void Player::moveDown()
{
    _Source.y = Down; //Set '_Source.y' Equal To 'Up' (_Direction Enum)
    _Sprite.move(0, _Speed); //Move Player Sprite

    if(_AnimClock.getElapsedTime() > _AnimTime)
    {
        _Sprite.setTextureRect(sf::IntRect(_Source.x * 32, _Source.y * 32, 32, 32)); //Crop Sprite Sheet

        //Animation
        _Source.x++;
        if(_Source.x * 32 >= _Sprite.getTexture()->getSize().x)
        {
            _Source.x = 0;
        }
        _AnimClock.restart();

    }
}

void Player::moveRight()
{
    _Source.y = Right; //Set '_Source.y' Equal To 'Up' (_Direction Enum)
    _Sprite.move(_Speed, 0); //Move Player Sprite

    if(_AnimClock.getElapsedTime() > _AnimTime)
    {
        _Sprite.setTextureRect(sf::IntRect(_Source.x * 32, _Source.y * 32, 32, 32)); //Crop Sprite Sheet

        //Animation
        _Source.x++;
        if(_Source.x * 32 >= _Sprite.getTexture()->getSize().x)
        {
            _Source.x = 0;
        }
        _AnimClock.restart();
    }
}


void Player::moveLeft()
{
    _Source.y = Left; //Set '_Source.y' Equal To 'Up' (_Direction Enum)
    _Sprite.move(-_Speed, 0); //Move Player Sprite

    if(_AnimClock.getElapsedTime() > _AnimTime)
    {
        _Sprite.setTextureRect(sf::IntRect(_Source.x * 32, _Source.y * 32, 32, 32)); //Crop Sprite Sheet

        //Animation
        _Source.x++;
        if(_Source.x * 32 >= _Sprite.getTexture()->getSize().x)
        {
            _Source.x = 0;
        }
        _AnimClock.restart();
    }
}

void Player::setSpeed(float TEMP_Speed, sf::Time TEMP__AnimTime) //Sprint Speed
{
    _Speed = TEMP_Speed;
    _AnimTime = TEMP__AnimTime;
}
 

[Nexus]

The overall code looks good (nicely indented, meaningful responsibilities and identifiers, no manual memory management). Some remarks, many of them are just minor issues:

• Identifiers containing two underscores or underscore + capital letter are reserved for the compiler and library implementation; you may not use them. E.g. TEMP__AnimTime or _Speed. For members, you can use something like mSpeed.
  
• Why is there a default constructor, if it doesn't initialize the Player?
  
• Since the destructor is empty (and not virtual), there is no need to define it.
• Use the constructor initializer list instead of assignments to initialize members.
• Consider const-correctness. The Player constructor should take a reference to a const texture, since the latter is not changed. I.e. Player(const sf::Texture& texture)
  
• Constructors callable with one argument should be declared explicit, unless you want implicit conversions.
  
• You should list the parameter names even in function declarations; after all, headers represent the API of your classes.
• I would not mix input handling and rendering chronologically (i.e. clear() is directly before draw()).
  
• Use float literals consistently (0.20f instead of 0.20)
  
• Make headers self-contained. At the moment, player.h requires SFML headers to be included beforehand, it should rather include the necessary headers itself.
  
• For bigger projects, it would be wise to include only the required headers, e.g. <SFML/Graphics/Sprite.hpp> instead of <SFML/Graphics.hpp>.
  
• I probably wouldn't use a getSprite() method, since it is unflexible with respect to changes in the rendering. For example, if the player suddenly consists of multiple sprites, you have to adapt everything. One option is to derive Player from sf::Drawable. Alternatively, a good idea would be to separate game logics (such as player position, speed, etc) from rendering and let Player only contain the logics. The sprite is then computed outside, e.g. by a separate Renderer class. There are many design possibilities here, with different advantages and drawbacks.
  
• Some syntax inconsistences such as if( and if (, or single-line if statements with/without {}

By the way, just in case you don't want to reinvent the wheel, you could have a look at Thor's Animation module.

[guitarmatt99]

Thanks for all of that! I will take it all in and improve it over time. I've recently been playing around with the way I name things which was actually a hard task. I wanted a way that would show what things are without it being hard to read which is why I chose '_Member' for member variables. Thanks for letting me know this is bad practice, I did try 'mVar' but I didnt like how it looked, shame.

When I put 'TEMP__AnimTime' that was unintentional, I didnt mean to have two underscores, I cleaned up my code last night and made a mistake with that! But thanks for pointing it out, I have changed it.

When you mentioned creating a 'Render' class, what exactly would it be doing, would it just be returning information?

Also what did you mean by the constructor not initializing the player?

[Ixrec]

Also what did you mean by the constructor not initializing the player?

He was referring to your default constructor

Player::Player() //Constructor
{

}

and the fact that nothing is initialized in here.  Since you don't initialize anything in there, it's hard to imagine you have a use case for this constructor, so you might as well not bother defining it.

[guitarmatt99]

Thanks, it was the way it was worded that confused me.

[Nexus]

Thanks for letting me know this is bad practice, I did try 'mVar' but I didnt like how it looked, shame.

You can also simply use var. There is no need to have a special prefix for members.

When you mentioned creating a 'Render' class, what exactly would it be doing, would it just be returning information?

There were already some threads where I mentioned it, e.g. here and here. What I have in mind is something like

class Player { ... }; // pure logic

class Renderer // pure graphics
{
public:
    void draw(const Player& player);
private:
    sf::RenderWindow& window; // window stored elsewhere
    sf::View view;
    ...
};

That is, the knowledge about how to render is extracted from Player. One possible implementation would be to create new sprites in Renderer::draw() on-the-fly (this usually won't be a performance problem). But you can also store rendering state (such as std::unique_ptr<sf::Drawable>) in the Player class, as long as Player doesn't touch it itself, but rather forward it to Renderer.

There are tons of design possibilities. We frequently discuss them in the forum, don't hesitate to search (for separation of graphics and logics in this case).