2D Tile Engine game

The game basically has a player (PlayerCharacter class) that can move around the world. It has a 2D world made of tiles managed by TileEngine class. The player can shoot projectiles and can collide with the boundary of the map and any solid tiles.

Entity.h:

#ifndef ENTITY_H
#define ENTITY_H

#include 

class Entity
{
public:
    Entity();
    Entity(float px, float py) : mPosX(px), mPosY(py) {}
    ~Entity();

    //Getters
    float GetPosX() { return mPosX; }
    float GetPosY() { return mPosY; }

    //Setters
    void SetPosX(float val) { mPosX = val; }
    void SetPosY(float val) { mPosY = val; }

protected:
    float mPosX;
    float mPosY;
};

#endif //ENTITY_H

Entity.cpp:

#include "Entity.h"

Entity::Entity()
{

}

Entity::~Entity()
{

}

PlayerCharacter.h:

```
#ifndef PLAYERCHARACTER_H
#define PLAYERCHARACTER_H

#include "Entity.h"
#include "WeaponStats.h"
#include "Projectile.h"
#include
#include

struct KeyState{
bool UpPressed;
bool DownPressed;
bool LeftPressed;
bool RightPressed;
bool LMBPressed;
};

class PlayerCharacter : public Entity
{
public:
//Constructor and Destructor
PlayerCharacter();
PlayerCharacter(float pPosX, float pPosY, float pSpeed = 0, float pHealth = 0);
PlayerCharacter(float pPosX, float pPosY, sf::Texture pTexture, float pSpeed = 0, float pHealth = 0);
~PlayerCharacter();

//Getters
float GetSpeed() { return mSpeed; }
float GetHealth() {return mHealth; }
sf::Sprite GetSprite() { return mSprite; }
sf::Texture GetTexture() { return mTexture; }
float GetDirection() { return mDirection; }
WeaponStats GetWeapon() { return mWeapon; }
std::vector& GetProjectiles() { return mProjectiles; }

//Setters
void SetSpeed(float val) { mSpeed = val; }
void SetHealth(float val);
void SetSprite(sf::Sprite val) { mSprite = val; }
void SetTexture(sf::Texture val);
void SetDiretion(float val) { mDirection = val; }
void Se

Remove unused headers

For example, in Entity.h you include #include but it's not used.
If you would only use it in your cpp, move the include there to save some compile time. If compile time is really important you can even predeclare in your header.

Use initializer lists for all constructors

For example, your zero-argument constructor in the Entity class (the one defined in the .cpp) doesn't initialize it's arguments, and you might up reading garbage memory.

Mark const methods as such

For example all getters can be marked as const, as well as all other functions that do not logically alter the state of the object.

float GetPosX() const { return mPosX; }
CheckTileSolidColision(const std::vector& CornerPoints) const;

Use virtual destructor's for classes you've inherited from

virtual ~Entity();

If you don't, the destructor of the subclass may not get executed if you're working on the interface.

Be consistent in naming conventions

It seems that you use the m prefix for members and p prefix for parameters. That's probably a good idea but be consistent

Projectile::Projectile(float px, float py, float pDirection, float pVelocity, float pDamage, sf::Texture mSpriteTexture) // Change mSpriteTexture in pSpriteTexture

Another tip is to use lowerCamelCase for variable names.

Try to avoid useless comments

Comments such as //ctor inside of a constructor don't say much - the syntax of the language and/or name of the function should be good enough to describe simple actions. Comments are very valuable, so feel free them on places where they are useful (in complex methods), but don't add any that add nothing.

Use nullptr over NULL

NULL is defined as a zero, which can also be considered as an integer by the compiler. This can get some weird issues when you have a pointer and integer overloaded function.

Pass arguments by const reference where possible

If an argument has a complicated copy constructor (e.g. if it has to copy memory (such as std::vector) or has to do atomic operations (such as increasing a refcount))

bool CheckTileSolidColision(std::vector pCornerPoints); // Change the argument to const std::vector& pCornerPoints

Work directly on an object instead of a temporary

in LevelEntityManager::GetPlayerNewPosition you use two temporary variables NewPlayerX and NewPlayerY just to copy them over into NewPlayerPos. You can the following code and work on NewPlayerPos directly:

sf::Vector2f NewPlayerPos(pPlayerPos);
 if (pKeyState.UpPressed)
    NewPlayerPos.y -= pPlayer.GetSpeed() * mFrameTime;
 ...

The same happens in your main.cpp where you create a temporary vector boolRow and then push them on SolidStateVec. Why not add them directly to SolidStateVec? You know the size beforehand, so you can call resize on SolidStateVec and acces the elements using the [] operator. (This way, the vector has to reallocate only once)

Use namespaces

You have some places with generic names, such as ToDegrees(), which might collide later on when you include some math library. Use namespaces from the beginning to prevent issues later on.

Prefer the compiler over the preprocessor

If you can, prefer a const variable over a preprocessor define.

#define _PI 3.14159265
const float _PI = 3.14159265.f;

The compiler can complain if you double declare, is strongly typed, ...

Cache sizes of containers while iterating

Depending on the container, calling GetSize() and operator[] can be expensive. Cache the size up front or use a range based for.

const size_t cornerPointsSize = pCornerPoints.size();
for (int i = 0; i < cornerPointsSize; ++i)
{
    const sf::Vector2f& currentPoint = CornerPoints[i];
    if (mTileEngine.CheckSolid(currentPoint.x, currentPoint.y))
        return true;
}

for (const sf::Vector2f& currentPoint : cornerPointsSize)
{
    if (mTileEngine.CheckSolid(currentPoint.x, currentPoint.y))
        return true;
}

Do nullptr checks

In some functions (e.g. Render(sf::RenderWindow* pTarget)) you pass a pointer while you're never checking if that pointer is actually valid. If the pointer cannot be invalid, pass a reference instead. If the pointer can be invalid, get some logging (use some form of asserts).

Watch out for integer divisions

return degrees * (_PI / 180);

180 is an integer, not a float. So you'll get a whole devision instead of a floating point devision if _PI is an integer as well. This is not the case, but it is dangerous so I recommend you change it to _PI / 180.0f

Clean up a little

Remove empty functions, don't pollute your main function. So either remove GenerateTestLevel() or transfer some of the logic that's currently in main.

Polling versus events

In your Update function you're polling the state. This means that every single frame, you're requesting state. In this case, it's about a few keys but if you have support for multiple players and controllers, this met

StackExchange Code Review Q#100792, answer score: 4