patchFreeciv - Patches: patch #3856, Add nativity check for...

 
 
Show feedback again

patch #3856: Add nativity check for find_closest_city()

Submitted by:  Emmet Hikory <persia>
Submitted on:  Fri 19 Apr 2013 11:55:07 PM UTC  
 
Category: generalPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Planned Release: 2.4.0, 2.5.0, 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)

Sat 20 Jul 2013 03:23:21 PM UTC, SVN revision 23086:

Saving undisbandable units is no longer hardcoded to require a coastal city.

Second part of gna patch #3856.

(Browse SVN revision 23086)

Jacob Nevins <jtn>
Project Administrator
Sat 20 Jul 2013 03:15:13 PM UTC, SVN revision 23083:

Saving undisbandable units is no longer hardcoded to require a coastal city.

Second part of gna patch #3856.

(Browse SVN revision 23083)

Jacob Nevins <jtn>
Project Administrator
Sat 20 Jul 2013 03:02:45 PM UTC, SVN revision 23080:

Saving undisbandable units is no longer hardcoded to require a coastal city.

Second part of gna patch #3856.

(Browse SVN revision 23080)

Jacob Nevins <jtn>
Project Administrator
Tue 16 Jul 2013 11:05:02 PM UTC, comment #9:

Experimentally, "UTYF_UNDISBANDABLE units are killed outright for maps without coastal cities" is still true for the standard ruleset regardless of whether I fix try_to_save_unit().

Which shouldn't have surprised me; the new requirement to save Undisbandable is to find a city native to the transport (not the cargo), so if your king was on a boat and you happened to have no coastal cities left, well, sucks to have been you.

Still, I can't see that the behaviour noted in comment #8 can be deliberate, so I'll submit a patch.

Jacob Nevins <jtn>
Project Administrator
Sat 06 Jul 2013 02:27:03 PM UTC, comment #8:

> 3) When teleporting a UTYF_UNDISBANDABLE unit in
> try_to_save_unit(), teleport to a city native to the transport,
> rather than always a coastal city. This only affects rulesets
> with non-UMT_SEA transports that can carry UTYF_UNDISBANDABLE
> units, so results were not shown in any of my autogame testing
> (no differences, but not actually exercised meaningfully).

Doesn't the fixed version now insist on a city that's native to the transport and coastal (since only_ocean is still TRUE)?
If so, "UTYF_UNDISBANDABLE units are killed outright for maps without coastal cities" is still true, I think (I haven't tested it).
(Also, what of the nativity of the undisbandable unit? -- looking at S2_4, I don't see anything stopping a unit with nativity requirements being teleported to a city where it will be "landlocked". That's probably another ticket, though, and also quite an obscure case.)

Jacob Nevins <jtn>
Project Administrator
Mon 20 May 2013 07:52:38 AM UTC, SVN revision 22878:

Make proper nativity checks in find_closest_city() instead of simple
move type and terrain class based ones.

Patch by Emmet Hikory

See patch #3856

(Browse SVN revision 22878)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 20 May 2013 07:52:29 AM UTC, SVN revision 22877:

Make proper nativity checks in find_closest_city() instead of simple
move type and terrain class based ones.

Patch by Emmet Hikory

See patch #3856

(Browse SVN revision 22877)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 20 May 2013 07:52:23 AM UTC, SVN revision 22876:

Make proper nativity checks in find_closest_city() instead of simple
move type and terrain class based ones.

Patch by Emmet Hikory

See patch #3856

(Browse SVN revision 22876)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 06 May 2013 09:44:42 AM UTC, comment #4:

Backport patches for S2_4, S2_5 attached. From manual inspection they appear to be only offset changes, but I had some rejects trying to apply the originally applied patch to these branches (it still applies to trunk with offsets). Also attached is the (tiny) patch for S2_3 that corrects the value provided for only_ocean. As previously discussed, I don't think it worth applying this: while it may improve the AI, the total implications deserve playtesting, and an elder stable branch is not the place for that (so I shan't be opening a separate ticket for it, nor do I anticipate application).

(file #17899, file #17900, file #17901)

Emmet Hikory <persia>
Project Member
Sun 05 May 2013 10:57:32 PM UTC, comment #3:

I should have make it more clear what I meant: I didn't want part of this patch, but just a bugfix that would correct the reversed check in S2_4 so that at least classic nativity would work correctly there.
But thinking it more, you are right that we should fix all this properly for generic nativity in S2_4 too.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 04 May 2013 09:52:53 AM UTC, comment #2:

Yes, the AI is looking for cities to defend with the reversed nativity check, and yes, it just makes the AI suboptimal (doesn't try as hard to defend non-coastal cities, oceanic-native units don't end up being used to defend if their assigned city goes away for some reason (assuming that they happened to be selected as defenders anyway, which is fairly unlikely (and even moreso since revision 22826))). There are indeed no catastrophic consequences, and no point backporting to 2_3.

I could open a separate ticket for only this issue, but I'm unsure which of the other changes should be disincluded for that. 2_4 has the more complex transport stuff, so I would think the change in wipe_unit() ought also be included (consider that UTYF_UNDISBANDABLE units are killed outright for maps without coastal cities when attempting to teleport with the current code: this may be an entirely different bug). The editor thing is mostly just cleanup, and doesn't need to be in 2_4, but if someone wanted to use any ruleset with interesting nativity, there is a fair chance of unexpected failure to assign homecities for editor-created units.

Of course, if there are changelog- or project-coordination- reasons to have multiple commits, I don't mind separating them, but one of them will have the entire implementation (and change every line in the previously attached patch), and the others will be one-line changes for the precise arguments to the extended function.

Emmet Hikory <persia>
Project Member
Sat 04 May 2013 08:37:01 AM UTC, comment #1:

Isn't AI looking cities to defend with reversed nativity check bug in all branches? Could you mkae separate bugfix ticket of it? (As this has gone unnoticed so far, I assume that the bug just makes AI less optimal and there's no catastrophic consequences of inland city getting selected for sea unit, so we'll probably not push fix to S2_3 any more)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 19 Apr 2013 11:55:07 PM UTC, original submission:

The find_closest_city() function takes an is_ocean parameter that is used for two different purposes: to identify nativity given certain assumptions, and to find coastal/border cities. The attached patch extends this function to take a unit_class argument used for nativity identification, and replaces the use of is_ocean in three places:

1) When the AI has a defending unit with nothing to do (e.g. city disbanded), that unit currently looks for a new city with an apparently reversed nativity check: UMT_LAND units head for coastal cities, and other units (sea/air) look for the closest city (which may not be reachable for sea units). This patch causes bored defenders to seek a native city. Autogame testing showed some differences with this change, although I was unable to idenfity specific generalisation of behaviour: my assumption is that sea units are more likely to rest in cities (where they are potentially defended by land units), and that land units might choose to fortify in other land cities (potentially closer to land borders).

2) When creating a new unit with the editor, assign a homecity that is native to the class being created, rather than only doing this for UMT_SEA units.

3) When teleporting a UTYF_UNDISBANDABLE unit in try_to_save_unit(), teleport to a city native to the transport, rather than always a coastal city. This only affects rulesets with non-UMT_SEA transports that can carry UTYF_UNDISBANDABLE units, so results were not shown in any of my autogame testing (no differences, but not actually exercised meaningfully).

This patch does not attempt to remove the is_ocean argument entirely. This remains useful for dai_manage_diplomat(), where the intent is to send bored diplomats to coastal cities to help defend the edges of the continent in cases where there is no access to other civilisations by land. Once AI diplomats learn how to use transport, this argument may be safely removed. Alternately, this could usefully be adjusted to indicate border/coastal cities rather than is_ocean: the current model behaves suboptimally when cities are adjacent to small lakes (also not addressed by this patch).

Note that this patch expects prior application of the wipe_unit() refactoring from patch #3804 (and bug# 20744).

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 #18306:  trunk-S2_5-S2_4-undisbandable-drown-ocean-restriction.patch added by jtn (1kB - text/x-patch - trunk/S2_5/S2_4 r23056: fix condition for saving drowning Undisbandable units)
file #17899:  native-find-closest-city.S2_4.patch added by persia (9kB - application/octet-stream)
file #17900:  restrict-target-cities-for-umt-sea.S2_3.patch added by persia (860B - application/octet-stream)
file #17901:  native-find-closest-city.S2_5.patch added by persia (9kB - application/octet-stream)
file #17777:  native-find-closest-city.patch added by persia (9kB - 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 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 16 Jul 2013 11:41:35 PM UTCjtnAttached File-=>Added trunk-S2_5-S2_4-undisbandable-drown-ocean-restriction.patch, #18306
    Mon 20 May 2013 07:52:48 AM UTCcazfiStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Mon 13 May 2013 09:48:20 PM UTCcazfiStatusNone=>Ready For Test
      Assigned toNone=>cazfi
      Planned Release=>2.4.0, 2.5.0, 2.6.0
    Mon 06 May 2013 09:44:42 AM UTCpersiaAttached File-=>Added native-find-closest-city.S2_4.patch, #17899
      Attached File-=>Added restrict-target-cities-for-umt-sea.S2_3.patch, #17900
      Attached File-=>Added native-find-closest-city.S2_5.patch, #17901
    Fri 19 Apr 2013 11:55:07 PM UTCpersiaAttached File-=>Added native-find-closest-city.patch, #17777
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup