Possibility for workshops to produce at a loss

[tizubythefizo]

It's (theoretically) possible for workshops to produce at a loss due to a check to make sure input costs are less than output price not taking into account a later cap on output price.

Class: TaleWorlds.CampaignSystem.Sandbox.CampaignBehaviors.WorkshopsCampaignBehavior

Price check method is the first half of WorkshopsCampaignBehavior.DoProduction()

for (int i = 0; i < production.Outputs.Count; i++)
{
	int item = production.Outputs[i].Item2;
	num += item;
	ItemModifier itemModifier = null;
	ItemObject randomItem = GetRandomItem(production.Outputs[i].Item1, town);
	if (randomItem != null)
	{
		town.GetItemPrice(new ItemRosterElement(randomItem, 1, itemModifier), null, isSelling: true);
		list.Add((randomItem, item));
		num2 += town.GetItemPrice(randomItem, null, isSelling: true) * item;
	}
}
if ((Campaign.Current.GameStarted && num2 <= inputMaterialCost) || num2 > town.Gold || inputMaterialCost > workshop.Capital)
{
	return false;
}

The actual cap is in WorkshopsCampaignBehavior.ProduceOutput

if (Campaign.Current.GameStarted && !doNotEffectCapital)
{
	int num = Math.Min(1000, itemPrice) * count;
	workshop.ChangeGold(num);
	town.ChangeGold(-num);
}

The initial check for profitability doesn't take the Math.Min(100, itemPrice) line into account.

So if the price per unit of an output is > 1000 and the total inputs are more than that cap * count the shop can still produce, but at a loss.

[Skau]

Would adding a check like if num <= town.Gold be enough?

[tizubythefizo]

No, that's not the issue. The check for town gold is already there and works as expected.

I think the line "num2 += town.GetItemPrice(randomItem, null, isSelling: true) * item;" would need to be changed to factor in the cap.

Something like

num2 += Math.Min(1000, town.GetItemPrice(randomItem, null, isSelling: true) * item;

or something along those lines.

But it's fragile since if the cap is latter changed, the fix code would need to be quickly updated to match it. They should have separated that logic into a method, but alas that didn't happen (well, at least as far as the decompiled code is concerned).

[Skau]

Right. If I understand correctly changing the hardcoded 1000 to something more dynamic for future-proofing would be great too. Like town.Gold or something.

[tizubythefizo]

Well, they way I'd have written the class would be to have something like

private int DetermineOutputPrice (int item, int count)
{
    return Math.Min(1000, town.GetItemPrice(item, null, isSelling:true) * count;
}

as opposed to something like

private int outputCap = 1000;

Better to encapsulate the actual logic, because that may change, than repeat the Math.Min function because the logic to determine what the cap on an output is itself could change (like it may not always use Math.Min after many refactorings).

But fundamentally the issue isn't the hardcoded 1000 that is the problem, it's the fact that the first check doesn't account for the cap at all.

They forgot to include that cap limit in the first check entirely, so it can have a different result than when the money amount is determined down in ProduceOutput when they should end up being the same.

[Tyler-IN]

@tizubythefizo have you considered making a PR?