patchFreeciv - Patches: patch #3830, Change shore bombardment model to...

 
 
Show feedback again

patch #3830: Change shore bombardment model to use nativity rather than UMT_*

Submitted by:  Emmet Hikory <persia>
Submitted on:  Wed 03 Apr 2013 12:30:06 PM UTC  
 
Category: generalPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Planned Release: 2.6.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Wed 26 Mar 2014 11:02:23 PM UTC, SVN revision 24736:

Changed shore bombardment to use generic nativity instead of hardcoded sea/land
rules.

Patch by Emmet Hikory

See patch #3830

(Browse SVN revision 24736)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 20 Mar 2014 04:00:25 AM UTC, comment #11:

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.

(file #20393)

Emmet Hikory <persia>
Project Member
Mon 03 Jun 2013 07:40:22 AM UTC, comment #10:

We now consider latest version in this ticket to be correct?

Targeting to S2_5 too as this seems like bugfix to can_attack(_from)_native_tile() behavior being ruleset defined.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 08 May 2013 10:33:20 PM UTC, comment #9:

Ah, OK. That makes perfect sense. My apologies I didn't understand it at the time.

Emmet Hikory <persia>
Project Member
Wed 08 May 2013 10:23:29 PM UTC, comment #8:

> That said, re-reading comment #4, was that a suggestion


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.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 06 May 2013 12:47:31 PM UTC, comment #7:

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 (!is_native_tile(unit_type(attacker), unit_tile(defender)
&& is_native_tile(unit_type(defender), unit_tile(defender)) {
att_fp = 1;
def_fp =1;
}

?

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.

Emmet Hikory <persia>
Project Member
Sun 05 May 2013 11:18:29 PM UTC, comment #6:

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)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 19 Apr 2013 11:31:19 PM UTC, comment #5:

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:

if (!is_native_tile(unit_type(attacker), unit_tile(defender))) {
*att_fp = 1;
if (!can_exist_at_tile(unit_type(defender), unit_tile(attacker))) {
*def_fp = 1;
}
}

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.

Emmet Hikory <persia>
Project Member
Fri 19 Apr 2013 10:52:34 PM UTC, comment #4:

> 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.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 19 Apr 2013 04:31:03 AM UTC, comment #3:

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.

(file #17763)

Emmet Hikory <persia>
Project Member
Sat 13 Apr 2013 08:39:32 AM UTC, comment #2:

(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.)

Jacob Nevins <jtn>
Project Administrator
Thu 11 Apr 2013 08:48:07 PM UTC, comment #1:

Reading the patch it seems to me that it does change behavior when sea unit attacks against another sea unit in harbour.

Old code would check if defender is land unit (native to tile it defends) and as sea unit in harbour is not that, it wouldn't adjust either of att_fp or def_fp.

New code will adjust att_fp regardless of defender properties. And actually it also adjusts def_fp regardless of attacker properties - attacker might be air unit for which both tiles are native.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 03 Apr 2013 12:30:06 PM UTC, original submission:

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).

Emmet Hikory <persia>
Project Member

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #20393:  native-bombardment+restricted+rebase.patch added by persia (881B - application/octet-stream)
file #17763:  native-bombardment+restricted.patch added by persia (881B - application/octet-stream)
file #17667:  native-bombardment.patch added by persia (1kB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jtn (Posted a comment)
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by persia (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 26 Mar 2014 11:02:32 PM UTCcazfiStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Sun 23 Mar 2014 02:28:53 AM UTCcazfiAssigned toNone=>cazfi
      Planned Release2.5.0, 2.6.0=>2.6.0
    Thu 20 Mar 2014 04:00:25 AM UTCpersiaAttached File-=>Added native-bombardment+restricted+rebase.patch, #20393
    Mon 03 Jun 2013 07:40:22 AM UTCcazfiStatusNone=>Ready For Test
      Planned Release=>2.5.0, 2.6.0
    Fri 19 Apr 2013 04:31:03 AM UTCpersiaAttached File-=>Added native-bombardment+restricted.patch, #17763
    Wed 03 Apr 2013 12:30:06 PM UTCpersiaAttached File-=>Added native-bombardment.patch, #17667
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup