Bug #3861

IsDepotTile is useless and do a lot of work for no result :)

Added by krinn over 7 years ago. Updated over 7 years ago.

Status:NewStart date:2012-03-23
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

I've take a look at the IsDepotTile function, i'm a bit surprise what this is suppose to do Zuu (i suppose proof of lack of sleep) ?
[code]
local old_rt = AIRoad.GetCurrentRoadType();
local rt_list = [AIRoad.ROADTYPE_ROAD, AIRoad.ROADTYPE_TRAM];
foreach(rt in rt_list) {
if(AIRoad.HasRoadType(tile, rt)) {
AIRoad.SetCurrentRoadType(rt);
if(AIRoad.IsRoadDepotTile(tile)) {
AIRoad.SetCurrentRoadType(old_rt);
return true;
}
}
}
AIRoad.SetCurrentRoadType(old_rt);
return false;
[/code]

switching to every roadtype will do nothing, at end you restore the previous type, but you assume that if you found the roadtype of the tile, then if it's a depot it will answer the question IsDepotTile
Yes, but it will just always answer the same, no need to query and switch to every roadtype, as you didn't return the info of what roadtype the depot is, just yes or no if it's a depot
I think the function should be (only speaking about road, but it's the same for rail)
- change to > IsDepotTile(tile) : return AIRoad.IsRoadDepotTile(tile); // 0 usage just need to use AIRoad.IsRoadDepotTile(tile)
change to -> IsDepotTile(tile) : if (AIRoad.IsRoadDepotTile(tile) || AIRail.IsRailDepotTile(tile) || AIWater....) return true; else return false; // this version will answer if any depot is there, could be useful for orders checks to see if a vehicle of any type is going to a depot (and it cannot fail as the order would have been refuse if trying to send an aircraft to a water depot)

I think you were wishing to test if the tile was a depot tile and in the good format, but without asking what is the "good format" the query should check, you cannot answer it : 0 usage
- change to (validate depot is of the "good format"-> IsDepotTile(tile, roadtypetocheck) and then return (AIRoad.HasRoadType(tile, roadtypetocheck) && AIRoad.IsRoadDepotTile(tile));
- or change to discover the road format : IsDepotTile(tile, vehicle_type) : if (AIRoad.HasRoadType(tile, rt) && AIRoad.IsRoadDepotTile(tile) return rt;
- another version could be IsDepotTile(tile, vehicleID) or (tile, engineID) so you can extract infos you need with AIVehicle.GetEngineType, then AIEngine.GetVehicleType and AIEngine.GetRoadType and gave a useful answer

As-is your function doesn't gave a valuable info, and worst, if the user suppose the function return true if the depot have the same roadtype as the current roadtype set, it could get a bad answer.

Also note that switch the current roadtype would be useful only if AIRoad.IsRoadDepotTile or AIRail.IsRailDepotTile would only answer true if they were the same roadtype as the current roadtype, but both function don't care, and just answer if the tile have a depot of the good vehicletype without looking at the roadtype the depot is.

All that to say, i think this function need a review (but with a coffee in hand) :D

newtile.txt Magnifier (997 Bytes) krinn, 2012-03-29 19:05

History

#1 Updated by krinn over 7 years ago

Here's a patch that should met what i think was your first needs :

It change the return value to return the roadtype or railtype need to enter/use that depot if it find one, on failure it return -1

At first, i was thinking you could have kept the true/false answering because squirrel false=0 and true!=0 but it's not doable as rt (for road or railtype) could be a zero value, and user will get false thinking there's no depot while it was getting in fact the railtype=0

So, -1 no depot
non -1 == the roadtype or railtype in use for road & rail and true (1) for airport/water

Also available in: Atom PDF