DEBUG: Boni of undersea tunnels for sea units bug

[NelsonAndBronte]

Hi Martin,

Have a look at UnitActor::GetIDAndType.

	if (isCity) {
		sint32			style;
		sint32			terrain;
		sint32			size;
		
		const TerrainRecord *rec = g_theTerrainDB->Get(g_theWorld->GetTileInfo(pos)->GetTerrainType());
		if(rec->GetMovementTypeLand() || rec->GetMovementTypeMountain()) {
			terrain = 0;
		} else {
			terrain = 1;
		}

The terrain values are decoded in citystyle.cdb as 0 = land, 1 = ocean.
As you see, if either GetMovementTypeLand() or GetMovementTypeMountain() returns TRUE, 'terrain' is set to 0. Therefore it looks to me as if your tunnel causes GetMovementTypeLand() to return TRUE and therefore you get a land city. I presume a different method should be used to determine if the underlaying cell is ocean or not.
I can't directly test any of this until I get my hands on a copy of CtP2.

[kaan]

It sounds plausible that underseatunnels return as land, thats the easy way to get land units to be able to walk in them.
this is the bug i have come to hate the most ;D

[NelsonAndBronte]

ugh... I think I was barking up the wrong tree in UnitActor::GetIDAndType. Althoug there is code there to retreive the terrain it doesn't seem to be used anyplace Dead code??

However, if we look at CityData::GetDesiredSpriteIndex the same basic error seems to be there -- a call to g_theWorld->IsLand(m_pos). AFAICS this will return TRUE if there is a tunnel in the square, and so the city sprite index returns a land city. Instead it should call g_theWorld->GetTerrain(pos)->GetEnvBase() to get the underlying terrain.
The g_theWorld->IsLand etc stuff looks to me to return based off of the variable m_env. If we look at Cell::CalcMovementType() it seems that sticking an improvement like a tunnel in a square will change m_env, which in turn changes what a call like IsLand will return.

Therefore I think calling g_theWorld->GetTerrain(pos)->GetEnvBase()->(whatever) instead will fix the problem.

Thus:

sint32 CityData::GetDesiredSpriteIndex(bool justTryLand)
{
	sint32 i;

	// removed DWT
	// bool isLand = justTryLand || g_theWorld->IsLand(m_pos);
	
	// Added DWT
	// We want to retreive the underlying terrain type
	// not the terrain type as modified by improvements
	// as a sea city on a tunnel will turn into a land city
	bool isLand = justTryLand || !(g_theWorld->GetTerrain(pos)->GetEnvBase()->GetMovementTypeSea() ||
				g_theWorld->GetTerrain(pos)->GetEnvBase()->GetMovementTypeShallowWater());

	const CityStyleRecord *styleRec = g_theCityStyleDB->Get(m_cityStyle);
	if(!styleRec) return -1;

	const AgeCityStyleRecord *ageStyleRec = styleRec->GetAgeStyle(g_player[m_owner]->m_age);
	if(!ageStyleRec) return -1;

	const AgeCityStyleRecord::SizeSprite *spr = NULL;
	const AgeCityStyleRecord::SizeSprite *lastTypeSpr = NULL;
	
	// GetType() below is 0 = land, 1 = ocean

	for(i = 0; i < ageStyleRec->GetNumSprites(); i++) {
		if(spr = ageStyleRec->GetSprites(i)) {
			if((isLand && spr->GetType() == 0) ||
			   (!isLand && spr->GetType() != 0)) {
				lastTypeSpr = spr;
				if(spr->GetMinSize() <= m_population &&
				   spr->GetMaxSize() >= m_population) {
					return spr->GetSprite();
				}
			}
		}
	}
	
	if(!justTryLand && !isLand) {
		
		return GetDesiredSpriteIndex(true);
	}

	if(lastTypeSpr) {
		
		return lastTypeSpr->GetSprite();
	}

	if(spr) {
		
		return spr->GetSprite();
	}
	return 0;
}

If someone who can compile the code wants to give that a spin??

[Martin Gühmann]

What I am interesting is is whether there is a flag in the city object whether the city is a land city or a sea city, I know there is such a flag in for the styles, because if you conquer a city it keeps its style.

-Martin

[NelsonAndBronte]

Does it keep its style forever? Or only until a pop is added? The code for changing ownership may simply not change the city style. So the style remains the same until the city changes again. The code I posted above seems to be how we decide whether or not the city is a land or sea city, as well as its style. I suspect that this function might not be called when you conquer a city, and so there is np change to the style.

[Martin Gühmann]

I think there shouldn't be a style change when you conquer the city in fact I like it to see that these cities keep their styles.

If we add to the city data the original land or sea data when the city is founded then we just need to change it on terraform events, from in game terraforming or teraforming by the cheat editor. This also allows to add more terrain dependent styles like for Mountain, Shallow and maybe Space.

-Martin

[Martin Gühmann]

Originally posted by NelsonAndBronte
Does it keep its style forever? Or only until a pop is added? The code for changing ownership may simply not change the city style. So the style remains the same until the city changes again. The code I posted above seems to be how we decide whether or not the city is a land or sea city, as well as its style. I suspect that this function might not be called when you conquer a city, and so there is np change to the style.

In fact it keeps it style forever. And this is also intentional there is a style variable in the CityData object. And that is also the intention of the crippled style set options in the scenario editor, but obviously when you place a city via the scenario editor the style variable is not reset.

-Martin

[NelsonAndBronte]

yeah -- i see that nothing calls CityData::SetCityStyle except the editor and the CityData class initialization. Therefore only when a new city is initialized is the city style changed. CityData::GetDesiredSpriteIndex, however, gets called by UnitActor, I think as part of the screen draw routines. So the sprite index is recalculated every time we draw the map. This may or may not be a good idea, but I still think the code above should kill the sea city bug. Hopefully i'll have the game soon so I can start to test stuff myself.

[Martin Gühmann]

Unfortunatly it doesn't kill the sea city sprite bug. First it should be m_pos instead of pos, but even with this change it doesn't compile.

Here are the signatures of the two functions you used in the code:

const Modifiers *GetEnvBase() const { return &m_EnvBase; }
bool GetMovementTypeSea() const { return (m_MovementType & k_Terrain_MovementType_Sea_Bit) != 0; }

g_theWorld->GetTerrain(m_pos)->GetEnvBase() returns a pointer to a Modifers but the GetMovementTypeSea function is a function of the TerrainRecord class.

-Martin

[NelsonAndBronte]

Thank you for giving that a try. Bleah -- I wonder if this isn't a completely futile exercise until I can properly debug my changes myself.

I added the line

	const TerrainRecord *rec = g_theTerrainDB->Get(g_theWorld->GetTerrainType(m_pos));

To create a terrain record with the terrain type of the cell the city is in. Then added

	bool isLand = justTryLand || !(rec->GetMovementTypeSea() || rec->GetMovementTypeShallowWater());

Which in turn looks up the terrain type in the terrain db and return whether or not its a water cell. This still avoids looking at the m_env varible.

Thus:

sint32 CityData::GetDesiredSpriteIndex(bool justTryLand)
{
	sint32 i;

	// removed DWT
	// bool isLand = justTryLand || g_theWorld->IsLand(m_pos);


	// Get the terrain type of the cell the city sits on

	const TerrainRecord *rec = g_theTerrainDB->Get(g_theWorld->GetTerrainType(m_pos));

	
	// Added DWT
	// We want to see if its a land cty by checking the underlying terrain type
	// not the terrain type as modified by improvements
	// as a sea city on a tunnel will turn into a land city
	
	bool isLand = justTryLand || !(rec->GetMovementTypeSea() || rec->GetMovementTypeShallowWater());
	
	
	const CityStyleRecord *styleRec = g_theCityStyleDB->Get(m_cityStyle);
	if(!styleRec) return -1;

	const AgeCityStyleRecord *ageStyleRec = styleRec->GetAgeStyle(g_player[m_owner]->m_age);
	if(!ageStyleRec) return -1;

	const AgeCityStyleRecord::SizeSprite *spr = NULL;
	const AgeCityStyleRecord::SizeSprite *lastTypeSpr = NULL;
	
	// GetType() below is 0 = land, 1 = ocean

	for(i = 0; i < ageStyleRec->GetNumSprites(); i++) {
		if(spr = ageStyleRec->GetSprites(i)) {
			if((isLand && spr->GetType() == 0) ||
			   (!isLand && spr->GetType() != 0)) {
				lastTypeSpr = spr;
				if(spr->GetMinSize() <= m_population &&
				   spr->GetMaxSize() >= m_population) {
					return spr->GetSprite();
				}
			}
		}
	}
	
	if(!justTryLand && !isLand) {
		
		return GetDesiredSpriteIndex(true);
	}

	if(lastTypeSpr) {
		
		return lastTypeSpr->GetSprite();
	}

	if(spr) {
		
		return spr->GetSprite();
	}
	return 0;
}

[Martin Gühmann]

That fixes the bug. Even if I prefer a sollution that allows more than two types of cities but for now it is enough.

-Martin

[NelsonAndBronte]

Hooray!

It will take a whole bunch of tinkering to allow for more than two city styles, I think....

Now back to the movement bonus for ships problem. I actually see one example where this is done correctly -- in ArmyData:: DeductMoveCost. However the mvoement point cost code is duplicated in many many places with some small variations, It will take some time to figure out why. The evils of code duplication certainly seem to be at work here.

[kaan]

Thank you for that fix, now i can sleep better at night

Martin, this means that our efforts to identify the source of the bug ages ago isnt all wasted

[NelsonAndBronte]

I can see no less than three different methods for handling the case of a sea unit moving over a tunnel. There is an additional, obviously broken method.
Methods 1 and 2 look as if they should work.

Method 1:
A unique method found in ArmyData:: DeductMoveCost. We check the map point and see if there is a tunnel there. If there is, and this unit cannot move on land, we use the base terrain cost.

if(m_array[i].GetMovementTypeAir()) {
	c = k_MOVE_AIR_COST;
} else if(g_theWorld->IsTunnel(pos)) {
	if(!m_array[i].GetMovementTypeLand()) {

		c = g_theWorld->GetTerrain(pos)->GetEnvBase()->GetMovement();
	} else {
		c = cost;
	}
}else if(m_array[i].Flag(k_UDF_FOUGHT_THIS_TURN)) {
	c = m_array[i].GetMovementPoints();
}else {
	c = cost;
}

Method 2: This code is repeated SIX TIMES over in TileHighlight.cpp. If at least one unit can move in the water and none can move on the land, use the base cost of the tile.

if (sel_army.GetMovementTypeAir()) { 
	cost = k_MOVE_AIR_COST; 

} else if (((sel_army.IsAtLeastOneMoveShallowWater() ||
			 sel_army.IsAtLeastOneMoveWater())) &&
		   (!sel_army.IsAtLeastOneMoveLand())) { 
	if(g_theWorld->GetTerrain(currPos)->GetEnvBase()->GetMovement()) {
		sint32 icost;
		g_theWorld->GetTerrain(currPos)->GetEnvBase()->GetMovement(icost);
		cost=icost;
	}
} else { 
	cost = g_theWorld->GetMoveCost(currPos);
}

Method 3: Found uniquely in UnitAstar::ComputeValidMovCost. We check and see if the unit has the sea or shallow water bits, and if so, use the base move cost of deep water. Not that if someone assigns a different base move cost to shallow water, this code will not return the correct value. for a shallow water tile. It also appears we always access the terrain db to get the deep water move cost, regardless of whether or not we need it.

float UnitAstar::ComputeValidMovCost(const MapPoint &pos, Cell *the_pos_cell)
{
	static const float move_cost_without_tunnel = 
		(float) g_theTerrainDB->Access(TERRAIN_WATER_DEEP)->GetEnvBase()->GetMovement();
	bool is_tunnel_and_boat = g_theWorld->IsTunnel(pos) &&

		((m_move_intersection & k_Unit_MovementType_Sea_Bit) ||
		 (m_move_intersection & k_Unit_MovementType_ShallowWater_Bit));
		 
	
	
	
	if (is_tunnel_and_boat)
		return float(min(m_army_minmax_move, move_cost_without_tunnel));
	else
		return float(min(m_army_minmax_move, the_pos_cell->GetMoveCost()));   
}

Method 4: A broken method in CellUnitList::IsMovePointsEnough. As you see, no notice is taken of tunnels.

    double cost; 
    
    if (GetMovementTypeAir()) { 
        cost = k_MOVE_AIR_COST; 
    } else { 
        cost = g_theWorld->GetMoveCost(pos); 
    }

    return IsMovePointsEnough(cost);

The same omission is in UnitData::IsMovePointsEnough:

        if (g_theUnitDB->Get(GetType())->GetMovementTypeAir() ) { 
            cost = k_MOVE_AIR_COST; 
        } else { 
            cost = g_theWorld->GetMoveCost(pos); 
        }

Thus,at minimum we should change CellUnitList::IsMovePointsEnough to:

BOOL CellUnitList::IsMovePointsEnough(const MapPoint &pos)
{

    double cost; 
    
    if (GetMovementTypeAir()) { 
        cost = k_MOVE_AIR_COST;
    } else if (g_theWorld->IsTunnel(pos)) {
		if !(GetMovementTypeLand()) {
    	    		cost = g_theWorld->GetTerrain(pos)->GetEnvBase()->GetMovement();
            	} else { 
            		cost = g_theWorld->GetMoveCost(pos);
            	}
    } else {
    	cost = g_theWorld->GetMoveCost(pos); 
    }

    return IsMovePointsEnough(cost); 
}

And UnitData::IsMovePointsEnough to:

BOOL UnitData::IsMovePointsEnough(const MapPoint &pos) const
{    
    if (Flag(k_UDF_FIRST_MOVE)) {
        return TRUE; 
    } else { 
        double cost; 

        if (g_theUnitDB->Get(GetType())->GetMovementTypeAir() ) { 
            cost = k_MOVE_AIR_COST;
        } else if (g_theWorld->IsTunnel(pos)) {
	    if !(GetMovementTypeLand()) {
    	    	cost = g_theWorld->GetTerrain(pos)->GetEnvBase()->GetMovement();
            } else { 
            	cost = g_theWorld->GetMoveCost(pos);
            }
        } else {
            	cost = g_theWorld->GetMoveCost(pos); 
        }

        return (cost <= m_movement_points ); 
    }
}

Thoughts? I'd be curious if the fix to the above two functions are enough to eliminate the problem. If so, that would be an OK short term fix. In the longer term we would need to consolidate all these methods for consistency.
If these changes don’t fix the bug, I'll have to do more digging and see if there are yet more places I've missed.

[Martin Gühmann]

Unfortunatly I don't have much time, so any takers to test this?

By the way NelsonAndBronte if you can provide us with altered source files within in a *.zip file with sved directory structure then it would be much easier for us to test it.

-Martin