Add leather addon

[ottml]

Fixes #1711

[Flamefire]

Thanks for working on this! I added some inline notes

[ottml]

From my side everything is finished so far. We can begin reviewing the changes.

[ottml]

@Flamefire If you have time it would be nice to get a review :)

[ottml]

@Flamefire Do you have time for a review? It would be nice if we can bring the pull request to master

[Flamefire]

Sorry for the delay this PR is huge (it even challenges the GitHub UI) and it took quite a bit of time to review and verify that the logic etc. is sound.

But it looks very good already and I only have some minor remarks for simplicity/easy of understanding like renaming.
So thank you very much!

I know many of the issues I marked are already present in the code this is based on but my intention is to not introduce more of what I think was or turned out not optimal/inconsistent with other parts of the codebase.

I kept the comments short to save time, please ask if anything is unclear or where you wouldn't agree or have reasons I might have missed.

2 differences to the doc I've seen:

While a soldier is armored they also have higher defence (equal to halfway towards the next rank).

@aztimh This was meant as a higher chance to not get hit in the fight in addition to the extra HP, wasn't it?
That would need to be implemented in noFighting, I'd skip a test for that as it would be to much effort for the gain.
Or could that be OP for "just" a pig/skin and 2(.5) processing steps? I'd slightly prefer implementing it as it might be expensive enough but let you have the final call

Armor also gives soldiers a 30% chance to resist a single hit from a catapult increasing resilience against siege bombardment. So 70% of the time they die as normal; 30% of the time they just lose their armor instead. It only works if a military building is fully armored as the weakest soldier is the one hit by catapult.

Together with:

Each armored rank is considered stronger than the unarmored of the same rank but not as strong as unarmored of the next rank up.

@aztimh I think the current implementation where the weakest soldier gets checked is fine, so a lower-rank soldier with armor looses its armor even if there is a higher rank soldier without armor. This contradicts the "only when fully armored" but I guess this is what you intended, isn't it?

Thanks again and I hope you don't mind me being "picky"

[aztimh]

2 differences to the doc I've seen:

While a soldier is armored they also have higher defence (equal to halfway towards the next rank).

@aztimh This was meant as a higher chance to not get hit in the fight in addition to the extra HP, wasn't it? That would need to be implemented in noFighting, I'd skip a test for that as it would be to much effort for the gain. Or could that be OP for "just" a pig/skin and 2(.5) processing steps? I'd slightly prefer implementing it as it might be expensive enough but let you have the final call

Armor also gives soldiers a 30% chance to resist a single hit from a catapult increasing resilience against siege bombardment. So 70% of the time they die as normal; 30% of the time they just lose their armor instead. It only works if a military building is fully armored as the weakest soldier is the one hit by catapult.

Together with:

Each armored rank is considered stronger than the unarmored of the same rank but not as strong as unarmored of the next rank up.

@aztimh I think the current implementation where the weakest soldier gets checked is fine, so a lower-rank soldier with armor looses its armor even if there is a higher rank soldier without armor. This contradicts the "only when fully armored" but I guess this is what you intended, isn't it?

Thanks again and I hope you don't mind me being "picky"

I think the only reason I was considering the extra dodge chance (defence boost) was in my testing sometimes in duels armor felt pretty underwhelming. The idea is that they have the small boost of defence towards the next rank but only on the first hit, once the armor is lost they revert to their usual stats for their rank until more armor is equipped. That way it is only a slight boosted chance of armor lasting a little longer before that first hit.

Yeah that catapult thing you said sounds good. We could change the Leatherworker building ? (tooltip) thing to:

"The leatherworker crafts armor from finely tanned leather. The armor is then transported to military buildings to bolster your soldiers' defense.

An armored soldier can withstand an additional hit in battle and sometimes survives a catapult strike, but the armor is lost once struck."

I think the reason we had it as when a building is fully armored is coz the armor goes from right to left like coin promotions but catapults apparently hit from left to right (weakest to strongest), but I think this tooltip still covers that without being overly wordy anyway.

No problems being picky, I'd prefer it to be as good as it can be and if the balance is off we can always change it later. The basic premise is that it is a small boost to military strength/defense and a bit of a counter to catapults which gives competative games the option for different (or no) limitations on catapults. It comes at the cost of wood (limits early spam due to needing it mostly for construction) and potentially food production but you can also get the headstart with hunter and skinner working together for both but that requires keeping forests for game production (still good for skins production in the early game).

The other balance is that it requires building an iron smelter and a metalworks to make tongs for the leatherworker as we have left the defaults at 0 for Skinner (Cleaver), Tanner (Saw), Leatherworker (Tongs). So I think it can be okay to have a little bit more defense as it is a pretty hefty investment (or at least requires an earlier metalworks).

[ottml]

I started changing the easy parts first to get down the number of open conversations. It would be nice if you (@Flamefire) can have a look at the applied changes and resolve this conversations if you are fine with the changes.

[Flamefire]

I started changing the easy parts first to get down the number of open conversations. It would be nice if you (@Flamefire) can have a look at the applied changes and resolve this conversations if you are fine with the changes.

Ok done. If you apply a suggested diff (best via add-to-batch and apply in 1 commit) on GitHub it will auto-resolve the discussion which is fine if it doesn't require additional changes and no further comment or review. Saves some time, but if in doubt do as you did before so we can take a last look.
Thanks again, very productive so far!

[Flamefire]

Seeing the conflicts: If you want some help resolving those tell me. I caused them (sorry for that) and could e.g. rebase your branch if you want.

[Flamefire]

The rebase caused trouble for GitHub to match the existing comments to code lines in the diff view and makes it harder to tell which changes are done after the last review. Please do those only at the very end. If you want/need the current master branch you can merge it until then.

That being said: Except for the operator change and adding an assert we are done here, so if you want to do a final rebase and combine some commits, e.g. adjacent commits with the same commit title and no further message where that makes sense, feel free.
Finally please confirm that/once you made some quick gameplay testing (just check the production, upgrade and fight works, e.g. in "Tür an Tür" or similar small map) and
If that works as you intended I'll merge as soon as CI passes 🎉