DEBUG: Boni of undersea tunnels for sea units bug

[NelsonAndBronte]

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:

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

If this function returns TRUE, then we want the base value for the terrain in the cell unmodified by terrain improvements:

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.... :D

[Long Axe]

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

[centrifuge]

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.... :D

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

[Immortal Wombat]

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?
:hmmm:

[NelsonAndBronte]

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?

[J Bytheway]

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 :).

[NelsonAndBronte]

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!! :q:
Ugh....

[MrBaggins]

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

[MrBaggins]

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

[NelsonAndBronte]

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" .... :)

[MrBaggins]

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

[NelsonAndBronte]

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....

[Martin Gühmann]

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

[NelsonAndBronte]

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....

[Martin Gühmann]

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

[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 :confused: 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 :D

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

[Fromafar]

I'll bite. However, it will take some time, because I first want to resynchronise with the altered source code update. Nice job, Martin :b:.

[Fromafar]

Status update after resynchronisation and changing the code: it doesn't work.

The ships are still running around swiftly in the tunnel, and there is no penalty for having to dive to reach it. Maybe we can ask someone to work on the animations :).

I am not sure about the validity of methods 2 and 4: the concept of a common movement cost for an army looks strange, because the individual units move at different rates.
When moving a land army through roads and tunnels and merging it at some location with a sea army (which may have moved before as well), you would have to consider each individual unit's move cost to see if you can get to the next location as a combined army. If the "land" cost - used because there is at least 1 land unit - is lower than the "sea" cost, and some ship has insufficient movement points for the sea cost, but sufficient ones for the land cost, it will give you an incorrect result.

But I don't think this is the cause: even when alone in an army, ships are using the fast tunnel movement rate.

[Martin Gühmann]

From a design standpoint an army has only as much movement points as much movement points the unit in that army with the least movement points has. As far as I know there is no problem with land stacks.

For sea stacks sea only armies shouldn't be allowed to use undersea tunnels, that should also be true for mixed armies, if one unit can't use the undersea tunnel than the whole armie can't use the undersea tunnel.

And I remember that this bug also exists in CTP1 and it was fixed in one of the patches, unfortunatly I don't know if it was in the hack patch or earlier.

-Martin

[Fromafar]

Originally posted by Martin Gühmann
From a design standpoint an army has only as much movement points as much movement points the unit in that army with the least movement points has.


This is a bit too pessimistic. I would not expect my combined land/air strike to fail, because since airplanes do not use roads, they make my land forces slow down considerably ;) .
As long as the individual units can get to the same spot at the same time, it should be fine to move them together.

And I also found the bug. The GetMovement() function returns a bool telling you whether the movement is possible. But some parts of the code interpret this as the actual cost, so you get movement cost 0 or 1 :eek:.

[Martin Gühmann]

Originally posted by Fromafar
This is a bit too pessimistic. I would not expect my combined land/air strike to fail, because since airplanes do not use roads, they make my land forces slow down considerably ;) .
As long as the individual units can get to the same spot at the same time, it should be fine to move them together.


Actual this is the reason why you shouldn't combine land units with aeroplanes, therefore we have airbases and aicraft cariers and that is also a problem the AI can't handle and therefore all slic scripts that deal with aircrafts try to prevent the AI from grouping land units and air units together. So that is the way it should work. If you want to move your airplanes further than the rest of the stack you have to remove them from the stack.

-Martin

[Fromafar]

I am afraid we are talking about 2 (or maybe 3) different things here.

Firstly, there is the problem that airplanes have to refuel, and land units do not. I fully agree with you there: it is unwise to keep your stack together, because it will kill the airplanes.

Secondly, there is the problem of optimal movement. When the optimal path for the land units is not straight (e.g. following roads/coastline), you may lose movement points of the airplane by keeping it with the land units. No argument there: ungrouping and recombing is better, and maybe even necessary to reach the target together at all.

Thirdly, and this is what I was talking about, there is the GUI for the human player. When the path is straight or short enough, I don't want to ungroup, move 2 or 3 different stacks to 1 tile just before the target, group, and make the final attack together. I just want to aim the whole force directly to the target.

Ideally, I would love my - competent AI :hmmm: - army generals and admirals to know how to move troops, and even synchronise the optimal paths (solve the second problem) themselves when I just point out the target on the map to them. But I may be a bit too optimistic right now.

[Martin Gühmann]

Originally posted by Fromafar
Thirdly, and this is what I was talking about, there is the GUI for the human player. When the path is straight or short enough, I don't want to ungroup, move 2 or 3 different stacks to 1 tile just before the target, group, and make the final attack together. I just want to aim the whole force directly to the target.

In that case you need to compute that would take the land units, so the aircrafts and the land units pay for that part of the way they move together. Once one units has used all its movepoints the stack can't move any further for the whole remaining turn.

So how is the path caculated in a mixed Air-Land stack, after a short test it seems that the path is calculated on the movepoints of the land units and their ability to use roads, that the aircraft can't use the road doesn't matter for the path colors, but the stack stops when the aircraft used all its movepoints.

In fact the path should be calculated on the ability of the land units to use roads, but it should be taken in consideration that the aircrafts can'T use roads, so that the green length of the path is displayed correctly.

-Martin

[Fromafar]

Basically, it now works the way you describe here. The actual movement cost deduction when executing the move is done correctly - for each unit individually in UnitData.cpp.

The path coloring in TileHighLight.cpp does it incorrectly, and may be off. But this will correct itself fairly quickly: because the actual movement is computed correctly, you may end up with a "green" army that has still movement points left at what was originally the end of the path.
Also, the fixed air and sea movement costs are always 100, and all air and sea movers have movement rates that are multiples of 100, so you will not get any problems with partial moves in the game.
I only encountered problems with Fusion Tanks in combination with tunnels in deep sea. Trying to move the army gave a red path, while ungrouping and selecting one unit at a time did give a green path for all units!

Any selected path is slavishly followed by all units as a group. There are no attempts to split off units temporarily and recombine later.