patchFreeciv - Patches: patch #3839, Low hanging nativity fixes

 
 
Show feedback again

patch #3839: Low hanging nativity fixes

Submitted by:  Emmet Hikory <persia>
Submitted on:  Mon 08 Apr 2013 07:58:06 AM UTC  
 
Category: generalPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Planned Release: 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)

Sun 26 May 2013 08:02:32 PM UTC, SVN revision 22901:

Replaced some nativity related assumptions about unit capabilities with
correct checks.

Patch by Emmet Hikory

See patch #3839

(Browse SVN revision 22901)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 26 May 2013 08:02:25 PM UTC, SVN revision 22900:

Replaced some nativity related assumptions about unit capabilities with
correct checks.

Patch by Emmet Hikory

See patch #3839

(Browse SVN revision 22900)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 13 May 2013 10:10:51 PM UTC, comment #10:

This change may increase the cases where a player may attempt an impossible paradrop, but it also reduces the cases where a player may not attempt a possible paradrop. While it makes no difference for classical rulesets, this code means that no UMT_SEA or UMT_BOTH unit with UTYF_PARADROP may paradrop at all for SDL players, and also means that players using the SDL client may not paradrop onto oceanic transport, regardless of the ruleset definition of paradrop_to_transport. The removal allows these behaviours at the cost of a network exchange potentially reporting "Cannot paradrop" for units attempting to do things like paradrop to non-native terrain without transport. Note that this network round-trip is currently present for the GTK clients (either library version: try paradropping into ocean with the classic ruleset).

The GTK clients currently use can_unit_paradrop() as the only condition, with everything else handled as server response from do_paradrop(), which makes sense to me, but I didn't want to tear all the conditionals out of the SDL client because I don't use it, so might not notice if I broke something removing more than necessary.

Emmet Hikory <persia>
Project Member
Mon 13 May 2013 09:55:34 PM UTC, comment #9:

> For the SDL client fix, it's mostly a random swipe to remove it
> from grep results. That function should really be rewritten
> entirely to do things correctly, and if the nativity issue is a
> good reminder about that, there's no reason to patch it away.


From the name of the function to be patched this is affecting how dialog gets constructed. So, is do_paradrop() comment relevant here? It won't be called before player requests (maybe illegally) the paradrop action. Consequently, does this change mean that playher can try to paradrop even when it will not be possible?

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 04 May 2013 10:24:38 AM UTC, comment #8:

assess_danger() now in bug #20785

further reduced patch attached.

(file #17889)

Emmet Hikory <persia>
Project Member
Sat 04 May 2013 08:52:39 AM UTC, comment #7:

> - assess_danger()
> Use utype_can_take_over() to indicate danger to cities


I think this one affects even standard rulesets, and is a bug in all branches (to be fixed in S2_4, S2_5, TRUNK). AI does not consider enemy Helicopters city capture threat.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 29 Apr 2013 12:04:46 AM UTC, comment #6:

Reduced patch, no longer including base_assess_defense_unit() or kill_something_with()

(file #17850)

Emmet Hikory <persia>
Project Member
Sun 28 Apr 2013 07:36:38 PM UTC, comment #5:

base_assess_defense_unit() change now part of patch #3885

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 22 Apr 2013 09:46:15 PM UTC, comment #4:

Ah, in that case, let's assume the kill_something_with() hunk to be unsafe: there's enough other work to be done in that part of the code for complex nativity that it will surely be revisited.

Emmet Hikory <persia>
Project Member
Mon 22 Apr 2013 09:31:36 PM UTC, comment #3:

kill_something_with() is the change I'm most worried about too. You just let more units (in non-default rulesets) to go on with the main codepath, and I've seen that code to crash when unit properties unexpected to original implementation encountered before.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 21 Apr 2013 12:12:01 AM UTC, comment #2:

Heh, perhaps my definition of "lightweight" was adjusted by the remainder of the nativity stuff :) I've no objection to splitting this into bits: it was assembled from bits as I tried to clean up nativity everywhere: these were the ones that were tiny patches, showed no behaviour changes in autogame testing, and didn't trigger me to add TODO items reminding me to investigate later.

For base_assess_defense_unit(), I'm very certain of the semantics, and there are a few other places in the code that have had that change made (this one appears to have been missed).

For assess_danger_unit(), there are no precedents that I found, but the behaviour is precisely the same for rulesets without UMT_LAND units granted CanAttackNonNative (and the change seems sensible).

For assess_danger(), I'm less certain, and it may be worth performing autogame testing with rulesets specifically designed to hit this condition (presence of UMT_LAND units with !CanOccupy and UMT_SEA units with CanOccupy).

For kill_something_with(), I was pleasantly surprised not to have differences in autogame testing: I was expecting this to fall into the more complicated category. Of everything collected here, I'm least confident in this change.

For dai_rampage_want(), I'm pretty sure the current code won't work usefully even in the presence of "Big Land" from the experimental ruleset, ignoring more interesting nativity (e.g. Hut on Mountains). That said, I'm not absolutely sure my solution happens to be correct.

For the SDL client fix, it's mostly a random swipe to remove it from grep results. That function should really be rewritten entirely to do things correctly, and if the nativity issue is a good reminder about that, there's no reason to patch it away.

Emmet Hikory <persia>
Project Member
Sat 20 Apr 2013 11:32:27 PM UTC, comment #1:

> all fairly lightweight.


My gut feeling disagrees here (= I want to investigate this more carefully instead of going fast forward). Value of this patch is in pointing out places that need to be changed, but I suspect that some of the cases are much more complex than straightforward changes presented. I may even end up splitting this to several tickets to investigate some of the cases in more detail.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 08 Apr 2013 07:58:06 AM UTC, original submission:

Some more collected nativity fixes, all fairly lightweight. My testing showed no changes to autogame results with these changes. Specific changes documented in the patch, but most of them are semantically obvious (and one has precedent in prior revisions), excepting the gui-sdl change, which replaces clearly incorrect code with slightly incorrect code and a FIXME note.

From what I can tell, looking at UMT_*, is_sailing_unit() and is_ground_unit(), the rest of them require more extensive code changes, rethinking of unit handling in various contexts, or recovery of lost semantics (where something is agreed only appropriate in some circumstance, but this isn't documented, or the documentation is out of date: the most amusing being a "temporary kludge" added as part of work-in-progress in 2002). Those few I have attempted generate autogame divergence, and so I believe they are better discussed individually (or in parts, for extreme cases like aiferry.c, where there are many assumptions conflated in the code which may be best untangled with a series of patches over time).

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 #17850:  low-hanging-nativity-fixes-reduced.patch added by persia (3kB - application/octet-stream)
file #17713:  low-hanging-nativity-fixes.patch added by persia (4kB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -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
    Sun 26 May 2013 08:02:51 PM UTCcazfiStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Tue 21 May 2013 08:52:41 PM UTCcazfiPlanned Release=>2.5.0, 2.6.0
    Mon 13 May 2013 09:55:34 PM UTCcazfiStatusIn Progress=>Ready For Test
    Sat 04 May 2013 10:24:38 AM UTCpersiaAttached File-=>Added low-hanging-nativity-fixes-more-reduced.patch, #17889
    Mon 29 Apr 2013 12:04:46 AM UTCpersiaAttached File-=>Added low-hanging-nativity-fixes-reduced.patch, #17850
    Sat 20 Apr 2013 11:32:27 PM UTCcazfiStatusNone=>In Progress
      Assigned toNone=>cazfi
    Mon 08 Apr 2013 07:58:06 AM UTCpersiaAttached File-=>Added low-hanging-nativity-fixes.patch, #17713
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup