But It will not compile properly, any ideas what the hell I am doing wrong?

I'm guessing Renderer is a SDL_Renderer*, right? Or something else? You should follow some convention on the naming of variables, charTexture and Renderer look like different things (a variable and a class name respectively).

Share this post

Link to post

Share on other sites

I have managed to make a texture wrapper class and I'm trying to implement a linked list for the generation of the textures/objects for when they are needed.

std::list is a standard library available to you (#include <list>) that is a linked list.
However, std::vector (or, depending on what you are doing, std::unordered_map) is a superior choice here.

Do I 'Have' to initialize the textures at the point of initialization or can I call them when they are needed from the folders

No, you don't need to point your pointers at dynamic memory the moment you create them, but you should at least initialize your pointers to null. You should never leave variables uninitialized.

Blah *myPointer; //Bad. Points to a random gibberish location in memory. 'Bad Things Happen' if you accidentally try to use it.
Blah *myPointer = nullptr; //Better. If you try to read or write to a null pointer, your program crashes. And that's a *good* thing.

However, in general, it's preferred not to use raw pointers to micro-manage memory lifetimes.

Even though SDL does this, that's because SDL is designed for the 'C' language.

In C++, it's better if your variables manage their own allocations and deallocations, to prevent human error mistakes when writing code.

Wrapping the texture in a class is one possible correct thing to do, but you might want to post your implementation (inbeween [ code ] and [ /code ] tags) so we can make sure it is written properly. Dynamic memory can be difficult to get right if you are just starting to learn the language.

Share on other sites

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.

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.

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.

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.

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, andIMG_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:

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!

Edited December 21, 2014 by Servant of the Lord

8

Share this post

Link to post

Share on other sites

Thanks Servant, that has to be the best reply I've ever had to any post ever, you've really helped me in a lot of areas as I definitely picked up bad habits from stumbling across Hungarian notation, and I didn't realize that the underscore would cause so much trouble.

A hell of a lot of this is me learning as I'm going so some of the changes I am making are just due to inexperience and lack of understanding so you've really helped me fill a lot of the gaps in. I'm off to tweak a lot of my code now and start aiming toward making an actual game that I can be proud of.

Thanks again, I am going to print this and keep referencing it to remind me what not to do.