Some questions about the code

[StefanP.MUC]

I'm currently working through the code and do some changes here and there. But while doing so I discovered some things that look strange at the first sight and lead me to some collisions with virtual and overloaded functions (especially while creating getters and setters).

Why do Creatures have two classes (Creature and CreatureClass, which is derived from AnimatedObject)? Looks like an unneccessary step to much.
What is ActiveObject for?

I'd suggest some reordering:

{l Code}: {l Select All Code}

// AnimatedObject and AttackableObject gave me some not-so-easy-to-resolve collisions
// stopping me from pushing to git
// creating a general class "Object" would solve this
Object
-- AnimatedObject : Object
-- AttackableObject : Object
-- ActiveObject(?) : Object

// Creature currently is not an ActiveObject (should it be? The name Active suggest to me that it is DOING something by itself)
Creature : AnimatedObject, AttackableObject, ActiveObject(?)

// Room currently is not an AnimatedObject (but it should be animated, shouldn't it?)
Room : AnimatedObject, AttackableObject, ActiveObject(?)

// Trap currently is not an AnimatedObject (but it should be animated, shouldn't it?)
Trap : AnimatedObject, AttackableObject, ActiveObject(?)


[oln]

There is a comment about it in there somewhere saying that it is not optimal, and I agree, as there is a bunch of strings that are stored for each creature that should only be needed to be stored for each class.
Will have to consult andrew on this though.

[andrewbuck]

The ActiveObject class is being used like an 'interface' in java. It forces the class to provide a doUpkeep() routine that gets called in the upkeep round of the turn. Things like creatures use this to make sure they are still alive, consume any mana from the keeper they need, etc. Traps use it to see if they should activate, rooms do their thing, etc.

The CreatureClass is is to keep things that are common to all creatures of a particular type, it is basically a template for a new creature. By inheriting it it should allow the = operator to work properly and automatically adds the data members like maxHP automatically.

-Buck

[StefanP.MUC]

Uhm, in ExampleFrameListener, line 472, what is this?

{l Code}: {l Select All Code}

#ifdef ASLKFEAGFJEALKGJESLKGJLESKGJLKESJG
#error

[oln]

A way of commenting out the part of the code I moved. #ifdef 0 didn't work, and nesting comments didn't work to well.
Though since the change I did seemed to work fine, I will remove it in the next commit.

[StefanP.MUC]

New question: CreatureSound.cpp, the setPosition method. I came to it because there is a TODO asking for optimization. Currently the function sets all sounds to the new position. Wouldn't it be enough to set only the position of the sound requested to be played?

Extend play(type) to play(type, &position) and remove the setPosition() methods?

Or is there a reason to not do it on this simple way?

[oln]

I am not sure. Some sounds could be done that way, though I was thinking that for some of the sounds, e.g movement, you actually want the sound to move with the creature, not just sound from from where the creature started moving. For things such as weapon hits, the positions could just be set when the sound is played.

[StefanP.MUC]

How do I find out how big the scene is? I tried finding it in Ogre's docs, but no luck.
I don't mean the tile count in the game map (that's there), but something like "scenemanager.size.width" (what would be equivalent to "gamemap.width * tile.size")
I need this to autocalculate the minimap camera.

Am I overseeing some simple function here?

[StefanP.MUC]

Forget the last question. I thought of something better for the minimap (by not using the Ogre cameras at all, since we don't need a 1:1 picture of the map, but a "map of the map" with shiny colors showing what is where and who does it belong to).

[oln]

Yeah, that might be better. We probably don't need fancy lights and stuff on the minimap, which would make it much faster to render.

[svenskmand]

Yes indeed a cartographic maps is the best.

[StefanP.MUC]

New question:
There's a todo about dirent.h, on how to do something like this on Windows. I think the only environment where dirent.h is not available is Visual Studio. Everywhere else it seems to be available (gcc has it, also on Windows). And For Visual Studio are existing free dirent.h implementations (this for example: http://www.softagalleria.net/dirent.php).
tinygettext basically uses exact the same function as we and also does not care about hacking around for Visual Studio.

Should we maybe just force potential VS devs to take dirent.h?

[svenskmand]

What is dirent.h?

[StefanP.MUC]

http://en.wikipedia.org/wiki/Dirent.h

[oln]

I have already put that file in for VS to get tinygettext to compile. (The license is compatible.)
I also finished the home path stuff in windows, just haven't merged and uploaded this yet.

[svenskmand]

Ahhh nice, we will then need to tell the game where to save the files from the installer, in Windows. I will soon try and figure out how to do that in WiX and then you guys probably need to read off that path in a file that the installer makes in the game dir. I will try and figure out how to do it and let you know.

[oln]

So you want to have an installer option where you can select where you want settings/screens to be saved?

[svenskmand]

That actually sounds like a good idea , I will see if I can do that. But I actually just wanted to tell the game and the installer that the stuff is saved in Documents/My Games, depending on operating system. Because on Win XP the game is allowed to modify the files in its own directory, so here we could just save the player files here. But on Vista and Win 7 that is not allowed so here we need to use the Documents/My Games folder. And it is the installers responsibility to know which OS the game is installed on. Then the game just gets a path where it is allowed to save its player files. Also when the game is removed the installer can ask if it should delete or keep the player files.

[StefanP.MUC]

Ah nice, then I can remove the todo in the ResourceManager now and make tinygettext use our ResourceManager (then there'll basically only be the POParser and the dictionary stuff left of tinygettext, so only the stuff we actually need ).

[StefanP.MUC]

The keyPressed() and keyReleased() events have the same code to move the camera around. Is this for a specific reason?
For me it seems to make no sense to still move the camera on releasing the key (because this is the moment when the user wants it to stop actually).

If this is NOT needed I'll remove it. If it IS needed, I'll at least merge the duplicate code into one function.

edit: Just got it building again after my refactoring. Seems the function is indeed needed (withit it, the scrolling "jumps" though the scene), but I still don't see exactly why. I noted that it is inverted to the keyPressed() event (when pressed turns left, then released turns right). Is this to make the movement smoother?

[StefanP.MUC]

Another question:

What to do with unused functions? Some classes have smaller functions (most also not documented and apperently very old) that are never called. Should they be deleted?

[oln]

Unless they are likely to be used again, I suppose we can delete them.

[StefanP.MUC]

On some places (for example RadialVector2 and Function) there is the following:

{l Code}: {l Select All Code}

#ifndef M_PI
#define M_PI 3.141592653589793238462643
#endif

What purpose does this have? M_PI comes from <cmath> so it will always be defined because we include it. Somewhere there's also a TODO about this, asking if we should always use this number.

Should we remove it? If not then we should decide if we want to always use our own define and define it somewhere centrally (and without the #ifndef).

[oln]

From what I remember, I had issues with it not being defined when compiling in Visual Studio.
As far as I know, the M_PI constant is not part of the C++ standard.
(Though we could potentially stuff it into a header file instead of defining it in every file it's used.)

Also, I see you have been replacing NULL with 0, though I can't remember having discussed/decided on that. I don't really have a preference for either, so if you prefer to use 0 for null pointers we'll do that.

[oln]

Didn't read the last line of you post.
Yes, I think that is what we should do for these kind of situations, create our own define, or preferably constant.