Very good, that's mostly correct!
There are a few problems and minor additions you should fix to improve it.
Class, function, and variable names should not begin with underscores
The C++ standard says:
Certain sets of names and function signatures are always reserved to the implementation:
Each name that contains a double underscore__ or begins with an underscore followed by an uppercase letter is reserved to the implementation for any use.
Each name that begins with an underscore is reserved to the implementation for use as a name in the global namespace.
It's easier to remember if you just say, "don't begin any name with an underscore". Compilers, when compiling your code, generate additional variable names using underscores, and that can occasionally, every blue moon, overwrite or conflict with variable names you wrote with underscores.
You named your textures memory variables beginning with underscores, which can cause problems. Different people have different coding styles. Some prefix their private member variables with m_ (you can use underscores, just not at the start of a name).
Personally, I have public member functions and member variables begin with a capital ( MyFunction(), MyVariable ) and private functions and variables begin with a lowercase. ( myPrivateFunction(), myPrivateVariable ). Other programmers use different styles.
C++'s Rule of Three:
Imagine you had a texture loaded, and then copied that Texture class.
Texture texture;
texture.Load("blah");
Texture texture2 = texture;
Only one SDL_Texture was loaded. But which Texture class will call SDL_FreeSurface() on it?
The answer is that they will (accidentally) both try to destroy the same texture, freeing invalid memory and resulting in undefined (and bad) behavior that, best case, crashes your program. Worst case, results in random hard-to-find bugs that cause really odd behavior at unpredictable and inconsistent times. Dangling pointers and related issues are the bane of programmers.
For any class that manages memory, you need not only a destructor, but also a copy-constructor and assignment operator. Alternatively, you can explicitly block copying of a class.
class Texture
{
public:
Texture(const Texture &) = delete;
Texture &operator=(const Texture &) = delete;
If you are using a version of C++ that predates C++11 (or have forgotten to enable C++11), instead of = delete, you can move the functions to 'private' access instead. You don't need to even write the functions, just declare them.
class Texture
{
private:
Texture(const Texture &); //Copy constructor
Texture &operator=(const Texture &); //Assignment operator.
public:
Texture();
//...etc...
};
Now it's the C++ Rule of Five:
With C++11, they introduced move-semantics. A copy duplicates the content of a class. A move transfers the content from one class to another, without copying.
For any class the manually manages the lifetime of a variable, you'll want to add a move constructor and a move assignment operator.
An easy way to do this is just to swap the variables (for example, swap the null pointer with the good pointer); though you don't always need to swap all of them, and swapping isn't even required as long as you leave the old object in a safe state.
class Texture
{
public:
Texture(Texture &&other)
{
std::swap(x, other.x);
std::swap(y, other.y);
//...etc...
std::swap(texture, other.texture);
}
Texture &operator=(Texture &&other)
{
std::swap(x, other.x);
std::swap(y, other.y);
//...etc...
std::swap(texture, other.texture);
}
};
There are faster ways of writing it, but that's for another time.
More information on the Rule of 3 / Rule of 5.
In the Load() function, std::string should be passed in by const-reference
Here:
bool Texture::Load(std::string Filename, SDL_Renderer* pRenderer)
Should be:
bool Texture::Load(const std::string &Filename, SDL_Renderer* pRenderer)
Because 'filename' is not modified inside the function, the Load() function doesn't need a copy of the string, just a reference to the original string. By making it a const reference, you also guarantee to the caller that you aren't going to modify 'Filename' within the function itself.
<personal_opinion>
And while we're in this function, 'pRenderer' doesn't need the 'p' in its name. You don't need to write 'pointer' in the name of the variable, you already know it's a pointer from reading the code, and your IDE can instantly tell you its a pointer when you hover your mouse over it.
You don't call your strings "std::string stringFilenameThatContainsText", you don't need to name your pointers "pRenderer", "ptrRenderer", or "rendererThatIsAPointer". Just plain "renderer" is fine.
Some programmers do that. But then again, some programmers do many ridiculous things like prefixing their class names with 'c' ("because it's a class!" ). Some of it was useful twenty years ago when we had poor tools, but is no longer useful now, and some of it is from genuinely good ideas that got twisted into bad ideas because of misunderstanding.
In general, I try to name my variables to explain what they are for usage-wise, not what they are type-wise. "Filename" is what it is used for, and so is a descriptive name. If I need to know the type of Filename, I can ask my IDE by hovering the mouse over it, or by jumping to its definition. But if "Filename" was named "AString" instead, I can't ask my IDE what the intended usage was that the original programmer had in mind.
</personal_opinion>
The Texture class (probably) shouldn't change its value each time you draw it
Imagine you had ten monsters you were wanting to draw, and all of them use the same appearance. They can all share the same Texture. The monster classes themselves will store the positions where the monsters want to be drawn. So why does the Texture class store the (x,y) location of the last place the Texture was drawn? What practical use does that have, if more than one object is sharing references/pointers to the same texture? (same thing for Width and Height)
It makes sense for textures to remember their (x,y,width,height) of a subportion of a spritesheet, but that doesn't appear to be occurring here.
To think about it another way, every time you draw a Texture class, the class changes part of its state. What for? Drawing a texture should be a const operation (i.e. it shouldn't modify the Texture class).
Error messages should give more context
In your Load() function, there are many ways for the function to fail. You properly detect this and give an error message. Excellent!
Still, bugs and crashes can be annoying. You want to make it as easy as possible to track down problems when they arise.
Take for example, this error-check:
SDL_Surface* pTempSurface = IMG_Load(Filename.c_str());
if(pTempSurface == NULL)
{
std::cout << "pTempSurface IMG_Load failed" << std::endl;
return false;
}
That's helpful - it says what went wrong (IMG_Load() failed).
But it's even more helpful if you add where it went wrong, why it went wrong, and under what circumstances.
Where it went wrong:
It failed in the Texture::Load() function. You know that reading the code, but you don't know that when reading an error logfile or the console output.
Why it went wrong:
IMG_GetError() returns an error message giving additional details when IMG_Load() fails. You can output that as part of your error output (put it in quotes and say what function produced the message: [[[ My error output message, and details, and IMG_Load() says: "The texture couldn't be loaded from the file." ]]]
Under what circumstances it went wrong:
Imagine your program crashes, and your reading your error logs. Your program loaded 100 textures just fine, but one of them failed.
If all you see is, "Texture::Load(): The image failed to load.", you don't know which of the 100 textures caused the problem!
The Texture::Load() function already has the filepath that it is trying to load - you should definitely output that filepath as part of the error output when something goes wrong.
It's nice when you preemptively help yourself debug potential future problems, by providing the information necessary to make the bugs less painful when they occur.
Think of how much easier it would be to track down problems if you got this message:
Function: Texture::Load()
Problem: Failed to load the texture
Reason: IMG_Load() says, "Couldn't find the file at the location given"
Filepath: ./Resources/Textures/SuperAwesomeGriffonSprite.png
You can add even more details to the error messages, and wrap the information up in functions to make virtually effortless to add the information to all your output without alot of repetitive typing and remembering.
Here's some of the other nice things you can add to your error output:
- The exact location in code (file, function name, and line number) that outputted the error (Google for C++ __FILE__, __LINE__, and __FUNCTION__ / __PRETTY_FUNCTION__ )
- The stacktrace - which function called what function called which function that called the function that encountered a bug.
- What overall and in general your program was working on in the big picture before it ran into problems.
- Syntax highlighting/formatting to make the output easier to read - some people output their error logs to HTML with CSS.
Some are a bit more work to implement than others, so this might not be a priority for your current project.
_________________________________________________
Hopefully that helps you somewhat, and explains the reasoning behind why I do things that way. Overall, your code looks very good, so you're already on the right track.
Good luck on your programming adventures!