06-11-2003 02:22									#1
	Boni of undersea tunnels for sea units bug Support Apolyton

Have a look at the CellUnitList::IsMovePointsEnough function. This function will test to see if the moving unit is an air unit and then apply the default cost for an air unit -- which therefore ignores the terrain cost of the cell. For any other unit, it uses themovement point value created by Cell::CalcTerrainMoveCost(). This function iterates through all the terrain improvements in a given cell and returns the movement cost of the most effective improvement present. Therefore, since a ship is not an airplane, it is allowed to use undersea tunnels.

It appears that we need a function analogous to GetMovementTypeAir viz:

code:

BOOL CellUnitList::GetMovementTypeWater() const 
{ 
    sint32 i; 
    for (i=0; i < m_nElements; i++) { 
        if (!m_array[i].GetMovementTypeSea() && !m_array[i].GetMovementTypeShallowWater()) {
            return FALSE; 
        }
    }
    
    return TRUE;
}  

code:

	const TerrainRecord *rec = g_theTerrainDB->Get(m_terrain_type);
	sint32 base;
	bool gotMovement = rec->GetEnvBase()->GetMovement(base);
	Assert(gotMovement);

 

Perhaps the base movement cost should just be stored along with the modified movement cost and accessed through a call to something like "World::GetBaseMoveCost"

Still no luck finding CtP2 in the bargain bins. I saw it a year ago at Mustapha Center in Singapore! Too bad I'm in Boston now....

	06-11-2003 02:34									#2
	Browse Apolyton AD-FREE

I can’t find the it either... damn

	06-11-2003 03:05									#3
	Re: Boni of undersea tunnels for sea units bug Enter the AD-FREE zone

quote: Originally posted by NelsonAndBronte Still no luck finding CtP2 in the bargain bins. I saw it a year ago at Mustapha Center in Singapore! Too bad I'm in Boston now....

check Ebay I just saw at least 10 listings for it.

	06-11-2003 03:10									#4
	Support Apolyton buy from Amazon

What would be the effects of that on a unit which can move on Land AND one or both of the water terrains, and is on land when the function is called?

	06-11-2003 03:19									#5
	Support Apolyton, buy Alpha Centauri

Oh, as in some sort of hovercraft? Thats a good point! I suppose then the solution is to state the assumption that that terrain movement boni are only applicable to units which can move on land. Thus, we actually want a CellUnitList::GetMovementTypeLand() function, and use the cell base movement cost for any unit for which this returns FALSE.

Thoughts?

	06-11-2003 04:07									#6
	Increase Your PM Length

I think that's the best solution, but it does mean that a hovercraft will effectively be able to enter and leave undersea tunnels at will, which would be a bit strange, but too hard to avoid. I guess the tunnels have ventilation shafts that they can shoot up and down .

	06-11-2003 20:45									#7
	Support Apolyton or Terrorists Win

This is more complicated than I first thought. I added a new variable "m_base_move_cost" to the Cell class. A new function CalcBaseTerrainMoveCost() sets this value; it contains the base move cost of the cell unmodified by improvements or rivers, etc. Now CalcTerrainMoveCost() accesses this variable instead of looking it up again.
The problem now is that there is a lot of redundant code in other places in the code base. The first example is the Cell Kill() function. This is called whenever we want to make a cell "dead." Unfortunately all the Kill() function does is change the terrain type. The rest of the implications, cutting terrain improvements, changing the surrounding cells terrain improvements to reflect the cut etc.. are handled in the functions which call Kill(). That means all of the different ways a cell can get killed (pollution, nukes, etc) have the same code repeated over and over again -- and therefore I have to edit each of those locations to update the base terrain movement. Similarly the test to see if we are moving an air unit or not is scattered all over the place in the pathing algorithms. The same code is repeated no less than SIX times in TileHighlight.cpp alone!!
Ugh....

	06-11-2003 20:46									#8
	Support Apolyton, buy Galactic Civilizations: Deluxe Edition

Thats one of the problems with the code. They really weren't big on having code deal with things "atomically".

	06-11-2003 20:50									#9
	Support Apolyton

Oh... and out of curiousity, NelsonAndBronte, but how did you come to decide upon that nick? Based on the historical figure and author? or?

	06-11-2003 20:54									#10
	Support Apolyton buy from Amazon

Admiral Nelson was made Duke of Bronte by the King of Naples. He always signed his name "Nelson Bronte" thereafter. I use it because one day I needed an AOL Instant messenger name right away. Since my real name was already taken, I randomly picked "NelsonAndBronte" ....

	06-11-2003 20:55									#11
	Lose 30 kilos (of popups)

okies. You learn something new (the whole King of Naples, and Nelson thingie) everyday.

	06-11-2003 20:57									#12
	Support Apolyton, pre-order Civilization IV

FWIW the author's name was originally "Brunty." Her father (i think) changed it to Bronte to take advantage of the association with Nelson and disguise it's Irish origins....

	06-11-2003 22:03									#13
	Help yourself to an AD-FREE life

Maybe it could also fix the sea city sprite bug, at least undersea tunnels and the sea city sprite bug are somehow related.

For the same code at other places: It has definatly to be removed. And replaced by one function or put into one function.

-Martin

	06-11-2003 22:52									#14
	Support Apolyton, buy Civilization 2

Hi Martin,

I don't think that the undersea tunnel thing and city sprite problem are connected. In the case of ships getting the tunnel movement bonus, its because the function which returns movement point cost nowhere checks for the condition of whether or not the moving unit is a ship. It only checks to see if the unit is a plane, in which case it gets a fixed movement point cost. Everything else gets the 'improved' movement point cost.
I had a quick look at the sea sprite thing. AFAICS it was only looking at city styles to determine what spite to place. I didn't really see any code that checked for an ocean city and changed the retreived sprite. I didn;t look too hard either, so I could have missed it.

I've founds tons of redundant code. Oh well....

	06-11-2003 23:08									#15
	Help yourself to an AD-FREE life

Yes it should only look on the city styles but if you place in the default game a city on an ocean tile of course with the cheat editor in a fresh game the right sprite is shown. If you lay a undersea tunnel under the city and you add via the cheat editor one pop the city turns into a land city. If you have the advance that gives you undersea tunnels then a undersea tunnel is created under the city automaticly when it is build. And it changes its style immidiatly. So I did a test I removed the movement bonuns of undersea tunnels and the cities look as they are supposed to look. And if you take a look on the minimap and you disabled showing the terrain types, the tiles with undersea tunnels are shown as land tiles. So there must be something else that checks if a city is on a land tile by evaluating the movement flags.

-Martin

	07-11-2003 01:19									#16
	Avatar Enlargement: We've got the solution

Hi Martin,

Have a look at UnitActor::GetIDAndType.

code:

	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;
		}

	07-11-2003 19:54									#17
	Full PM-box? Change here!

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

	07-11-2003 20:25									#18
	Avatar Enlargement: We've got the solution

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:

code:

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;
}

Last edited by NelsonAndBronte on 07-11-2003 at 20:36

	07-11-2003 20:33									#19
	Support Apolyton, buy Civilization 2

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

	07-11-2003 20:41									#20
	Support Apolyton, pre-order Civilization IV

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.

	07-11-2003 20:48									#21
	Support Apolyton buy from Amazon

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

	07-11-2003 20:51									#22
	Tired of ads?

quote: 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

	07-11-2003 21:04									#23
	Avatar Enlargement: We've got the solution

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.

	07-11-2003 21:19									#24
	Support Apolyton buy from Amazon

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

	07-11-2003 22:04									#25
	Suffering from ads?

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

code:

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

code:

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

code:

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;
}

	07-11-2003 22:21									#26
	Inflate your Upload Space

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

	07-11-2003 22:55									#27
	Support Apolyton, pre-order Civilization IV

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.

	07-11-2003 23:29									#28
	Support Apolyton, buy Galactic Civilizations

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

	10-11-2003 20:44									#29
	Full PM-box? Change here!

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.

code:


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;
}

code:

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);
}

code:


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()));   
}

code:


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

    return IsMovePointsEnough(cost); 

code:

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

code:


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); 
}

code:

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 ); 
    }
}

	22-11-2003 04:56									#30
	Enter the AD-FREE zone

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