PROJECT: Good Specific Terrain Improvements

[Ekmek]

I'm still looking and I'm going to go back to my Dummies to get the terminology down right but what I'm seeing is that the code should be:

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

but you said that was wrong before so I'm going to keep digging because I don't think it should be:

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

but these are the only two options on seeing based on examples in the code.

[Martin Gühmann]

Originally posted by E
but you said that was wrong before so I'm going to keep digging because I don't think it should be:

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

And why do you think this is wrong?

-Martin

[Ekmek]

because I thought it was too easy an answer....

I guess this worked but only ifthe function is in player.cpp so it already knows it applies to the player. My lack of C++ vocabulary confused me. I'll make the changes and post the files to this thread.

[Ekmek]

Martin,

I made the changes. One thing I want to double check though:

Terrailutil.cpp

		if(rec->IsRestrictedToGood == 0) {
			for(i = 0; i < rec->GetNumCantBuildOn(); i++) {
				if(rec->GetCantBuildOnIndex(i) == cell->GetTerrain()) {
					return false;
				}
			}
		} 
		else {
			sint32 good;
			if (g_theWorld->GetGood(pos, good)) {
				for(i = 0; i < rec->GetNumIsRestrictedToGood(); i++) {
					if(rec->GetIsRestrictedToGood(i) == good) {
						return true; 
					}  
				}	 
				return false;
			}
		}
	}

When we went through this code awhile ago so it should be good, but I don't understand why the first line is:
if(rec->IsRestrictedToGood
and not CantBuildOn like the code was originally. Will the game still check the CantBuildOn in this case?

attached are all the files cdb and cpp.
{removed}

[Martin Gühmann]

Originally posted by E

Terrailutil.cpp

		if(rec->IsRestrictedToGood == 0) {
			for(i = 0; i < rec->GetNumCantBuildOn(); i++) {
				if(rec->GetCantBuildOnIndex(i) == cell->GetTerrain()) {
					return false;
				}
			}
		} 
		else {
			sint32 good;
			if (g_theWorld->GetGood(pos, good)) {
				for(i = 0; i < rec->GetNumIsRestrictedToGood(); i++) {
					if(rec->GetIsRestrictedToGood(i) == good) {
						return true; 
					}  
				}	 
				return false;
			}
		}
	}

When we went through this code awhile ago so it should be good, but I don't understand why the first line is:
if(rec->IsRestrictedToGood
and not CantBuildOn like the code was originally. Will the game still check the CantBuildOn in this case?

Yes we went through a while ago. But there are two possibilities that happend.

First I overlooked that it doesn't work.
Second possibility you forgot to fix something I told you to fix.

The indention was that it is a stupid idea to restrict a tile improvement to a certain good and to forbid the construction on the underlying terrain.

So the design is if there is no good restriction the original code is used and the CanBuildOn stuff is used. If there is at least one restriction to a certain good your new code is used.

But the code does not to this what I just described. It even does not compile, because you didn't do the right test whether there are any restrictions to certain goods.

So again how do you get figure out that the number of your good restrictions is zero?

Well I leave this queston to you.

-Martin

[Ekmek]

Well I know I have to GetNum so I think the code should be:

		if(rec->GetNumIsRestrictedToGood () == 0) {
			for(i = 0; i < rec->GetNumCantBuildOn(); i++) {
				if(rec->GetCantBuildOnIndex(i) == cell->GetTerrain()) {
					return false;
				}
			}
		} 
		else {
			if(irec->GetNumIsRestrictedToGood() > 0) {
			sint32 good;
				if (g_theWorld->GetGood(pos, good)) {
					for(i = 0; i < rec->GetNumIsRestrictedToGood(); i++) {
						if(rec->GetIsRestrictedToGood(i) == good) {
							return true; 
						}  
					}	 
				return false;
				}
			}
		}

BUT I added that extra line for the check but if thats wrong then the code should be:

		if(rec->GetNumIsRestrictedToGood () == 0) {
			for(i = 0; i < rec->GetNumCantBuildOn(); i++) {
				if(rec->GetCantBuildOnIndex(i) == cell->GetTerrain()) {
					return false;
				}
			}
		} 
		else {
			sint32 good;
			if (g_theWorld->GetGood(pos, good)) {
				for(i = 0; i < rec->GetNumIsRestrictedToGood(); i++) {
					if(rec->GetIsRestrictedToGood(i) == good) {
						return true; 
					}  
				}	 
				return false;
			}
		}

Where there any other code problems with the CultureOnly, etc?
{removed}

[Martin Gühmann]

Originally posted by E
Well I know I have to GetNum so I think the code should be:

Everything else would be a surprise.

Originally posted by E
BUT I added that extra line for the check but if thats wrong then the code should be:

Why do you want the extra check. At first it doesn't compile, because you forgot a closing brace, and arrays can't have a size that is smaller then 0. If you really want to check this you can ask whether you have less or zero elements in your IsRestrictedToGood array and if this holds you execute the original code otherwise your new code.

Originally posted by E
Where there any other code problems with the CultureOnly, etc?

Well I told you that g_player is a list of players and not player properties. You didn't look on the descriptions of the functions in terrainutil.cpp.

Then something like this TerrainImprovementRecord *rec is not a variable for improvement flag it is a terrain improvement record in the terrain improvement database.

In comparision to this:

// Parameters :  sint32 pl              : Variable for player

This is wrong:

//              pl: The player's attributes if they can build wonder

It is rather the index of the player that is checked for whether the terrain improvement in question can be build.

Then this:

// - Added  a check in CanPlayerBuild for the CultureOnly flag to see if a Unit  
//   is limited to certain CityStyles (E 2005/03/12)

Well that is nice that the unit is limited to a certain city style. Of which style by the way the style of the owner city or the owner civilisation and why are you talking about units in terrainutil.cpp

// - Added a check in CanPlayerBuild for the GovernmentOnly flag to see if a Unit  
//   is limited to certain CityStyles (E 2005/03/12)

What do you mean limitation to certain city styles or to certain governments?

// - Added  a check in CanPlayerBuildAt for the IsRestrictToGood flag so an   
//   improvement can only be built on a good tile (-E 2005/03/12)

Should be a tile with a certain good on it. And of course these functions all start with the prefix terrainutil_.

Then let's take a look on the terrainutil_CanPlayerBuild function itsself. In this function there is no irec defined. Therefore this doesn't compile.

And lease use the GetGovernmentType function instead of m_government_type if you don't want to modify its value.

In the Player::CanBuildUnit method you find this irec thing again. And the description of the function tells you that it is called BOOL CityData::CanBuildUnit, of course this is wrong.

And the best is that the top of the file doesn't say anything about your changes you have done in this file.

OK let's come to CityData.cpp:

// - CityStyleOnly code added to CanBuildUnit, CanBuildBuilding, CanBuildWOnder 
//   by E April 20th 2005
// - Governmenttype and CultureOnly added to CanBuildBuilding and canBuildWonder
//   by E April 20th 2005

Nice that you have added these stuff but for what?

Again in CityData::CanBuildUnit is no irec defined. The only method that has an irec is CityData::CanBuildBuilding.

And CityData::CanBuildWonder has no irec as well, but there is no database record defined at all, so you have do more then renaming it in comparision to the other methods. You have to retrieve a wonder record first and please don't call it irec like city improvement record.

Well that should be all for now.

-Martin

[Ekmek]

Thanks for going easy on me. I updated all the stuff and attached here ( i did my wonder rec from what I saw in another file).

Everything else seemed straight forward.

Also I did install the C++for dummies CD and using its cpp program. It has a compiler too so as a warning I may be compiling soon.

{removed}

[Martin Gühmann]

Originally posted by E
Thanks for going easy on me. I updated all the stuff and attached here ( i did my wonder rec from what I saw in another file).

So far we aren't finished.

// - Added a check in terrainutil_CanPlayerBuild for the GovernmentOnly flag 
//   to see if a tile improvement is limited to certain player 
//   governments - added by E March 12th 2005

Actual this should be something along whether the player has the right government or his government is matching to the record you find in tileimp.txt. And check the description of the terrainuti.cpp again there are two added with two following spaces.

// Added GovernmentType flag from Units to use for Improvements
	if(rec->GetNumGovernmentType() > 0) {
		sint32 i;
		bool found = false;
		for(i = 0; i < rec->GetNumGovernmentType(); i++) {
			if(rec->GetGovernmentTypeIndex(i) == g_player[pl]->m_government_type->GetGovernmentType()) {
				found = true;
				break;
			}
		}
		if(!found)
			return false;
	}

m_government_type is a member of Player like GetGovernmentType(), so this doesn't compile. Actual you was supposed to replace all instances of m_government_type by GetGovernmentType() you have added in all the three files. This has the advantange that you cannot modify this variable by acciendent. And I can make all these members in the long run private.

The rest of terrainutil.cpp seems to be ok. So let's go to Player.cpp:

// - CultureOnly code added to CanBuildUnit; checks the CultureOnly flag to a
//   player's citystyle by E April 20th 2005

Nice that it checks it but what for?

// Remark(s)  : CultureOnly added by E; checks if player an unit have the
//              same CityStyle in order to build

Is this English? And by the way remove the last four trailing tabs.

OK let's go to CityData.cpp

//            : Added PrerequisiteBuilding check to see if a cit has a building
//              needed to build a unit.

A city without y.

// Added by E - checks if a city has a building required to build the unit
	if(rec->GetNumPrerequisiteBuilding() > 0) {
	    sint32 o;
    	for(o = 0; o < rec->GetNumPrerequisiteBuilding(); o++) {
			sint32 b = rec->GetPrerequisiteBuildingIndex(o);
			if(!(GetEffectiveBuildings() & ((uint64)1 << (uint64)b)))
				return FALSE;
		}
	}

And some additional code fortunatly for you it compiles and it is correct, but please stop now adding anything else, before the rest is correct. Therefore I had again to check the *.cdb files, fortunately there where correct as well.

Well, the only thing you can do is to check the white space, there are two tabs replaced by spaces in.

And there again is a m_government_type instead of a GetGovernmentType()

// Returns    : Whether the city can build the building specified by type.

This line is wrong at least if your method is called CityData::CanBuildWonder

//            : GovernmentType flag for wonderss limits wonders to govt type.

wonders with an s too much. And the rest of the CityData::CanBuildWonder looks ok, except that there are again some spaces where tabs should be. And I rather tought of a more generic name than wrec, so that you can paste the piece of code more easily into other locations. But well there is no real need to change it now.

Originally posted by E
Also I did install the C++for dummies CD and using its cpp program. It has a compiler too so as a warning I may be compiling soon.

Maybe you should finish this project first and then commit it to the respiratory. So that you have a good code base. I hope you have a Microsoft compiler, as noone as far as I know has compiled it on anything else.

-Martin

[Ekmek]

Martin,

Thanks for checking the stuff. Hopefully the end is insight. I didn't add anything new and I think once this code is done and I commit it to the svn I'm going to take a coding break. I think you need it I'll get back into my graphics I need to work on and go through my C++ book more and do the exercises. I notice I can understand the code fairly well (like with PreReqBuilding) but still not good enough to write so I'm going to do some self study and maybe take a class before I do any more coding after this.

[Ekmek]

Martin,
are their any problems or should I commit them to ther server now?

[Martin Gühmann]

Originally posted by E
Martin,
are their any problems or should I commit them to ther server now?

No, you shouldn't. The code seems to be ok, but there are problems with your comments.

CityData.cpp:

// - Governmenttype added to CanBuildWonder and CanBuildBuilding; Checks if
//   the pler's government matches the flag for the wonder or building - by E 
//   April 20th 2005

What is a pler?

And you didn't take care about the white space usage when you copied the PrerequisiteBuilding check. There are two tabs replaced by spaces. The same holds for the line when you retrieve the pointer on the wonder record.

Player.cpp seems to be ok.

terrainutil.cpp:

// - Added  a check in CanPlayerBuild for the CultureOnly flag to see if a   
//   Tile improvement is limited to certain CityStyles (E 2005/03/12)
// - Added a check in terrainutil_CanPlayerBuild to check a to see if player's 
//   government is matching to the record you find in tileimp.txt - (E 2005/03/12)
// - Added  a check in terrainutil_CanPlayerBuildAt for the IsRestrictToGood 
//   flag so a tile improvement can only be built on a tile with a 
//   certain good on it - (E 2005/03/12)

First there are three "Added"s after the first and the third "Added" there are 2 spaces after each even if there should be just one space.

And now two the middle statement. I don't think this is good English:

// - Added a check in terrainutil_CanPlayerBuild to check [b]a[/b] to see if player's 
//   government is matching to the record you find in tileimp.txt - (E 2005/03/12)

There is an indefinete article at the wrong place. And I think it is better to say that an additional check was added to figure out if the given unit is restricted to a certain types of government and whether the player has the according government. Your task by the way is now to sum it up so that it is still correct. And of course correct another thing terrainutil.cpp has nothing to do with units. It has to do with terrain and what you can build on it.

And this statement is also wrong:

// Name       :  terrainutil_CanPlayerBuildAt
//
// Description:  Checks terrain improvement properties to see if it can build 
//               on a tile only if it has a good

That's to focussed on your particular addition. The terrainutil_CanPlayerBuildAt is more general.

Well that should be now for today. And of course all the fixes has to be done.

-Martin

[Ekmek]

inching toward the finish line....I added your corrections, the only other thing I did was change wrec to rec like you suggested before (not replace I did each one by hand just to make sure I didn't miss) and in Unit.cdb I removed the futureuse flag NeedsBuildingToBuild since PreRequisiteBuilding was already added and you saw that it works. Other than that I made your corrections.

[Ekmek]

Martin,

Any other problems?

[Martin Gühmann]

Originally posted by E
Martin,

Any other problems?

No real problems, except that this line in CityData::CanBuildWonder still contains spaces even if it should be tabs, well actual a single tab:

    const WonderRecord* rec = wonderutil_Get(type);

Once you have done this you can commit it to the respository. Of course first you have to Update your files, so that all the stuff gets merged properly. But I think there shouldn't be any problems.

By the way MergeTortoise is a powerfull tool that can spot every space at the wrong place easily. And it is very helpfull to concentrate on your modifications.

I would say the next project is to fix the CanSettleOn feature at least the part that can be done in UnitData.cpp.

And maybe before you should get your compiler working. By the what compiler did you got your hands on?

-Martin