Limiting City placement/terrain specific cities

[Ekmek]

This has probably been asked before but...

How do I make it so cities can only be built on grassland and plains tiles.

I've seen that I can restrict it to land and sea, how do I get it for specific tiles...

On top of thatcan I have later game settlers build on other specific terrain?

how do I get sea cities look different than normal cities?

I'd like to eventually have unique looking desert cities, arctic cities etc for my mod.

thanks....

[Martin Gühmann]

Originally posted by E
How do I make it so cities can only be built on grassland and plains tiles.

I've seen that I can restrict it to land and sea, how do I get it for specific tiles...

By using slic, that is triggered on the Settle events. And prevents the events from beeing executed of course if the conditions are met. The only problem is that the AI might not handle it well, or you put it under slic control. Conditions fro preventing it from being executed are the location's terrain types.

Originally posted by E
On top of thatcan I have later game settlers build on other specific terrain?

Then add another condition that is unit specific.

Originally posted by E
how do I get sea cities look different than normal cities?

By using the Apolytonm edition of CTP2.

Originally posted by E
I'd like to eventually have unique looking desert cities, arctic cities etc for my mod.

Also another thing that is hard encoded and can only be changed in the source code.

-Martin

[Ekmek]

I guess it might be easier to see how the code hands settle-land and create settle grass, or settle plains instead of building a big slic...

but looking at units.txt It could be podssible to have terrain specific cities if I could make certain settlers settle on certain tiles...

I found this in unit.cpp

BOOL Unit::CanSettle(const MapPoint &pos) const
{
    return GetData()->CanSettle(pos); 
} 

BOOL Unit::IsSettleLand() const
{
    return GetDBRec()->GetSettleLand(); 
} 

BOOL Unit::IsSettleMountain() const
{
    return GetDBRec()->GetSettleMountain(); 
} 

BOOL Unit::IsSettleWater() const
{
    return GetDBRec()->GetSettleWater(); 
}

and found this in unitdata.cpp

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
	}

	if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == FALSE) {
		return FALSE; 
	}

	if (g_theWorld->HasCity(pos)) 
		return FALSE; 

	if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		searching = FALSE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		searching = FALSE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		searching = FALSE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		searching = FALSE; 
     
	if (searching) 
		return FALSE; 

    return TRUE; 
}

and in citydata.cpp

	if(!rec->GetMovementTypeLand() && !rec->GetMovementTypeTrade() && !rec->GetIsTrader()) {
		if(g_theWorld->IsWater(pos.x, pos.y) && rec->GetSeaCityCanBuild()) {
			
			
			return TRUE;
		}
		
		if((g_theWorld->IsLand(pos.x, pos.y) || 
			g_theWorld->IsMountain(pos.x, pos.y)) &&
			rec->GetLandCityCanBuild()) {
			
			return TRUE;
		}
		
		
		if(rec->GetMovementTypeSea() || rec->GetMovementTypeShallowWater()) {
			
			if(g_theWorld->IsNextToWater(pos.x, pos.y)) {
				return TRUE;
			}
			
			return FALSE;
		} else if(rec->GetMovementTypeAir()) {
			return TRUE;
		}
		return FALSE;
	}

	
	return TRUE;
}

I think I'm learning my way around the code. And I think like your tutorial on SLIC I need to add something here in wrlEnv.cpp

sint32 World::EnvIsLand(const uint32 env) const
{
	return ((env & k_MASK_ENV_MOVEMENT_TYPE) & k_BIT_MOVEMENT_TYPE_LAND) != 0; 
}

sint32 World::IsLand(const MapPoint &pos) const

{
	return EnvIsLand(GetCell(pos)->m_env);

}

sint32 World::IsLand(const sint32 x, const sint32 y) const

{ 
	Assert (m_isYwrap ? (-k_MAP_WRAPAROUND < y) : 0 <= y); 
	Assert (m_isYwrap ? (y < (m_size.y + k_MAP_WRAPAROUND)) : y < m_size.y); 
	
	return EnvIsLand(m_map[x][y]->m_env);

I know I'm an annoying amateur, but I'm thinking it is here we could add terrain specific settle spots. I'd like to find some little code to practice on first but it looks like everything requires a lot of work ...I was going to add a IsOpenLand type for plains and grasslands, but I wanted to check in first, don't want to screw up the code.

[Martin Gühmann]

The better idea is to make it terrain specific rather than class specific. It should depent on the terrain records from the database. To modify the text files according to units should be the easiest task as these files are generated automaticly by dbgen.

You should take a look into the *.cdb files. Oh and of course I don't tell you were you can find them, because you should already be familiar with the builtin windows file search engine.

The second problem is to make CTP2 use the new stuff and of course to make it backwards compartible. But to that when you have modified the unit.txt.

-Martin

[Ekmek]

Thanks Martin. Looking at the way the world.h used it and trying to follow the links for SettleLand and is Land. It looks like t identifies everything by MovementType.

The terrain.txt requires a field that is Movementtype and I think it is there that things are defined as land or sea. I'm thinking of just adding more MovementType based on tile names (plains, grassland, etc) plus Extra 1 to 10 (for modders).

I think Settle:Land loks for MovementType:Land so if I change things to have new Movement Types than it might work. Of course for grassland tiles I'll have to have Movement Type:Land and Movementtype: Grass and update the txt for a later release (like you said easiest part).


I'll mess with this probably this weekend (and the SLIC stuff too) I just hope the more professional programmers, like yourself, wil be available to check my work.

THANKS!

[Martin Gühmann]

Originally posted by E
The terrain.txt requires a field that is Movementtype and I think it is there that things are defined as land or sea. I'm thinking of just adding more MovementType based on tile names (plains, grassland, etc) plus Extra 1 to 10 (for modders).

I think Settle:Land loks for MovementType:Land so if I change things to have new Movement Types than it might work. Of course for grassland tiles I'll have to have Movement Type:Land and Movementtype: Grass and update the txt for a later release (like you said easiest part).

So you really like to force every modder to change the source code if he wants to add a new terrain? Actual I meant the movent type should be replaced by terrain type. And terrain type is determined by the entries you have in your terrain database, the actual database records no hard encoded builtin stuff.

-Martin

[Ekmek]

Ahhhhhhhhh!

Just have it look elsewhere, right? Just have it check terraintype then make the unit.txt have settle:grassland etc for each settler etc.

Ok, I'll take a look. I guess as a side benefit it could make some more unique units like Jungle guerrillas, desert camels...

[Martin Gühmann]

Actual I thought of something like this:

   CantBuildOn TERRAIN_BROWN_MOUNTAIN
   CantBuildOn TERRAIN_GLACIER
   CantBuildOn TERRAIN_MOUNTAIN
   CantBuildOn TERRAIN_TUNDRA
   CantBuildOn TERRAIN_WATER_BEACH
   CantBuildOn TERRAIN_WATER_DEEP
   CantBuildOn TERRAIN_WATER_RIFT
   CantBuildOn TERRAIN_WATER_SHALLOW
   CantBuildOn TERRAIN_WATER_SHELF
   CantBuildOn TERRAIN_WATER_TRENCH
   CantBuildOn TERRAIN_WATER_VOLCANO
   CantBuildOn TERRAIN_WHITE_HILL
   CantBuildOn TERRAIN_WHITE_MOUNTAIN

This is from tileimp.txt, of course it should be something like CanSettleOn or CantSettleOn instead of CantBuildOn and of course this would go into the settler unit records.

-Martin

[Ekmek]

I found this in terrainimprovementrecord.cpp

sint32 TerrainImprovementRecord::GetCantBuildOnIndex(sint32 index) const
{
    Assert(index >= 0);
    Assert(index < m_numCantBuildOn);
    if((index < 0) || (index >= m_numCantBuildOn)) {
        return 0;
    }
    return m_CantBuildOn[index];
}

const TerrainRecord *TerrainImprovementRecord::GetCantBuildOn(sint32 index) const
{
    Assert(index >= 0);
    Assert(index < m_numCantBuildOn);
    if((index < 0) || (index >= m_numCantBuildOn)) {
        return 0;
    }
    return g_theTerrainDB->Get(m_CantBuildOn[index]);
}

theoretically I should be able to port this into unitdata.cpp in the settle areas right?

[Martin Gühmann]

Originally posted by E
theoretically I should be able to port this into unitdata.cpp in the settle areas right?

Of course you should use the equivalent in UmitRecord you have still to create and you do that by modifying one of these *.ccdb files.

-Martin

[Ekmek]

If found this in terrain CDB. I think I could add all my terrain types here, but I think that will create the problem that modders have to mod the code. So should I copy the terrain movement types into settle types ? I'm kind of lost at how to link it to a terrain name, but I'll keep digging.

 unit.cdb
	Bits Settle { 
		Land,
		Water,
		Mountain,
		Space

probably add a can'tsettle?

 terrain.cdb
	Bits MovementType {
		Land,
		Sea,
		Air,
		Mountain,
		Trade,
		ShallowWater,
		Space
	}		

	# Map record to the pseudo-enum (TERRAIN_*, not an enum any more)
	Bits InternalType {
		Forest,
		Plains,
		Tundra,
		Glacier,
		Grassland,
		Desert,
		Swamp,
		Jungle,
		Mountain,
		Hill,
		WaterShallow,
		WaterDeep,
		WaterVolcano,
		WaterBeach,
		WaterShelf,
		WaterTrench,
		WaterRift,
		Dead,
		BrownHill,
		BrownMountain,
		WhiteHill,
		WhiteMountain,
		WaterKelp,
		WaterReef,
		Special
	}

	# For gfx system use
	Int TilesetIndex
}

 terrimprove.cdb
Record Terrain[] CantBuildOn

Or should I just copy the terrimprove cantbuild on and rename it and put it into the unit.cdb?

[Martin Gühmann]

Originally posted by E

 terrimprove.cdb
Record Terrain[] CantBuildOn

Or should I just copy the terrimprove cantbuild on and rename it and put it into the unit.cdb?

Actual this is the one I have in mind, you have here a list of terrain, and I think we should go for a CanSettleOn flag. And of course it should be backwards compartible, if the unit does not have any CanSettleOn entry it should go for the old terrain class thingy stuff.

-Martin

[Ekmek]

martin,

Question #1
I added CanSettleOn into Unit.cdb assuming that its almost a mirror copy however as I went into UnitRecord.h (at the top it says don't edit it, does that still apply?) I was taking stuff from terrimproverecord.h then realized that this code should start copying the normal settle code to allow city settling (as opposed to not building tiles)

I think its at this part of the code that I should start copying:

 from newdb/UnitRecord.h
 // Settle flag group
    uint32           GetSettle() const { return m_Settle; }
    bool             GetSettleLand() const { return (m_Settle & k_Unit_Settle_Land_Bit) != 0; }
    bool             GetSettleWater() const { return (m_Settle & k_Unit_Settle_Water_Bit) != 0; }
    bool             GetSettleMountain() const { return (m_Settle & k_Unit_Settle_Mountain_Bit) != 0; }
    bool             GetSettleSpace() const { return (m_Settle & k_Unit_Settle_Space_Bit) != 0; }
    // End Settle flag group
    //
    sint32           GetSettleCityTypeIndex() const { return m_SettleCityType; }
    const UnitRecord *GetSettleCityType() const;
    sint32           GetSettleSize() const { return m_SettleSize; }
    sint32           GetSettleBuildingIndex(sint32 index) const;
    const BuildingRecord *GetSettleBuilding(sint32 index) const;
    sint32           GetNumSettleBuilding() const { return m_numSettleBuilding;}
    bool             GetSpaceLaunch() const { return (m_flags0 & k_Unit_SpaceLaunch_Bit) != 0; }
    bool             GetSpaceLaunch(sint32 &value) const {
                         if((m_flags0 & k_Unit_SpaceLaunch_Bit) == 0) return false;
                         value = m_SpaceLaunchValue;
                         return true;

I could just do a name replace but I'm thinking I have to put a reference to the index where the GetSettleLand stuff is. Something like:

sint32           GetCanSettleOnIndex(sint32 index) const;
    const TerrainRecord *GetCanSettleOn(sint32 index) const;
    sint32           GetNumCanSettleOn() const { return m_numCantBuildOn;}

I'm thinking I should either add the above code in the settle section or copy the settle section and then just replace.


Question #2
I know I have to link it to settle some how and since its Can instead of Cant I figure I have to change the true\false statement.

		for(i = 0; i < rec->GetNumCantBuildOn(); i++) {
			if(rec->GetCantBuildOnIndex(i) == cell->GetTerrain()) {
				return FALSE;

I thinking I add it like this:

 from UnitData.cpp
BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
	}

	for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
			if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) {
				return FALSE;

	if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == FALSE) {
		return FALSE; 
	}

	if (g_theWorld->HasCity(pos)) 
		return FALSE; 

	if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		searching = FALSE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		searching = FALSE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		searching = FALSE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		searching = FALSE; 
     
	if (searching) 
		return FALSE; 

    return TRUE; 
}

I think this is a small problem but if anyone could provide a recommendation I'd appreciate it.

[Martin Gühmann]

Originally posted by E
martin,

Question #1
I added CanSettleOn into Unit.cdb assuming that its almost a mirror copy however as I went into UnitRecord.h (at the top it says don't edit it, does that still apply?) I was taking stuff from terrimproverecord.h then realized that this code should start copying the normal settle code to allow city settling (as opposed to not building tiles)

You don't touch any of these *Record.* files they are generated by dbgen automaticly, that is the sense of these *.cdb files.

Originally posted by E
Question #2
I know I have to link it to settle some how and since its Can instead of Cant I figure I have to change the true\false statement.

		for(i = 0; i < rec->GetNumCantBuildOn(); i++) {
			if(rec->GetCantBuildOnIndex(i) == cell->GetTerrain()) {
				return FALSE;

As the function you are in should return whether a the given unit can settle at the current location it should return TRUE. However you should close all your blocks you open with a brace '{'.

I thinking I add it like this:

Originally posted by E

 from UnitData.cpp
BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
	}

	for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
			if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) {
				return FALSE;

	if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == FALSE) {
		return FALSE; 
	}

	if (g_theWorld->HasCity(pos)) 
		return FALSE; 

	if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		searching = FALSE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		searching = FALSE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		searching = FALSE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		searching = FALSE; 
     
	if (searching) 
		return FALSE; 

    return TRUE; 
}

That's actual not a good idea even if you add the closing braces and make the new code return TRUE. You would still allow, then to settle in cities with your new code. However for the sake of backwards compartibility you should put the code into a if-else instructions. The new code comes under if condition, the condition is whether the CanSettleOn array as more than zero members, meaning it exists. Otherwise the else block is executed that contains the old code with the GetSettleLand etc. However this searching variable is superfluous all the code related to it can be deleted, when this variable is set to FALSE, the function should return TRUE.

-Martin

[Ekmek]

I guess this should be moved to the source code forum but anyways...

Originally posted by Martin Gühmann

That's actual not a good idea even if you add the closing braces and make the new code return TRUE. You would still allow, then to settle in cities with your new code. However for the sake of backwards compartibility you should put the code into a if-else instructions. The new code comes under if condition, the condition is whether the CanSettleOn array as more than zero members, meaning it exists. Otherwise the else block is executed that contains the old code with the GetSettleLand etc. However this searching variable is superfluous all the code related to it can be deleted, when this variable is set to FALSE, the function should return TRUE.

-Martin

Martin, thanks for taking the time to help me with this. I really hope yu don't feel anoyed because I'm really learning a lot. and if anything if I grasp enough of this I'll stop pestering you with requests

Having said that, I think understand what you said and I made these code adjustments...

 from UnitData.cpp
BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
	}


	if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == FALSE) {
		return FALSE; 
	}

	if (g_theWorld->HasCity(pos)) 
		return FALSE; 

	if(rec->GetCanSettleOnIndex() == cell->GetTerrain()) { 
				return TRUE;
               } 

	else if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		searching = FALSE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		searching = FALSE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		searching = FALSE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		searching = FALSE; 
     
	if (searching) 
		return FALSE; 

    return TRUE; 
}

Am I getting closer? and I didn't see any other references to Settle and CantBuildOn so I think this is the only place in the code to do this.

Seriously, thanks for the help.

[Martin Gühmann]

Originally posted by E
Am I getting closer? and I didn't see any other references to Settle and CantBuildOn so I think this is the only place in the code to do this.

Yes you are getting closer, but still you aren't at the end. The code still doesn't work, the code you have added still belongs into a for loop and now I think this should be enough, however the code should still be optimized by removing this searching variable.

-Martin

[Ekmek]

Thanks martin,
this is the change I added to remove the searching function. Not sure where else to go in regards to the for loop. But you did say this enough so this is the code I've added. I'll attach it here until you say its worthy of going into altered files...

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
	}


	if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == FALSE) {
		return FALSE; 
	}

	if (g_theWorld->HasCity(pos)) 
		return FALSE; 
      if(rec->GetCanSettleOnIndex() == cell->GetTerrain()) { //added by E
		return TRUE;
	 }
	if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		searching = FALSE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		searching = FALSE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		searching = FALSE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		searching = FALSE; 
     
	//if (searching)   --->E add notation to remove searching function recommend by Martin G 
	//return FALSE;    --->E add notation to remove searching function recommend by Martin G

    return TRUE; 
}

[Martin Gühmann]

This still doesn't work I said that you should insert you initial for loop at the place where you inserted the code fragment the time before.

Next thing seraching is not a function it is a variable of type sint32 that is that initially to TRUE, as far as I know True is a #define and with the value 1.

This searching variable is still in the function, actual I asked you to comment out every instance of it. And I asked you to do another thing, namly to insert everywhere a return TRUE; where it was originally set to FALSE.

-Martin

[Ekmek]

Ok, I added back in the for code that I had before, and removed my simple coded and commented out the searching stuff. Does the for statement work now? Do I need to define the variable "i" ?
I think I'm understanding your responses but thanks for your help. slowly but surely I'm learning.

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	//sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		//return FALSE; 
                               return TRUE;
	}

for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
			if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) {
				return TRUE;


	else if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == TRUE) {
		//return FALSE; 
                               return TRUE;
	}

	else if (g_theWorld->HasCity(pos)) 
		//return FALSE;
                               return TRUE; 
      
	 }
	else if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		//searching = FALSE;
                                 return TRUE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		//searching = FALSE;
                               return TRUE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		//searching = FALSE;
                                return TRUE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		//searching = FALSE; 
                                return TRUE;
     
	//if (searching)    
	//return FALSE;   

    return TRUE; 
}

[Martin Gühmann]

Originally posted by E
Ok, I added back in the for code that I had before,

You added back the for loop but you should add it right before the code that checks in the original version whether the unit has the right properties for instance whether it can settle on and its current position is land.

Originally posted by E
and removed my simple coded and commented out the searching stuff.

You removed it and a lot more, now the function returns always TRUE no matter what you do. You should have replaced everytime if the searching variable was set to FALSE by return TRUE; I never talked about any return FALSE; statements. And there is another thing I think I didn't talked about yet. The last return TRUE; in the function must become a return FALSE; sice we have removed the searching variable.

Originally posted by E
Does the for statement work now?

No, it still doesn't work, it even doesn't compile sice you have forgotten to close the two blocks that were opened by an open brace '{'. Of course you do this by adding closing braces '}'. One for closing the if block after the return and another one to close the block of the for loop afterwards you have closed the if block.

Originally posted by E
Do I need to define the variable "i" ?

Of it needs to be defined before, since the i should be just a counting variable, you can do it in the same way like the searching variable, and the same place where it was defined at the top of the function, but you can also define it right before the for loop. Actual this is the way to do it if you have something else then a primitive type or a pointer.

-Martin

[Ekmek]

ok I think I got it, but maybe I left something out...

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	//sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
                
	}


	else if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == TRUE) {
		return FALSE; 
                               
	}

       for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
			if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) }
				return TRUE;

	else if (g_theWorld->HasCity(pos)) 
		return FALSE;
        else if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		//searching = FALSE;
                                 return TRUE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		//searching = FALSE;
                               return TRUE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		//searching = FALSE;
                                return TRUE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		//searching = FALSE; 
                                return TRUE;
     
	//if (searching)    
	//return FALSE;   

    return FALSE; 
}

[Martin Gühmann]

Originally posted by E
ok I think I got it, but maybe I left something out...

Yes we are getting closer.

You forgot to define the i.

And instead of closing the two {}-blocks you have opened in the for loop you replaced the second open brace by a closing brace.

So again the return TRUE; under the for loop belongs into the if block of the for loop and of course this if block needs also to be encapsulated. So the foor loop opens a {}-block and the following if is within this block that opens also a {}-block, that contains a return TRUE;

And another problem you cannot continue after a for loop with an else if there is no coresponding if, so turn the else if right after the for loop into a simple if.

-Martin

[Ekmek]

Thanks Martin. Hope your patience isnt getting exhausted but if its not too time consuming maybe your work with me and bigMC can sort of make up how short we are on programmers

I think I got it, now, I'll post my file after I get a satisfactory grade from the teacher and hopefully my next homework goes better (I did learn a lot thanks)

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 

{ 
  	//sint32 searching = TRUE;  
                sint32 i;
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
                
	}


	else if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == TRUE) {
		return FALSE; 
                               
	}

       for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
			if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) {
			return TRUE;
       }
}

	if (g_theWorld->HasCity(pos)) 
		return FALSE;
        else if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		//searching = FALSE;
                                 return TRUE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		//searching = FALSE;
                               return TRUE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		//searching = FALSE;
                                return TRUE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		//searching = FALSE; 
                                return TRUE;
     
	//if (searching)    
	//return FALSE;   

    return FALSE; 
}

[Martin Gühmann]

Looks compilable now, and it should do what you indend to do. So let's come to the aesthetical part and clean up the lay out. C++ is a layout free language, that means it doesn't matter how much white space (spaces, new lines, tabs) you insert into the code or how less. But for a human reader it does mater for the sake of readibility.

Well as our server is still not running properly I would continue with the ACTIVISION_ORIGINAL stuff.

So copy the original code, to the above code in the souce file. Mark the beginning of the old code with

#if defined(ACTIVISION_ORIGINAL)

Seperate the new code with the preprocessor derective

#else

And mark the end of the new code with

#endif

Look into the code for examples for this.

Now cleanup your code the easiest thing is to remove the outcommented bits of old code.

Now to the next step: idention

Whenever you open a block with a brace '{' use one tab to indent the following code. However a certain number of spaces also do the job, that is actual a matter of preference, however I would do it like it is done in the rest of the file. And there we have tabs.

Here is a code fragment for example:

if(something){
	//Indent the inside code by one tab
	for(i = 0; i < something_else; ++i){
		//Indent the inside code by one tab again
	}
}

The most important thing is that you see on a glance which piece of code belongs into which block, and you can also see whether you have forgotten to close a block by a closing brace '}'.

I prefer a compact style, no space between the if or for and the following open parenthesis '('. And I put the brace that follows the if or for onto the same line, and without spaces as well. In another style the following brace is put on a new line. And the following code is put on another new line. I don't like this style, because it adds so much lines without any information, maybe accept that you can find the matching pairs of braces better. But that is no gain if I see then just half of the code on one glance.

What you have to do now is to remove some tabs at one place and add some at another place, so that it looks well. And some new lines should be removed, as they don't make the code clearer. Of course the ar also new lines that makes the code clearer. But I guess this lies in the eye of the beholder.

And another little request, could you post the additions to the unit.cdb, just to check whether everything is alright there.

-Martin

[Ekmek]

Martin,
Thanks. I'll start on my clean up in the mean time here is what I put in unit.cdb

Record Advance             EnableAdvance
	Record Advance[0..5]        ObsoleteAdvance
	Record Government[] GovernmentsModified
	Record Terrain[] CantSettleOn //added by E

	Bits Size {
		Small,
		Medium,
		Large
	}

[Ekmek]

Martin,

Here are the files so you can check my formatting. Thanks for the help!

[Martin Gühmann]

First it is not the same code you have posted above and the format looks terrible.

If I define ACTIVISION_ORIGINAL, it doesn't generate the original code at least the code does the same. But if I don't define it, it doesn't matter anymore whether the settle unit has pop and can build or not.

And the formating is a mixture of tabs and spaces. So again to do the ACTIVISION_ORIGINAL part, do soemthing like this:

#if defined(ACTIVISION_ORIGINAL)
	 //Original code by Activision, does not contain 
	 //any changes, and no format changes, either.
#else
	 //Your code including all the alteration, 
	 //since you modified, the half function you
	 //you put the functions old and new into the
	 //the according blocks as a whole. 
#endif

The next step is to remove all these spaces from your code now, and replace them by tabs (the key left of the Q) if necessary. And again if you open a block with a brace the following comes into a new line with one tab of indention. If you close a block you close it with a closing brace, and that brace should get a new line and should have the same indetion like the if, for, etc. that opened its block, so that you can easily see, which closing brace, belongs to which if. Then else if and else, occur never without an if, therefore they get the same level of indetion like the coresponding if so that you can see which else or else if belongs to which if.

-Martin

[Ekmek]

Ok I gave formatting another go, but I did notice that the activision 'If-else' statements have braces around their 'return false' but they did not do it with the later 'SettleLand' stuff. Should I added braces there or is it not necessary?

Also I'm editing in WordPad, is there a better program to use (Notepad was a problem sometimes)

BOOL UDUnitTypeCanSettle(sint32 unit_type, const MapPoint &pos) 
      {
#if defined(ACTIVISION_ORIGINAL)
     sint32 searching = TRUE;    
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
  	sint32 t = rec->GetSettleCityTypeIndex();

	if (t < 0) {
		return FALSE; 
	}

	if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == FALSE) {
		return FALSE; 
	}

	if (g_theWorld->HasCity(pos)) 
		return FALSE; 

	if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
		searching = FALSE; 
	else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
		searching = FALSE; 
	else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
		searching = FALSE; 
	else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
		searching = FALSE; 
     
	if (searching) 
		return FALSE; 
#else
	sint32 i;
	const UnitRecord *rec = g_theUnitDB->Get(unit_type);   
	sint32 t = rec->GetSettleCityTypeIndex();
	if (t < 0) {
		return FALSE;      
	}
		else if (g_theUnitDB->Get(t)->GetHasPopAndCanBuild() == TRUE) {
			return FALSE;                               
		}
	for(i = 0; i < rec->GetNumCanSettleOn(); i++) {
		if(rec->GetCanSettleOnIndex(i) == cell->GetTerrain()) {
			return TRUE;
		}
	}
	if (g_theWorld->HasCity(pos)) 
		return FALSE;
		else if (rec->GetSettleLand() && g_theWorld->IsLand(pos))
			return TRUE; 
		else if (rec->GetSettleMountain() && g_theWorld->IsMountain(pos))
			return TRUE; 
		else if (rec->GetSettleWater() && g_theWorld->IsWater(pos))
			return TRUE; 
		else if (rec->GetSettleSpace() && g_theWorld->IsSpace(pos))
			return TRUE;
#endif
	return FALSE;
}

[Ekmek]

and the files

[Martin Gühmann]

At first, looks better now, but still we aren't at the end.

Originally posted by E
Ok I gave formatting another go, but I did notice that the activision 'If-else' statements have braces around their 'return false' but they did not do it with the later 'SettleLand' stuff. Should I added braces there or is it not necessary?

That's a good question, and in fact the is a difference. Everything within the braces following the if are under the condition of the if. If the following is not included within braces, only the statement directly following is conditioned by the if.

int i = 8;
if(something){
	i = i + 1;
	i++;
	--i;
	i+=2;
}

If something == false the value of i is at the end after the last statement 8, because nothing in the if block was executed.

int i = 8;
if(something)
	i = i + 1;
	i++;
	--i;
	i+=2;

In this case i equal 10, only the statement right after the if is not executed if something equal false.

Of course for readibility you would only indent the code dependent on the if.

int i = 8;
if(something)
	i = i + 1;
i++;
--i;
i+=2;

Originally posted by E
Also I'm editing in WordPad, is there a better program to use (Notepad was a problem sometimes)

What about EditPlus 2, it has syntax highlighting and Locutus has a set of syntax files for slic, and there should also be some updated ones here around in the forum.

Now let's come to the layout, the first else if statement is indented in comparison to its coresponding if. Actual the else if code is executed if the condition of the if is false, and therefore they are on the same level and get equal indention.

For the sake of readibility I also would sourround the for loop in the middle with new lines. However this is really a matter of prefference and taste.

And the first brace was in the original code on the line start. And should also be there if you give it a new line like in the rest of the file.

And now to the ACTIVISION_ORIGINAL statement, the old function retuned TRUE, if the execution came to the last statement, not return FALSE as it is now even if you define ACTIVISION_ORIGINAL. However if you put the preprocessor derectives into the function, then you have to include the last return as you have changed it.

-Martin