Some questions about the code

[StefanP.MUC]

Yes, seems like M_PI isn't part of the standard (neither C nor C++). So we should define it ourselves at some central place (we could use Globals.h for these kind of stuff, but all externs and global vars should still be removed from Globals even if we keep the file).

NULL is C-style. In C++ NULL exactly the same as 0. I wasn't sure either until some days ago, but after reading some articles about this topic I found out that most people seem to prefer 0 in C++. So I think could be considered as a "de facto standard".

[oln]

Also, I think we shouldn't use const references for primitive types:
http://stackoverflow.com/questions/6727 ... tive-types

[StefanP.MUC]

Yes, I agree, sounds reasonable. Never thought of it from this perspective.

[StefanP.MUC]

Just fixed the references from my last commits again (for the rest, I'll do it on the fly when I stumble upon referenced primitives).

[svenskmand]

I have not touched the code so the decision is entirely yours, but I think that using NULL is conceptually clearer than using 0, it also looks nicer. It is probably also better for newcomers to use NULL since my guess is that most people use this. Just my two cents.

[StefanP.MUC]

Here are some links about this topic:
http://www.cplusplus.com/forum/beginner/5604/
http://www.cplusplus.com/reference/clib ... ring/NULL/
http://www2.research.att.com/~bs/bs_faq2.html#null

The new C++11 standard has a new keyword nullptr for this (maybe we should decide if we want not only to stick to C++, but also to a specific standard).

IMHO, these are all good reasons to use 0 (or later nullptr) instead of NULL.

But another topic, about the place of using const:

{l Code}: {l Select All Code}

//currently we do this
const int function() const { return 0; }

//according to the standard const works to its left
//and only to the right if there's nothing on its left
//so we should really do this to be standard conform
int const function() const { return 0; }


While this doesn't make any difference on value types, it can make a difference on pointers if one is not careful:

{l Code}: {l Select All Code}

//(changable pointer) to (constant int)
//const operates on int here
const int* ptr;

//above is the same as same as this
//const still operates on int here
int const* ptr;

//(constant pointer) to (changable int)
//const operates on * here
int* const ptr

//(constant pointer) to (constant int)
int const * const ptr;


Any opinions here? I'm still undecided what I like more. Having const on the left makes reding the code easier, imho. But sticking to the standard has more value as an argument than plain readability, I think.

[oln]

I think it's to early to rely on C++11/tr1 support, even though it does contain a lot of useful stuff. (Though, a lot of it is available in boost.)

Const before or after the type is really a matter of style, I am more used to putting it before the type.
(So is Stroustrup apparently)

[svenskmand]

I think this is the way to go then:

In short, as long as NULL is
#define NULL 0

From Stefans first link.

[StefanP.MUC]

A new question:
HP, mana and exp are of type "double". Why? Shouldn't they be "unsignd int"? unsigned because they can't and must'n be negative and int because they are countable.
Yeah, I know, some of the calculations do +0.1, but this can easily be fixed by a simple rescaling (everything times 10).

Never saw a game where you could have "10.5 / 12.7" HP or something.

Not sure how much impact this would have, but hp, exp, and mana are numbers used extremly often in any kind of calculations. As far as I know, calculations with double precission are much slower than float or int, even on modern CPU's. (So this could be another reason to change these numbers to unsigned int)

[oln]

I've seen both. I think we should change to integers for consistency, especially for multiplayer. I don't think the speed difference will be noticeable.

[svenskmand]

I agree with Oln. E.g. Blizzard uses decimal point values in many of their games. I do not know how they store them internally though.

[StefanP.MUC]

Another one:
What exactly is TemporaryMapLight good for? It's a light that destroys itself after a while, but is it ever used (and what for)? And if it's needed, why couldn't the same task be done by MapLight itself: give it a doUpkeep() and only execute if(!isPermanent)?
So all in all TemporaryMapLight looks rather useless to me. But maybe I did not see an important point here.

[oln]

They were used as an effect for tile claiming, but I turned that off to limit the number of lights. So they are not used for anything right now. (The classes are from before my time.)

[StefanP.MUC]

So, would you say that the functionality of this class can be merged into MapLight? Or drop completly? Leaving it as it is now seems rather pointless to me (especially that it's a derived class that only adds one "trivial" feature that could easily be handled by the base class itself).

[oln]

Yeah, drop or merge.

[StefanP.MUC]

OK, dropped it. Also merged the two ctors and the one initialize() function into one default ctor.

[StefanP.MUC]

New question: While merging some of the constructors with the initialize() method into one default constructor I noticed that some value assignments have a sem_wait() and sem_post() already in the constructor.

Does this make sense? It doesn't cause any problems, of course, but there's also no benefit, is it? Because it's the constructor of the object, the object still does not fully exist while executing the constructor, so there shouldn't be any other code accessing the object yet (writing multithread code that allows accessing an object before the ctor is finished - if this is even possible at all, no idea - it would lead to segfaults, I believe?). So no need to lock any variables in the ctor and waste cpu time.

Or am I wrong here?

edit: After reading through some sites found by google, it seems that locking in a ctor is only needed when accessing static variables or when using copy constructors.

[StefanP.MUC]

New question:
getOgreNamePrefix() is currently a pure virtual in MovableGameEntity. Also non-moving objects need an unique Ogre name, or am I wrong here? Also, this thing isn't used very consistent, some classes just return an empty string "".

What was the original intention of this thing? Should it be dropped (seems this wouldn't really change anything) or used consitently from GameEntity?

[oln]

Seems like it's used for ogre entity names yes, prefix + getName(). Seems it's just used to prefix the entity name with the name of the object type. Maybe it's to avoid name collisions.

[StefanP.MUC]

Then I think it should be common to every GameEntity. When doing this, I think it's also good to unify to unique number generation that is code-doubled almost everywhere.

And also a new question:
What exactly does the class Field do? It's used by Creature only as "battleField". It has much members from GameEntity, do you think it should be a GameEntity, too?

[oln]

Filed is used to generate a threat map for the creature Ai it seems. It has meshes to make it possible to display it for debugging purposes.

[StefanP.MUC]

What I always wondered: What exactly is the logical difference between Player and Seat? When to use Player and when to use Seat? Aren't they a bijection? The comments in both classes always refer to an entity named "player", leading me to the conclusion that a player is bound to exactly one seat and that it is the one and only player within this seat.

[oln]

This is my interpretation/suggestion. For the original intent we would have to ask andrew or alienwolf. As
The player class contains (or should contain) only the interface-specific data, such as what building is selected and nickname (not sure why the hand-stuff is in Player). The seat contains the game variables such as gold, mana, goals etc. They seem to be a bijection, but maybe the idea is that, say a network player that has dropped could be replaced by an AI player or similar. I remember planning to subclass it into networkPlayer, AIPlayer etc. but I don't know if that makes sense anymore.

[svenskmand]

We could also just have a Player class, that takes a ControllerStrategy as input. The ControllerStrategy can then either accept user input, or be an AI player. Then there is no need to have all the Seat stuff, and the Player class would be cleaner and nicely seperated from the controller of it which could either be a player or a ai.