[Fixed] battle calc not accounting for limited bombards

[ubernaut]

different maps different results

[redrum]

@zlefin Correct, the battle calc doesn't consider the bombard limited by landing unit currently.

@Cernel @ubernaut So it appears "Battle is Amphibious" controls 2 things currently:

1. Whether isMarine bonus is applied to all attackers that have it set.
2. Whether any bombard units use their bombard value rather than attack value (this is why you only see a difference on maps like TWW where units have bombard values different from attack values). This really shouldn't be the case as if they are bombarding they should always use their bombard value.

[Cernel]

@redrum Wow, I wrapped my head around, didn't even guess that. That's definitely a dumb bug there. In games without marines, would you think it would be better just not to have the "Battle is Amphibious" option or needing it for adding the bombard units to the battlecalculator?

Also, in the moment that thing should have one job, I would rename it to something like "Add marine bonus" or "Add sea borne modifiers to any units". I also really dislike the use of the term "amphibious", as that, biologically, would mean something that can fully live in and outside water. For example, a duck can swim in water and go on earth, but it is not an amphibious.

[redrum]

@Cernel Yeah, quite a strange bug indeed. Probably due to all standard A&A maps having bombard equal to attack for units.

For now I've fixed that logic so bombard units always use their bombard value and added a tooltip for "Battle Is Amphibious" to say "Determines if isMarine bonus is applied for all attacking land units". PR: https://github.com/triplea-game/triplea/pull/4917

But yes for maps that don't have marines, its completely meaningless and could now just be hidden. You could take it the other direction as well where is shows/hides potential bombarding units in the attacking unit list. I'll think about this a bit but at least the bug is fixed now.

[Cernel]

@redrum Ok, minor note, as it is implied in my renaming suggestion, TripleA supports also negative marine bonuses, so "Determines if isMarine bonus is applied for all attacking land units" would be more correct as "Determines if the isMarine modifier applies for all attacking land units", but I also see the use of the term "bonus" as clearer and totally fitting to all standard cases (actually, I think only WWII Classic 3rd Edition Iron Blitz matters), so it just depends if one wants to focus on the typical case, or be more generally correct. Also, I definitely suggest not using "isMarine" in any descriptions, as that is just xml coding, that should not directly matter for the users. It should be rather communicated in actual gaming terminology, for example "the marine bonus" or "the sea borne bonus for marine units" or just generally "the sea borne modifiers" (you could have a map in which all or most land units have negative isMarine bonuses, and none is really a marine unit).

[redrum]

@Cernel Alright. I changed the parameter name to "Add amphibious attack modifiers" and the tooltip to "Applies amphibious attack modifiers to all attacking land units" as that aligns with the unit tooltip which calls it "Amphibious attack modifier" for any unit with IsMarine not 0.

[redrum]

@zlefin I've now added logic to handle "Shore Bombard Per Ground Unit Restricted" in the PR as well. So should be fixed in the pre-release once that's merged.

[Cernel]

@redrum Is that excluding units that have no transport cost?

Another minor case in which you would be allowed to battlecalculate something impossible is that you could have a battle in which there are marine units that are not using the marine bonus (cause you didn't check it) but are allowing ships to bombard. This could be addressed by excluding all marine units (like in the above example of non-sea-transportable units) from granting bombard ability, if the check is missing, but I think it would be strange if that check does that for marines, while nothing like that influences the other units (and most likely it would also confuse people, not expecting such a behaviour); so this goes basically back to the matter that you cannot define what units were offloaded and what not, that is maybe not sensible to address, as the user would rather set himself the battleships to the right number, by rules, than somewhat defining what is being offloaded (if it would be possible, in some ways).

Maybe going off topic, I want to underline that this issue, or rather the pointless ability of calculating for impossible bombard situations, is also present since the basic Classic (v1), as you can battlecalculate for battles with only air units and naval bombard, or, for custom maps, with non-transportable land units, instead. Really, all rulesets factually have a "Shore Bombard Per Ground Unit Restricted", except that, rather than Boolean, that should have been infinite for v1 and v2 (1 offloaded unit grants infinite shore bombards) and equal to 1 for v3 onwards (if this would have been done this way, you could have also had custom maps setting it to some other natural numbers).

[redrum]

@Cernel No, its checking for all land units. I'm not even aware of any maps that have mobile combat units with no transport cost.

[zlefin]

Nice work!