Sorry for the long delay: Yes, the most recently provided patch is "correct", in that it preserves the current behaviour and doesn't use the old nativity features. I prefer the logic expressed in comment #5, but wanted discussion about it in case others had other opinions.

I've attached a rebase against r24699 for ease of application if there isn't more discussion.

It wasn't a suggestion (for what we want to have in future) but explanation of what I think old code was trying to do. And as I said, in the past air units were not concern as they had hardcoded Unreachable property against everything but Fighters. The defender check then was just to make sure it's not ship in harbour.

Not so far as I can tell: the function was originally put in combat.c in largely the current form (using !is_ocean_tile() is new, but the old implementation was essentially the same check) in SVN revision 3369 (at which point HELI_MOVING and is_heli_unit() were still present in common/unit.c) applying specifically to sailing units attacking ground units: everyone else keeps their firepower. If a mistake was made in the conflation of AIR_MOVING and HELI_MOVING, it doesn't appear to have happened for this specific conditional ("land bombardment").

The logic described in my last comment would penalise sailing units and not penalise helicopters for units with classical nativity, rather than not penalising either (as in the implementation as of SVN revision 3369 or the most recently attached patch).

The argument for changing the logic is that the Battleship cannot enter the tile in which the Helicopter resides (so should be penalised), but the Helicopter is under no such restriction (so should not be penalised). The argument for not changing the logic is that as long as the two units are both native to either the attacking or defending square, they will meet there to fight, and do so without penalty. I still prefer the first argument, but I've also still not reached absolute confidence. The original logic may also have included assumptions about the AttackNonNative status of the defender: perhaps the idea is that the defender can't respond because there is no expectation of a means to attack the attacker: in such a case, perhaps the nested conditional should be about whether the defender can attack the attacker, rather than just whether it could exist on the attacker's tile.

That said, re-reading comment #4, was that a suggestion that the condition be:

If so, I don't like this at all because of the disadvantages this causes for air units as compared to the is_ground_unit() test. Testing that the defender is non-native to attacking tile gives a discriminator that happens to have the same result for classical nativity and can perhaps be explained in terms of unit capabilities, although I don't have any objection to a test that checks to ensure the defender is both non-native to the attacking tile and native to the defending tile, if that seems like it might have a better explanation.

it might be worth checking if this worked differently when "HELI_MOVING" was separate from "AIR_MOVING" (before they get combined as "BOTH_MOVING") I now think it's quite likely that I made some mistake regarding helicopter handling at that point (as Air units have always been Unreachable, this implementation probably has been written without thinking them at all)

I understand why an attacking unit might get reduced firepower when attacking a non-native tile. I don't understand why this reduction fails to apply depending on the defending unit: I would expect it to apply in all cases, as it ought indicate remote bombardment. Is the intent to allow the greater firepower when the defender is weakened by being non-native?

Separately, I understand why a defending unit might get reduced firepower when defending against an attack from a non-native tile if the attacking unit is not native to the defending tile, as this also indicates remote bombardment. I'm not sure that a unit native to both tiles should be so penalised, as it can defend directly.

For identity with past behaviour, the most recently posted patch is correct. Given more thought, I think attacker firepower should be reduced unconditionally if the defending tile is non-native, as the attacker cannot engage in typical tactics to overtake the tile, and that defender firepower should be reduced if the attacker is non-native to the attacked tile AND the defender is non-native to the attacking tile. Like so:

This behaves the same as the original code for air->land, air->sea, air->air, sea->land, sea->sea, land->air, and land->land. This behaves differently in the sea->air and land->sea cases. For sea->air (e.g. Battleship attacking Helicopter), the firepower of the attacker is reduced, but the firepower of the defender is not. For land->sea (e.g. AttackNonNative Howitzer vs. Battleship), the firepower of both units is reduced.

Note that the most recently posted patch matches the land->sea behaviour described immediately above, but does not match the sea->air behaviour, instead not reducing the firepower of either unit. As much as I currently think the logic in this post is correct, I've had that feeling about other logic previously, so would appreciate more discussion before posting another full patch.

> That said, I'm not sure I understand the specific meaning of
> this check in a nativity-based environment if it doesn't mean
> that units have a hard time attacking/defending when they are
> not native to the target tile.

Took me a while too since I followed logic of your patch. It makes no sense that defender (land unit) must be non-native to attacker tile (sea). But it makes sense that defender (land) unit must be native to its own tile (land).

Original logic: It's shore bombardment only if sea unit attacks against land unit on land tile.

Updated patch attached restoring original behaviour. The sea unit in harbor case gets adjusted anyway because of BadCityDefender (but, yes, this shouldn't be conflated). The ground unit defending against air unit case made fp=2 ground units weaker at anti-air when attacked from sea, which convinces me not to argue the situation (as it doesn't make sense to have different anti-air capabilities depending on the position of the air unit). That said, I'm not sure I understand the specific meaning of this check in a nativity-based environment if it doesn't mean that units have a hard time attacking/defending when they are not native to the target tile.

(Since I wasn't sure: this is referring to normal attack that the comments call "bombardment", not the technical meaning of "bombard" that UTYF_BOMBARDER units do. That has its own hardcoded restrictions on terrain type, but fixing that should be the subject of a separate ticket.)

This patch changes the shore bombardment to use the nativity model. I was unsure if it should also check UCF_CAN_OCCUPY_CITY, but decided not to check that so that if someone allowed Howitzers to engage in shore-to-sea bombardment in the ruleset, they would be similarly handicapped. I believe this patch will make no difference in play with any of the supplied rulesets (only "Sea" units ever have UCF_ATTACK_NON_NATIVE set).

Copyright (C) 2004-2006, the Gna! people. Posted items are owned by whoever posted them.
Verbatim copying and distribution of this entire article is permitted in any medium, provided this notice is preserved.