TradeRouteBug - City storage limit exceeded

[raubwuerger]

I think I've discovered a nasty bug within the transportation routes. One city gets too much of all goods totally ignoring the import limits and also the warehouse limit. Apparently this things are not checked when unloading goods ...

[raubwuerger]

Started analyzing the code and doing some little refactoring. Method with over 500 LoC is hard to debug ...

[devolution79]

Hi @raubwuerger , coincidentally I've also started my own effort at fixing up this tangled mess that is AI_tradeRoutes. My working branch can be found here: https://github.com/We-the-People-civ4col-mod/Mod/tree/AI_tradeRoutes_rework

The actual bug (but I am certain that there are many more) that you've encountered may possibly be in the code that force-unloads (look for UnloadMode::Force). If I remember correctly, it was added as a fix to another bug but it may very well have created another bug. I've started the work to rework this logic in my branch though.

[raubwuerger]

Hello @devolution79 , I'm sorry but I somehow overlooked your branch. I'll take a look at your changes ...

[raubwuerger]

I changed the code from

void CvSelectionGroupAI::unloadToCity(CvCity* pCity, CvUnit* unit, UnloadMode um)
{
        if (um == UnloadMode::NoForce && pCity->getMaxImportAmount(unit->getYield()) > 0)
        {
            int totalStored = unit->getYieldStored();
            int toUnload = estimateYieldsToLoad(pCity, totalStored, unit->getYield(), 0, 0);
            if(toUnload <= 0)
                return;
            if(toUnload < totalStored)
                unit->unloadStoredAmount(toUnload);
            else
                unit->unload();
        }
        else
            unit->unload();
}

to this

void CvSelectionGroupAI::unloadToCity(CvCity* pCity, CvUnit* unit, UnloadMode um)
{
    int totalStored = unit->getYieldStored();
    int toUnload = estimateYieldsToLoad(pCity, totalStored, unit->getYield(), 0, 0);
    if (toUnload <= 0)
    {
        return;
    }
    unit->unloadStoredAmount(toUnload);
}

and it works.

I don't know what UnloadMode::Force is good for! It bypasses any limits (importLimit and city warehouse limit). The method estimateYieldsToLoad() is already dealing with the import limit.

[devolution79]

I don't understand the purpose of estimateYieldsToLoad when unloading though and I think it should be removed (see my code). Also, be careful!, you need to implement a fall-back in case we weren't able to unload. Otherwise the cargo may get "stuck" in the wagon or perhaps even worse, create an endless loop (I think that use of Force was a fix for an issue like that but then we created another issue 🙄 ). I suppose that pushing a skip mission should be the least we could do (at least it will defer the problem until the next turn and then there may be a city ready for the cargo) . Failing that the entire load could be force unloaded in the export city in case of the AI. If you feel up for it we could try some form of queueing scheme.

[Nightinggale]

I don't understand the purpose of estimateYieldsToLoad when unloading though and I think it should be removed (see my code).

As I recall it was added to avoid what happened here. However the code was likely not great as it wasn't bug free. For instance it didn't consider cargo stuck on transports and I added a fallback for that, which might be what is happening here. Not sure.

I never fully got the code in question as I feel like it's a feature, which is... well let's just say it's a little bit here and a little bit there. The code organization isn't great, but admittedly it just builds on top of code, which wasn't particularly organized to begin with. We can likely come up with something better if we want to spend the time doing so.

[devolution79]

estimateYieldsToLoad only makes sense when trying to estimate the consumption rate at the source but has no meaning when actually at the destination. The problem is that we can easily solve @raubwuerger issue by only unloading what the city can accomodate (and this makes sense for humans) but then we have the problem that the surplus cargo will remain in the wagon. What is it supposed to do now ? Just wait it out perhaps (and thus waste cargoturns) ? If we choose to wait it out, we'd need some way to signal the producer that it should slow down production as well. A somewhat simple way to deal with the current issue would be to implement the following rules: 1) For the AI we can just force unload like the original logic since the AI has a cheat that allows it to sell surplus yields anyway just like the customs house or alternatively, dump it all at the export city (best port city in the code). For humans: 2) stop automation and show a message that the wagon has surplus yields that could not be delivered. I suppose that we could try to be a bit clever and first try to deliver the load to a nearby city that could accept it (but then you risk the problem of denying another wagon that would try to target the same city). It quickly gets complicated to deal with after that 🙄

[MrZorG33]

For humans: 2) stop automation and show a message that the wagon has surplus yields that could not be delivered.

bad option, because in the middle and late game spam of such messages will start) this will be very annoying.