Hey guys!
I hope I'm posting this in the right forum (terribly sorry if not!). This one's going to be a bit of an unusual request.

I've been coding away at a small minigame in C++ using SDL and OpenGL, trying to figure out the best ways to write stuff before I tackle anything bigger. Having finished some basic mechanics (running, jumping, collecting, getting shot...), I decided that the best thing to do would be to ask some folks to check my code and point out the grave mistakes that I am no doubt making.

So, please, could anyone download my source code, check it out and tell me what I'm doing wrong, why I shouldn't do it in that particular way and how I should be doing it instead? And please also tell me how serious the issue is and if I can leave it the way it is, etc. etc. The more info the better I guess.



The code itself is fully functional, should compile without any problems. Only thing it needs is the SDL library which I haven't included in the .rar (+ opengl ofc.).

Download VC++ source code

Tell me if I forgot to include something in the .rar.

So, anyone up for helping me become a better programmer? ^^

I have one suggestion: use a tool like CMake to create IDE project files apart from Visual Studio easily aswell. If you use cross-platform libraries like SDL and OpenGL, why limit yourself to Windows?

So that will enable me to do what...? Have my source code VC++ independent?
I'll probably start tearing up through its docs later but while we're here - is it haz a crash course? And is it free for commercial use?

Thx :)

Had a quick look at the code..

When initializing member variables in a constructor, don't do


but



Don't define destructors and constructors if they don't do anything actually.

In the implementation of a method it's not necessary to write the namespace of the class explicitly, e.g.


You can just write alive = 0 instead, as you're in the local scope. Makes the code more readable.

BasicFunc.h/cpp are essentially C-code with a .cpp file extension. It breaks encapsulation by exposing variables like SDL_Surface* screen. There's no need to make this global. The code should be refactored into own files (collision, timing, texture loading in one file is a not very logical). Make the timer member variables unsigned to double the amount of time you can track.

The way you implemented texture loading, you can load textures multiple times. You might want to consider having a "pool" of loaded textures and point to these textures instead of loading them again, if they don't need to be manipulated by one object (in which case it would be legitimate to load a texture multiple times). Otherwise you waste memory.

You shouldn't use immediate mode. While vertex buffer objects might be a little overkill for quads, consider using vertex arrays.

Do you define texture width and height as floats so you can avoid integer division as in "frameWidthOffset=1/(texture.Width/frameWidth);"? Just cast instead. Consider writing such statements like "1.f * frameWidth / texture.Width" instead (it's equivalent) to keep precision.

bool randBool(){	static bool FirstRun = true;	if (FirstRun){		FirstRun = false;		srand((unsigned) time(NULL) * 48645625); //Randomize using time and a big random number	}	return rand() > RAND_MAX / 2;}

The "big random number" doesn't change anything and you should only call srand once at the beginning of your program and remove this check from randBool. Also consider return rand() % 2 instead.

Refactor the drawing code in main() into own functions/classes, it's also a little hard to follow because you use globals a lot where you don't need to (textures are fine in the global namespace for simplicity I guess, but why not move the timer inside the main() scope and pass the passed time to functions that need it?).

You have a lot of "magic numbers". You shouldn't hardcode all those numbers in main(), and most of the calculations in main could be done once during initialization instead of every loop (1024*widthFactor etc).

So it's a little untidy, but I see no big flaws and I like that you're not over-engineering things! Seems like it would be fun to play, but I can't test it right now.. although I might create a makefile really quickly cause it looks funny :)

Keep up the good work!

Quote:Original post by Dandi8
So that will enable me to do what...? Have my source code VC++ independent?
I'll probably start tearing up through its docs later but while we're here - is it haz a crash course? And is it free for commercial use?

Thx :)

Yes, it's free for commercial use. Check out cmake.org for the docs and google "cmake sdl" for some help on that, it's pretty straight forward.
You create a text file with some infos what you want "find sdl, find opengl, use these source files, create a binary named so-and-so..", then use the cmake program to create project files for various visual studio IDEs, makefiles, eclipse and some others. It's especially useful because it will find all the libraries and headers for you automatically (and if it doesn't it will inform you and let you specify the locations yourself).

Glad to help

Just tested your program on Mac OS X, worked flawlessly! However, my compiler complained about
A) #include <sdl.h>, it should be SDL.h. Windows' file system isn't case sensitive, but others are. You might want to rename it to the proper case.
B) text.cpp line 233: ifstream only accepts const char* as file name, not std::string. Change the argument to the ifstream constructor to x.c_str(). Does that really compile under Windows?

By the way, I realized you're copying non-POD types ("more complicated things than int and other built-in types"). Rarely will you want to pass an std::string by value, because it will create a copy of it. If you don't need a copied instance of the string, just use a function prototype like void foo(const std::string& x). This will use the same string you passed as argument instead of copying it and working on the copy (faster, uses less memory for huge strings).

Thank you! It's awesome help to me. More please! :D

And some questions:
What do you mean 'pool of loaded textures'?
And vertex arrays? I'm currently guessing what immediate mode is XD

Sorry, I'm a noob to that stuff :(

And I shall definetely checkout cmake soon.


Stuff I fixed:
-lowercase SDL include - didn't know that!
-That laser constructor thingy ---- Could you explain what I just fixed here and why I shouldn't define destructors and constructors that don't do stuff? Can I do it with vars other than bool?
-Namespaces
-texture width/height as floats -- I assume you meant the struct. Fixed and typecasted.
-srand ---- put it into my Init() function instead. But I'm keeping the big number, I actually noticed it helps with stuff being random with every run for some reason o.O
-line 233 c_str ---- Could you explain what the c_str thing actually does anyway? It compiles fine without it on Windows for some reason


Stuff I intend to fix:
-refactoring the drawing code in main - but could you elaborate on that? I'm not sure what functions/classes I would split that to. I already have a ton of functions in main. And what do you mean with the timer?
-magic numbers ---- although I tried to minimize my numbers' magic as much as I could
-BasicFunc.c ---- I'm keeping everything in this file mainly to be able to just attach it to any new project and use my functions. On second thought, I guess I could break it up and include everything in basincfunc.h... Well, gonna do it later!
-That std::string thingy ---- could you explain that a bit more? And those non-POD types?


How is there no need to make SDL_Surface* screen global? That way I can refer to the screen in any file I want. Am I not right?

How do I distinguish c from C++ code (regarding basicfunc)?
Could you give me an example of over-engineering?
Oh and what is the difference between an int and a GLint?

Is there a better way to handle my animation?
What about my text class/functions?

And could you tell me something about compiling for other platforms? I haven't done that yet.

Wow... so many questions, hope you have time to answer ^^

Quote:-That laser constructor thingy ---- Could you explain what I just fixed here and why I shouldn't define destructors and constructors that don't do stuff?

For the change that you made, look up 'c++ initializer list'. For your second question, I guess the real question would be: why *would* you define a constructor or destructor that doesn't do anything?

For a more in-depth answer, look up 'c++ compiler generated constructor destructor'.

Quote:Can I do it with vars other than bool?

Yes.

Quote:-line 233 c_str ---- Could you explain what the c_str thing actually does anyway? It compiles fine without it on Windows for some reason

That's something you can look up. A Google search for 'string c_str', for example, returns this, among other things.

In brief though, it returns a pointer to a null-terminated C-style string.

Quote:-magic numbers ---- although I tried to minimize my numbers' magic as much as I could

Ideally, you really shouldn't have any 'magic numbers' at all aside from really trivial ones.

Quote:How is there no need to make SDL_Surface* screen global? That way I can refer to the screen in any file I want. Am I not right?

The real question is, why do you need to be able to refer to the screen object in 'any file you want'?

Writing procedural code and making stuff global is perfectly fine, and is a time-honored approach to writing games. However, it can be argued that for larger-scale projects, taking an object-oriented approach can have some advantages. (In object-oriented design, the use of globals is generally avoided; for the why's of this, just search the forum archives for 'global variables'.)

Quote:Could you give me an example of over-engineering?

To over-engineer just means, essentially, to make something more complex than is needed. (Or at least that's how I think of it.)

Quote:Oh and what is the difference between an int and a GLint?

GLint is a typedef for one of the built-in integer types (usually - perhaps always - int). The purpose as I understand it is essentially portability; that is, you can just use GLint and let OpenGL worry about choosing a type that is appropriate for that platform.

Ok, I'm gonna look up all those things. Meanwhile, if you can say anything more about my functions, their quality, etc. etc. then please do.

The more critique I get on the subject the better.

Oh and if you could point me to a good tutorial on vertex arrays, that would be great. I can't seem to find any that would describe the process well. I don't even know how to put the vertex coordinates in the array in the first place o.O

Edit:
And should I use GLint instead of int for better cross-platformability?

Edit2:
I managed to code in the vertex arrays method.
Here's the updated source code
Is this what you meant?

[Edited by - Dandi8 on July 8, 2010 9:18:58 AM]

Quote:Original post by Dandi8
What do you mean 'pool of loaded textures'?

That may be something to remember for later. Right now you're simply loading all textures you need once during initialization, which is fine because you know exactly which textures you're going to need.
By a "pool" I simply mean that you do a little book keeping which textures you already loaded
1. LoadTexture(".foo.png")
2a. If foo.png is already saved with a valid OpenGL texture ID in the array "loadedTextures", then simply return that texture.
2b. If foo.png is not loaded already, load the file foo.png and store the Texture struct in the array loadedTextures, then return the texture.
This way if you load 100 stickmen for example, you will only have loaded the texture once, even if you call LoadTexture separately for each.

Quote:Original post by Dandi8
And vertex arrays? I'm currently guessing what immediate mode is XD

Just search for such keywords on the internet, you're bound to find something quickly. The problem with immediate mode (glBegin(..) ... glEnd()) is that
A) it's deprecated, it will disappear so you shouldn't rely on it anymore
B) it's slow. The poor guy who has to implement the OpenGL functions for you has the problem that he can't tell whether you're going to define a normal next, maybe a coordinate or a texture coordinate, maybe you're doing something else entirely (call glClear() although you're not allowed to for example). That makes it hard to optimize. Also calling one function for each vertex is a lot of overhead ("extra work for the program"), although that's not so bad for single quads.

Quote:Original post by Dandi8
Sorry, I'm a noob to that stuff :(

So we're here to help!

Your program compiled flawlessly, just a warning when compiling with -Wall:

length() returns an unsigned int (to be more precise, size_t). You might as well use unsigned ii. Comparison between unsigned and signed ints can expose you to a few bugs. The unsigned int may be larger than a signed int can hold
A signed int's maximum numbers is 2^16 - 1 = 65.535. Bits can be either 0 or 1, so 2 possibilities. Therefore you can have 2*2.....*2*2 (2^16) different states for the whole 16 bit number. We do -1 because 0 is one of these numbers, reducing the maximum by one.
An unsigned integer is 2^32 - 1 = 4.294.967.295 Bits at maximum. That maximum is higher than the 65.535 of a signed int.
If your text length is bigger than 65535, this loop will repeat infinitely, because the signed int wille always be smaller than the unsigned int. When the signed int reaches 65535 and ii++ is called, it will "overflow" and become the smalles number it can, which is negative.
Also: please use proper loop variable names. ii will become very confusing once you write more complex loops.

Quote:Original post by Dandi8
Stuff I intend to fix:
-refactoring the drawing code in main - but could you elaborate on that? I'm not sure what functions/classes I would split that to. I already have a ton of functions in main. And what do you mean with the timer?

Imagine this: your game will feature complicated artificial intelligence, level loading, huge masses of textures, networking, joystick support and all other fancy things in the future.. at some point you will lose control of your code, because everything is everywhere. It will be hard to fix things because everything can possibly change everything.
So what you try is to make a clear separation between things that can't possibly have anything to do with each other.
If I, as a foreigner to your code, look at it, I can say "oh his collision code doesn't work.. I'll check out the collision code" (pulled that example out of my magic hat). But when I realize you mix other things into that code I will have to check your drawing and sound code too, which should not be the case.
Look at your main.cpp and ask yourself "WHY is this line of code here, WHICH component does it belong to, WHAT does it try to achieve?" and you should be able to come up with an answer yourself. But yes, that's something you will need experience in which only comes when you think about these things yourself first, so I won't spoil the fun yet :]

Quote:Original post by Dandi8
-magic numbers ---- although I tried to minimize my numbers' magic as much as I could

I'll take random example how to think about this:

Why did you calculate the number, which component does it belong to, what does it try to achieve?
You calculated it because you need to position text on screen, it belongs to the new game text and it tries to center it somewhere. How about calling it newGameLabelTopLeft?
In addition, 512 should not be hardcoded. I guess 512 is half of the window width. What if you want to change the resolution of the window in the future (which you probably will)? You should be able to change this value when the window is resized. You also save a lot of calculations by only calculating these variables when something has changed (initialization, window resize here).

Quote:Original post by Dandi8
-That std::string thingy ---- could you explain that a bit more? And those non-POD types?

Read about POD types here first: http://en.wikipedia.org/wiki/Plain_old_data_structure
Read about pass by value and and pass by reference here: http://en.wikipedia.org/wiki/Pass_by_value#Call_by_value

Now consider these two versions of a functions which is given a string and has to decide whether the first letter is capitalized:


This function looks innocent, but if s were the content of a book you would realize that this function is slow for some reason. When you call

You will have the string saved in the variable "story" in the global scope. Once you call the function you pass it the string "by value". That means that the function will copy "story" into its own variable "s". It will check the first letter, return the result, the destructor of "s" is called and you now only have the text in "story" again. Wouldn't it be great if the function would just look at story's first character? You can, by passing by reference (you point to the memory address of story)

Now s is the same thing as story and you don't copy the memory, great! But wait.. the function changed story? Seems like someone made a programming mistake. Surely a mere check whether the text is capitalized at the beginning should not be able to change it? That's why you use const std::string& instead, the string will be immutable ("can't be changed").

Quote:Original post by Dandi8
How is there no need to make SDL_Surface* screen global? That way I can refer to the screen in any file I want. Am I not right?

Only make things global when pretty much everyone is guaranteed to need access to the data and if it's safe to change it.
Your collision code shouldn't need to access screen for example. But it potentially could, as a foreigner to your code I can't tell whether it does before I looked at that code. If screen is not global, I have a guarantee that your collision code does not use it when it's not explicitly told to do so (by passing screen as argument in collision functions).
Also, let's say you implement a function that draws on your screen, you were drunk and introduces a bug (because you never create bugs when you're sober). The drawing function messes up the variable and changes screen to 0 because of some error.. Now all other functions using screen are in danger if they didn't check screen for a null pointer (which they wouldn't have to if they had a guarantee that no one can set screen to 0 just like that).

Quote:Original post by Dandi8
How do I distinguish c from C++ code (regarding basicfunc)?

What I mean is that while you use C++ syntax, you don't "think" in C++. You use practices from C. You have not understood the concept of data encapsulation yet, for example. You use free functions (functions that don't belong to classes) even where you use data that should be hidden as private member variables. SDL_Surface* screen could belong to a renderer class for example, and all access to screen can be controlled by it. Free functions should only be used for.. managing logic of your program, if that makes sense? For example I would use a void prepareLevel() function that loads the next level file, positions all enemies, resets the computer players' AI (artificial intelligence) and such, but I would not make loadLevel() a free function, but stick it into a "class Level" instead, because it will handle a lot of data that should only be changed with its permission. Changed the "width" variable of the level? class Level should always know that because it will have to resize the level, for example.
You just reduce the chance that foreign coders can mess up your code and protect yourself from destroying something by accident, too.

Quote:Original post by Dandi8
Could you give me an example of over-engineering?

You want to get rid of a spider, so you build a laser cannon and should it, instead of picking the poor thing up and throwing it out of the window (my girlfriend is prone to this behavior).

Quote:Original post by Dandi8
Is there a better way to handle my animation?

What are you trying to achieve, what are you unhappy with? Putting the straw man's textures into one big texture ("texture atlas/sprite sheet") is really good already. I don't know which feature you want to extend upon your current animation.



What about my text class/functions?

I don't have much time to look through it right now, but it seems like you could simplify a lot of it. For example you use rows like "carriage += ((A_WIDTH) + TEXT_SPACE) * (float(charWidth) / float(CHAR_WIDTH));" a lot. You could calculate most of that formula once, save it in a variable and use that variable to replace that part of the formula in all of your lines. It would make the code more readable and if you solved an error in that formula it would correct all lines.

Quote:Original post by Dandi8
And could you tell me something about compiling for other platforms? I haven't done that yet.

It's fun. I don't know what to answer, that's a very general question.. :P

By the way, try to enter a game over check that restarts the game after a button press. Would make your game more playable already. And a cool charging laser sound! F*** yeah!