patchFreeciv - Patches: patch #1341, Replace Improvement replaced_by...

 
 
Show feedback again

patch #1341: Replace Improvement replaced_by with a requirements vector

Submitted by:  pepeto <pepeto>
Submitted on:  Sat 17 Oct 2009 02:20:27 PM UTC  
 
Category: rulesetsPriority: 1 - Later
Status: DonePrivacy: Public
Assigned to: Emmet Hikory <persia>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)

Mon 30 Jun 2014 05:51:06 AM UTC, SVN revision 25333:

Use obsolete_by vector for Improvement replaced_by

See patch #1341

(Browse SVN revision 25333)

Emmet Hikory <persia>
Project MemberIn charge of this item.
Sun 29 Jun 2014 09:15:54 AM UTC, comment #7:

Ah, heh, indeed. Behaviour (2) seems so obviously correct I hadn't realised the bug was present before. Do we have a bug number for that? Does it exist/should it be fixed for 2.4.3/2.5.0?

Emmet Hikory <persia>
Project MemberIn charge of this item.
Sun 29 Jun 2014 06:15:30 AM UTC, comment #6:

So (2) has behavior changed. That's probably only good, and means that once this is in we can clean up rulesets for not protecting against the rare case of city having two Barracks at once (guards against getting double effects)

Marko Lindqvist <cazfi>
Project Administrator
Sun 29 Jun 2014 05:24:31 AM UTC, comment #5:

1) is now tested, and indeed works as expected: learning Gunpowder causes one to sell all Barracks, even if one has not built any Barracks II

2) Is also tested: attempts to force worklist inclusion of the obsolete Barracks I in a city containing Barracks II by a player ignorant of Gunpowder are ignored.

Further, I've inspected the codepath more closely: while there are guards in the AI and the client, the server enforces the restriction that only non-obsolete things may be built: while can_player_build_improvement_direct() is sometimes called without the caller checking improvement_obsolete(), this is always checked at some point in the call stack within cityturn.c between the can_player_build_improvement_direct() call and actually processing the building.

Emmet Hikory <persia>
Project MemberIn charge of this item.
Sun 29 Jun 2014 12:49:18 AM UTC, comment #4:

I'll test these conditions to verify, but from code interpretation:

1) Because, unlike other requirements vectors, obsolete_by is disjunctive, Barracks is interpreted as obsolete by improvement_obsolete(), so handled appropriately. Thinking about this, I suppose another way to consider bug #21419 would have been to change the semantics to "sustained_until" or similar, and have all the requirements negated, but if we did that, the result would end up the same.

2) All calls to building_upgrades_to() are from constructions that check can_city_build_improvement_foo(), so that one would build Barracks in that city. I suspect this is a bug, and that this behaviour should be prevented. I don't beleive the client offers the choice, or the AI tries to do it because of improvement redundancy, but that's not sufficient guard (one could construct a hacked client that would miss a client-only guard).

Emmet Hikory <persia>
Project MemberIn charge of this item.
Sat 28 Jun 2014 09:53:38 PM UTC, comment #3:

Does this cause behavior changes?

What happens in these situations:
1) You learn Gunpowder, but haven't built any Barracks II yet (all existing Barracks should be sold)
2) You conquer city with Barracks II, but don't know Gunpowder yourself, so you (try to) build Barracks

Marko Lindqvist <cazfi>
Project Administrator
Sat 28 Jun 2014 04:13:00 PM UTC, comment #2:

As it turns out, the obsolete_by vector can also replace replaced_by, with a couple helper functions for parsing (in large part thanks to the work in bug #21419).

Note that in the event a ruleset author adds multiple buildings to the obsolete_by vector, only the first of them will be considered a sensible target for automatic worklist upgrades: I'm not sure where this ought be documented (if anywhere).

(file #21187)

Emmet Hikory <persia>
Project MemberIn charge of this item.
Thu 02 Jan 2014 05:11:31 PM UTC, comment #1:

obsolete_by done by patch #3941.
replaced_by still not done.

Jacob Nevins <jtn>
Project Administrator
Sat 17 Oct 2009 02:20:27 PM UTC, original submission:

Remove the following fields from the 'struct impr_type':

  • obsolete_by
  • replaced_by

Replace them by 'requirement_vector nreqs'.

pepeto <pepeto>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

Attached Files

 

Digest:
   patch dependencies.

Digest:
   patch dependencies.

 

Carbon-Copy List
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by persia (Updated the item)
  • -unavailable- added by jtn (Updated the item)
  • -unavailable- added by dmarks (Updated the item)
  • -unavailable- added by pepeto (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 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 30 Jun 2014 06:07:28 AM UTCpersiaStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Sat 28 Jun 2014 04:13:00 PM UTCpersiaAttached File-=>Added use-obsolete_by-vector-for-improvement-replaced_by.patch, #21187
      StatusNone=>Ready For Test
      Assigned toNone=>persia
      Planned Release=>2.6.0
      SummaryImprovement negated requirements=>Replace Improvement replaced_by with a requirements vector
    Thu 02 Jan 2014 05:11:31 PM UTCjtnDependencies-=>Depends on patch #3941
    Mon 18 Jun 2012 01:15:23 AM UTCjtnPlanned Release2.4.0=>
    Wed 27 Oct 2010 01:54:13 PM UTCpepetoPlanned Release2.3.0=>2.4.0
    Sun 20 Dec 2009 09:41:11 AM UTCdmarksPlanned Release=>2.3.0
    Sat 17 Oct 2009 02:31:36 PM UTCpepetoDependencies-=>patch #1338 is dependent
    Sat 17 Oct 2009 02:20:36 PM UTCpepetoPriority5 - Normal=>1 - Later
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup