PROJECT: Good Specific Terrain Improvements

[Ekmek]

	if(irec->GetNumNeedsCityGood() > 0) {
		sint32 s;
		bool found = false;
		for(s = 0; s < irec->GetNumNeedsCityGood(); s++) {
			if(irec->GetNeedsCityGoodIndex(s) == m_collectingResources || m_buyingResources) {
				found = true;
				break;
			}
		}
		if(!found)
			return FALSE;
	}

Just a random questioni n the possibility of adding strategic goods. I was thinking of making an attribute NeedsCityGood which means that the city must either have the good in its radius or is receiving the good from trade in order to let that city build a unit.

Now I'm NOT looking to make it like Civ3 where you need road connections etc, because if only certain cities can build units it makes not only the good strategic but the cities strategic (unless you trade the good within the empire).

But I'm not sure m_buyingResources and m_collectingResources pertains to goods. I haven't mapped out how this class is being filled (not sure if it counts food, prod, gold) but I think its goods. If this is the case then I'd like to add this code along with my already finished stuff.

[Martin Gühmann]

Well today I am just able to have a brief look at the files, E. One thing I should point out is the CultureOnly check in CityData::CanBuildUnit, since this method calls Player::CanBuildUnit, you should put it into Player::CanBuildUnit as it is Player-dependent and not City-dependent.

OK let's see whether I can look closer on the files, tomorrow. By the way you don't need to attach the files in the files in the Altered Source Files thread. You have to submit them anyway to the server. Therefore you can post them into the discussion where the belong into. Well and to your question tomorrow, if noone else answers it.

-Martin

[Ekmek]

Thanks, Martin. i suspect as much on the player stuff so I'll make the changes.

As for the code above, everything I saw in citydata.h and citydata.cpp seems to confirm that this is the class/object where the goods that a city can trade or in its radius is kept.

I think my check is okay but not sure how resources/goods are called from the DB.

[Ekmek]

Ok here's the added Player.cpp and updated CityData.cpp. I'm feeling confident with tortoise so if its good I shouldn't have a problem updating it.

	if(irec->GetNumNeedsCityGood() > 0) {
		sint32 s;
		bool found = false;
		for(s = 0; s < irec->GetNumNeedsCityGood(); s++) {
			if(irec->GetNeedsCityGoodIndex(s) == m_collectingResources || m_buyingResources) {
				found = true;
				break;
			}
		}
		if(!found)
			return FALSE;
	}

I added the || as an or if I'm not mistaken I think that makes it work better. As far as my looking around the code is I think its right, but I'm not familar with the resource DB so thats where I may have a problem.

[Martin Gühmann]

E, you shouldn't stop making changes until the stuff you already have done is right. The more you do now the more errors you can do. And I doupt that I can have a close look on the files, today. And maybe this holds for tomorrow as well.

This line is definatly wrong:

                        if(irec->GetNeedsCityGoodIndex(s) == m_collectingResources || m_buyingResources) {

Either m_collectingResources and m_buyingResources are booleans or they are arrays and I think they are rather arrays than booleans. But without the source code here I can't tell you for sure. In both cases you can't compare them like this. Well you can but then you get nonsense.

If they are booleans you can only use them to decide whether you should do the NeedsCityGood check or not.

If they are arrays you have of course to compare each array element of the arrays m_buyingResources and m_collectingResources with each element of NeedsCityGood. And I am not sure whether NeedsCityGood should be an array.

-Martin

[Ekmek]

THanks Martin,
I did not put the NeedsCityGood in the files I posted. I plan on finishing that first then do this new code, so I'm following your advice

I'll look into other ways to compare the tool, but I just really want to see if the city either has the good in the radius or is receiving it from trade.

	if(irec->GetNumNeedsCityGood() > 0) {
		sint32 s;
		bool found = false;
		for(s = 0; s < irec->GetNumNeedsCityGood(); s++) {
			if(irec->GetNeedsCityGoodIndex(s) == m_collectingResources[resource]) {
				found = true;
				break;
			} else {
                        if(irec->GetNeedsCityGoodIndex(s) == m_buyingResources[resource]) {
				found = true;
				break;
			} 


		}
		if(!found)
			return FALSE;
	}

[Martin Gühmann]

And still it doesn't work, resource is not defined and is not variable, so you compare each good in the NeedsCityGood array with a random value if you define resource at least, so it will prduce nonsence or crash.

-Martin

[Ekmek]

ok. I'll take a break on NeedsCityGood.

As for the file I uploaded it does NOT have NeedsCityGood in it, but just the other stuff we;ve been working on. I know you can't check it today or so but I'd like to get that one done first and then I'll come back to this NeedsCityGood

[Martin Gühmann]

Originally posted by E
I know you can't check it today or so but I'd like to get that one done first and then I'll come back to this NeedsCityGood

Actual I was able to check.

I don't touch now the *.cdb last time they were ok and I think they still compile, but there is some stuff we have definatly to talk about.

Then your descriptions have to get preciser. For instance a sint32 is a number and a number can't describe all the properties of a player, in the instance in question it is an index of an player in the global list of players g_player.

Then this line in a Player member function:

if(irec->GetCultureOnlyIndex(s) == g_player[m_owner]->GetCivilisation()->GetCityStyle()) {

Maybe Player has a member m_owner, but if you are in a Player member function you don't need to get a pointer on a Player object if it points on the same address in the memory as the this pointer.

Then you forgot to remove a #else in terrainutil_CanPlayerBuildAt. Otherwise the stuff seems to be fine, and check again your description, somewhere there was a city even it should be a unit or player, no idea which one.

-Martin

[Ekmek]

Should I remove the [m_owner] then? it sounds like what your saying...

[Martin Gühmann]

Originally posted by E
Should I remove the [m_owner] then? it sounds like what your saying...

Not quite. So again what is g_player? It is an array of pointers on objects of type player.

So now find the right answer.

-Martin

[Ekmek]

Ok I've gone through player.h and I think I have, but I have a few questions:

1) Is this for only the player.cpp file or all code that uses this function? I'm thinking its all of it.

2) DynamicArray. I assume this is an array that is constantl updated and changed, but whats thedifference between an array and a dynamic array?

3) m_owner is part of the player index which I assume means it just tracks players. What does the player index do or better yet where do I find out what the player index does and how do I know when I find it?

4) *m_civilisation is a dynamic array that stores thecivilisation of player from the looks of this. So I think this is what I need instead of m_owner, BUT why is it dynamic?
it has this:

	Civilisation *GetCivilisation(void) { return (m_civilisation) ; }
	void GetPluralCivName(MBCHAR *s) ;
	void GetSingularCivName(MBCHAR *s) ;
	MBCHAR *GetLeaderName(void) ;

But it looks like its under the player Index, but then I see class Selected Item. So I got a little lost trying to map where get Civ should be, but I'm still guessing m_civilisation (and I think its yet another there on how to try a CivOnly code too)

SOOOOOO based on what I've seen and if I had to guess I'd say the code should look like:

if(irec->GetCultureOnlyIndex(s) == g_player[m_civilisation]->GetCityStyle()) {

removing the GetCivilisation because m_civilisation does it now.

[Martin Gühmann]

Originally posted by E
1) Is this for only the player.cpp file or all code that uses this function? I'm thinking its all of it.

Well you should be more specific about this. I assume you mean whether this holds for all function in the player.cpp file or to other files as well. I guess it is a question related to the this pointer.

Only non static member functions have a this pointer. So you cannot access this in static functions. Static functions are member of the class, non static functions are member of an object, on that a pointer and point including the this pointer. And therefore static functions don't have a this pointer.

Of course any non static function of class Player can be implemented in any file of the project you want, so you could implement each function of Player in a different *.cpp file, so only a function of Player specified by Player:: can have a this pointer on a opject of type Player.

Originally posted by E
2) DynamicArray. I assume this is an array that is constantl updated and changed, but whats thedifference between an array and a dynamic array?

No, that's wrong. A dynamic arry is an array that increases its size when if it is full and another element should be added. I think it shrinks also in size if possible. However the STL offer better containers, espeacilly those that don't leak.

Originally posted by E 3) m_owner is part of the player index which I assume means it just tracks players. What does the player index do or better yet where do I find out what the player index does and how do I know when I find it?

No m_owner is a player index. It is used to access data that requires a player index, for instance because it is saved in an array with player index dependency. To use this in a non static function of Player to access from the list of players is stupid, because you just get the this pointer and you have the this pointer anyway, whether you write it into the code explecitly or or by just calling functions or other members of the object.

Originally posted by E
4) *m_civilisation is a dynamic array that stores thecivilisation of player from the looks of this. So I think this is what I need instead of m_owner, BUT why is it dynamic?
it has this:

	Civilisation *GetCivilisation(void) { return (m_civilisation) ; }
	void GetPluralCivName(MBCHAR *s) ;
	void GetSingularCivName(MBCHAR *s) ;
	MBCHAR *GetLeaderName(void) ;

But it looks like its under the player Index, but then I see class Selected Item. So I got a little lost trying to map where get Civ should be, but I'm still guessing m_civilisation (and I think its yet another there on how to try a CivOnly code too)

Well for the civ specific code you should find a GetCivName function or create one of your own if necessary, but for that later, when you have finished this project.

Originally posted by E
SOOOOOO based on what I've seen and if I had to guess I'd say the code should look like:

if(irec->GetCultureOnlyIndex(s) == g_player[m_civilisation]->GetCityStyle()) {

removing the GetCivilisation because m_civilisation does it now.

No you are from at first you assume m_civilisation is a DynamicArray, but then you use it as index. Probably it is rather a pointer on something. And is returned by GetCivilisation. So reguess again.

-Martin

[Ekmek]

Originally posted by Martin Gühmann
Maybe Player has a member m_owner, but if you are in a Player member function you don't need to get a pointer on a Player object if it points on the same address in the memory as the this pointer.

Wait, relooking this again. Are saying that with the Player::CanBuildUnit I don't need g_player just:

if(irec->GetCultureOnlyIndex(s) == m_owner->GetCivilisation()->GetCityStyle()) {

but in player I do see instances of g-player[m_owmer] like in the same function Player::CanBuildUnit

	n = g_player[m_owner]->m_all_units->Num();
		for(i = 0; i < n; i++) {
			if(m_all_units->Access(i).GetDBRec()->GetSlaveRaids())
				return FALSE;
		}

[Martin Gühmann]

Originally posted by E
Wait, relooking this again. Are saying that with the Player::CanBuildUnit I don't need g_player just:

if(irec->GetCultureOnlyIndex(s) == m_owner->GetCivilisation()->GetCityStyle()) {

No, that doesn't even compile. m_owner is a player index, and not a pointer on a player. You can only use it in arrays or functions that needs to access a player from a list, like g_player is such a list. And it is implemented as the simplest list like construct a plain array.

Originally posted by E
but in player I do see instances of g-player[m_owmer] like in the same function Player::CanBuildUnit

	n = g_player[m_owner]->m_all_units->Num();
		for(i = 0; i < n; i++) {
			if(m_all_units->Access(i).GetDBRec()->GetSlaveRaids())
				return FALSE;
		}

It is nice that you look at other people's code, but you shouldn't think that it is correct just because a programmer wrote it or the way as it is programmed it is the best one. In fact you find a lot of incorrect code in the CTP2 source and bad style code.

One think for instance is that you find in the source code some goto commands. It is actual very frowned upon. If it is used it may be a sign of very good, clever and intellegent way of programming. But this is rather the seldom case. More frequent it is a sign of a very bad way of programming. And in the CTP2 gotos I saw so far are all of the latter type.

The bit of code above, is a not very ellegant way of programming, because g_player[m_owner] is equal to the this pointer at least it should be equal to the this pointer.

So you do an operation to access a array to retrieve a pointer that you want then to use, that is one additional operation than if you use the this pointer directly.

Of course you can rely on the optimizer of your compiler, but that is again a bad style of programming, at least if it is so simple like here to do the optimization yourself.

And of course the is a final point if you aren't so familiar with the code then prbably you don't know that g_player[m_owner] is the this pointer, so he could think here is another object of type player accessed. But this isn't true.

So this point is about making the code easier to understand and that is also the point about the goto, with the goto it is hard to see where it brings you to.

And now you should get it with the this pointer, this is important. And of course use your C++ for Dummies.

-Martin