New Project Help

[Aelis440]

Hello everyone,

I'm currently part of a game development project using Irrlicht. We've spent a few weeks learning how to use the engine as well as C++. Being new to both of these, we were hoping to get some feedback on the manner in which we're writing our code.

In particular, we're interested in any glaring problems with our structure (headers, class definitions, etc.) as well as any issues we may have inadvertently caused (memory leaks, etc.) form our lack of experience with the language. We're more than happy for any input or suggestions. The Irrlicht community has done a great job in helping new people use the engine and I hope that by learning now, I can continue to contribute to the community in the future. Thanks a ton for any help you can offer.

I've included an executable of the game, you can run it to see what we've made so far. Basic controls are: WASD to move, B will create additional characters, use tab or the buttons at the bottom to move between them. Hold down the right mouse button to move the camera to either side. Thanks again for any help.

http://smallguild.com/ftp/sample.zip

[rogerborg]

It's great that you're trying to develop good habits early. I don't have a great deal of time to review this, but here's what I noticed on a quick glance through. Please don't take offence at any terseness in the review comments. They're not meant as criticisms of you; this is just the way that I do code reviews.

Includes.h

Code: Select all

#if defined(WIN32)
#include <conio.h>
#else
#include "conio.h"
#endif

In C/C++, the difference between #include "foo.h" and #include <foo.h> is just the directories that are searched for the specified file. They're both implementation dependent, but <foo.h> will (typically) only search directories in the compiler's include paths, whereas "foo.h" will also first search in 'project' directories, e.g. the same directory as the including file. If you're including a system include like conio.h, then you should almost always use <conio.h> unless you have provided your own implementation which you want used in preference.

Code: Select all

#ifndef IRRLICHT_H
#define IRRLICHT_H
#include <irrlicht.h>
using namespace irr;
using namespace scene;
using namespace video;
using namespace core;
using namespace gui;

Et al. I'm pretty sure that what you want is a single #ifndef guard around the whole of includes.h, rather than one around each internal #include. All of those files that you're #including have internal guards.

Code: Select all

#pragma comment(lib, "Irrlicht.lib")
#pragma comment(lib, "irrKlang.lib")

The result of #pragma is implementation dependent, but you'd be safer putting these declarations in one .cpp file rather than in a header.

Code: Select all

wchar_t* toWideChar(int i)
{
...

This relies on includes.h only being included once (from anywhere). It's working now because you only have one .cpp file, but when you start to #include it from multiple compilation units (.cpp files), you'll get errors about multiple definitions of toWideChar(). Instead, declare toWideChar() as extern in includes.h, i.e.:

Code: Select all

extern wchar_t* toWideChar(int i);

and define it once in one .cpp file. Same for float_round() and trimString().


In GUI.h

It looks like you're using using IGUIEnvironment*, so I'm not sure why you're passing all those other pointers to getEngine(), or storing them in the class. Also, why not just pass the IGUIEnvironment* to initialize() instead? Same for the other classes that do this.


In Manager.h

The comment on checkKeyUp() is incorrect. Each event passed to it will only have information on one key, so a switch would be sufficient.

Same for checkKeyDown(). A switch would be tidier than all those empty if() cases.


In Resource.h

No minValue, or clamping to 0? I expect you'll need that sooner or later.

setCurrentPercent() is confusingly named. From that name, I'd expect it to set currentValue to a multiple of maxValue, not to multiply currentValue.

(percent/100) is a strictly integer comparison, so will result in 0 for any value of percent < 100. You'll want to use /100.0, or cast percent to a floating point value.

name is unused.

Is there any reason why you can't pass the name, limits and current value to the constructor?


In Character.h

moveCharacter() will move the character by a fixed amount each time it's called. Unless you limit the speed at which you call moveCharacter(), your characters' speed will be dependent on your frame rate. This is almost certainly not what you want. The usual way to deal with this is to calculate a frame delta time, and multiply all your movements by it. Same in turnCharacter()


General observations:
It's good practice to use constants of the correct type, i.e double, rather than relying on the compiler to cast them for you (see the problem in setCurrentPercent()).

Magic numbers are evil, because they're hard to maintain, and they don't tell you anything about their meaning. Descriptively named enumerated values or constant variables are a better solution. I personally put all of my constants as const statics in an SConstants structure to keep them all in one place.

There's no validation of return values, or error handling: you have many crash bugs waiting to happen as soon as anything unexpected happens, and no diagnostics to help you figure out what went wrong.

The sooner you get into the habit of assuming that every function call will fail, check all return values, and gracefully handle failures from any point in your execution, the better. It's not something that you want to try to shoehorn in just before you ship.

[Aelis440]

Thanks so much for the response, it's extremely helpful information. I was unsure whether or not a single #ifndef guard would work properly with multiple includes so I had them separated.

As far as your comment about the GUI class, I did it to keep consistency with the way I'm using my character, camera, etc. classes. I'm new to C++ as I said and could not figure out a way add my custom code to the base classes defined in Irrlicht. To that end, I wrote everything so they would use their own classes and simply have the base classes as a member of my classes. For something like the GUI, that's not necessary, but I preferred consistency in this case. The core problem comes down to me not being able to either inherit the base classes as well as not being able to have a default constructor when making an array of classes.

The resource class wasn't finished, but you suggestions are quite good. : D

Your comment about moving characters confuses me. It would seem to make more sense that character movement IS dependent on frame rate so that the movement is calculated synchronously with the draw rate of the screen. Otherwise, you would have your character jumping all over the place in low frame rate areas. This is only my first implementation and I obviously have no real gameplay to test it in, but that would seem to be the more ideal situation. I'll definitely keep it in mind as I continue working on this.

I'm not sure what you mean by "Magic numbers".

You are quite correct about not having error handling, that's something I had planned on implementing once I had my basic groundwork set up. If you have any suggestions on how best to go about doing that (I've never done it before, either), I'd be more than happy to hear it. : D

Again, I REALLY appreciate the information, fixing my bad implementations now and discontinuing them will save me a lot more time in the future. If you (or anyone else that's interested) have any quick tips on preventing memory leaks and that sort of thing, it's another topic I'm pretty paranoid about. I don't know if C++ usually handles that kind of thing on its own or if it's something I have to do manually (i.e destroying local ints once I finish using them in a function).

Again, thanks. : )

[cecil0812]

"Magic numbers" are a term used to denote hard coded numbers in your code. For instance:

Code: Select all

if (index == 15)
{
  // Do something with 15
}

15 is a magic number here. The better solution would be something like:

Code: Select all

const int SPECIAL_INDEX = 15;

if (index == SPECIAL_INDEX)
{
  // Do something with 15
}

This is a rather contrived example, but hopefully you get the point Magic numbers are bad because someone coming along and reading the code will look at it and wonder why 15 was special. A well named constant can tell them why it's special.

Also, if you need to change that number from 15 to say, 8, it's easy to do in one place rather than just hoping you changed it everywhere it was referenced and later found a place that you forgot.

[bitplane]

It would seem to make more sense that character movement IS dependent on frame rate so that the movement is calculated synchronously with the draw rate of the screen. Otherwise, you would have your character jumping all over the place in low frame rate areas.

Well, the frame-rate dependent movement is bad because your characters would whiz around in high frame-rate areas and slow down in low frame rate ones. There's a huge difference between 300fps and 60fps, even if the user doesn't see it!
There's two ways around this problem. The most sensible is as rogerborg said is using a delta time: "newPosition = oldPosition + velocity*time", which works well when you're just considering moving in a direction or turning.
But if you're getting 1fps and want the precise curve of the path of a grenade, including where and when it bounces and perfect collision with other moving objects in between frames, then you'll need some tough mathematics to take care of movement and collision for you.
To avoid all this you can use a "fixed time-step" - simply keep your delta at a fixed number of milliseconds (say 3) and do several physics loops to catch up with the animation before rendering the scene.
This can run into problems if the physics takes longer than those 3ms to calculate, so the standard advice is to either use a proper physics engine to do the maths for you, or just use simple movement and collision where everything goes in a straight line between frames, bullets arrive the instant they are fired, slow projectiles exist as lines, and everything acts strange at low frame rates.