patchFreeciv - Patches: patch #3835, Negated requirements sanity...

 
 
Show feedback again

patch #3835: Negated requirements sanity checking improvements

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

Thu 30 May 2013 03:41:47 PM UTC, SVN revision 22914:

Fixed requirement list sanity checking in respect to negated
requirements.

Patch by Emmet Hikory

See patch #3835

(Browse SVN revision 22914)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 30 May 2013 03:41:41 PM UTC, SVN revision 22913:

Fixed requirement list sanity checking in respect to negated
requirements.

Patch by Emmet Hikory

See patch #3835

(Browse SVN revision 22913)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 30 May 2013 03:41:30 PM UTC, SVN revision 22912:

Fixed requirement list sanity checking in respect to negated
requirements.

Patch by Emmet Hikory

See patch #3835

(Browse SVN revision 22912)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 30 May 2013 03:41:24 PM UTC, SVN revision 22911:

Fixed requirement list sanity checking in respect to negated
requirements.

Patch by Emmet Hikory

See patch #3835

(Browse SVN revision 22911)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 09 May 2013 08:34:36 AM UTC, comment #12:

Adding a new patch for S2_6 changing "negated" to "present" for application above patch #3879

(file #17939)

Emmet Hikory <persia>
Project Member
Thu 09 May 2013 07:09:22 AM UTC, comment #11:

> In the event that survives is not compared in
> are_requirements_opposites(), should it not also be removed from
> are_requirements_equal()?


Pakcet handling wants to know if data is bitwise equal, not if it's semantically equivalent.
This is actually argument for keeping survives comparison in are_requirements_opposites() as one would expect these two functions, named like that, to handle "survives" in the same way.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 10 Apr 2013 12:02:39 AM UTC, comment #10:

I'm inclined to agree that survives shouldn't matter (the code was written to compare the code structures, rather than with careful semantic analysis). That said, I can think of a couple cases where one might use this:

a) Creating a condition that applies only if some Wonder (Great or Small) has been both built and subsequently destroyed (e.g. nobody can build National Libraries until someone destroys the Library of Alexandria).

b) Creating a condition that applies only if some nation has been in the game and then subsequently been destroyed (e.g. nobody can build Janissaries until the Byzantines have been thoroughly conquered (but still have remaining citizens in cities (nationality condition)). [Note: this depends on someone addressing patch #1339 ]

I don't know if either of these are at all likely, or if there are other future semantics where opposing "survives" may be semantically meaningful. Even if preserved, I imagine this class of requirement would be much more useful for specific scenarios than for a general ruleset intended to be played with random maps and nations.

In the event that survives is not compared in are_requirements_opposites(), should it not also be removed from are_requirements_equal()? (Anyone contemplating this should investigate the callers: it may be that the packets_gen stuff complicates this and requires different semantics than the rssanity stuff.)

Emmet Hikory <persia>
Project Member
Tue 09 Apr 2013 09:46:05 PM UTC, comment #9:

Reading the patch once more, I noticed that for requirements to be considered opposites they should have same 'survives'. I'm 95% sure 'survives' shouldn't matter here. Of course, 5% is still a big uncertainty.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 08 Apr 2013 04:42:00 AM UTC, comment #8:

And when I check what precisely needs doing for S2_4 and trunk patches to ensure that !oceanic+grassland and land+!forest are both acceptable, I discover that I was indeed reading the code wrong. These work with the patches previously attached (and land+grassland fails). My confusion was that the same conditional is being used to increment local_reqs_of_type[] and to control access to the switch statement, whereas for reqs_of_type[], there are two separate conditionals. Apologies for any inconvenience (and thanks for the CodingStyle note that prevents "if (foo) bar;", as once I can remember this, I should not be so easily confused).

Note that this rejects rulesets that specify ocean+desert or land+desert, helping ruleset authors catch when they may have made a mistake, but does not catch situations like !land+!lake+!ocean+!deep_ocean or even !land+!oceanic, which I expect to handle with the 2.6 branch later.

Emmet Hikory <persia>
Project Member
Mon 08 Apr 2013 02:33:00 AM UTC, comment #7:

I haven't reviewed all of effects.c, but there's a number of calls into it in various places that aren't AI-specific (like from combat), and the few parts I inspected tend to call into requirements.c at a much lower level than the reqs processing for other things (buildings, governments, bases, etc.). In those cases where "negated" is not respected, I'll prepare patches for S2_5 (and potentially earlier).

For 2.6, I expect I will end up migrating all rulesets to contain a boolean value in the requirement specification, simply because the majority-use codepath doesn't accept nreqs as such. I can certainly start with the Alien ruleset and be sure to run autogame tests also against that, but I won't be able to do only that ruleset and still achieve the scope I hope to reach.

Emmet Hikory <persia>
Project Member
Sun 07 Apr 2013 10:50:23 PM UTC, comment #6:

About getting rid of effects nreqs:
Is there some other place totally breaking if one tries to make ruleset that works that way already, than ai evalutaion of building effects in aicity.c? That one place could be even considered a bug, and simply checking against handling negated effects as non-negated would improve that ("no handling" is the same handling as nreqs currently get there). That we could fix for 2.5 already. Second step would be switching alien ruleset to use negated reqs instead of nreqs, so it could be used in testing. Alien ruleset should make its debut in 2.6, and so should your reqs rework, so it makes sense that they are paired in development and testing.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 07 Apr 2013 07:52:01 PM UTC, comment #5:

> In practice, such redundancies will slow processing the
> requirements vector, but should not otherwise impact the game.


Yeah, the reason sanity checks against such redundancies is to help ruleset authors spot where they may have made an error - did they mean something completely different when they wrote the redundant requirement.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 07 Apr 2013 07:40:04 PM UTC, comment #4:

Unless refactored, the code can't complain about "desert"+"not oceanic", so that will remain. In practice, such redundancies will slow processing the requirements vector, but should not otherwise impact the game. I have a branch where I'm playing with requirements stuff, and hope to have something reviewable for 2.6 at some point, which should include facilities for much deeper inspection of potential conflicts or contradictions (which expensive operations would only happen at ruleset load time).

I'll update the S2_4 and trunk patches to protect against false failure when one specifies "oceanic"+"not lake" instead of "not lake"+"oceanic" (I believe the current code works with the second but not with the first, but I may have the order wrong: this is speculation from reading the code rather than a bug as a result of testing). S2_3 is unaffected by this (doesn't check local_reqs).

Emmet Hikory <persia>
Project Member
Sun 07 Apr 2013 07:12:10 PM UTC, comment #3:

> I wonder if the local_reqs_of_type[] checks in S2_4 and trunk
> should be wrapped in a conditional like the reqs_of_type[]
> checks that follow. In the absence of such a conditional, it is
> potentially possible (depending on the ordering of individual
> requirements in the requirements_vector) that one of these might
> be triggered for a negated requirement (or am I reading the code
> wrong?).


Yes, "not oceanic" + "not desert" + "not jungle" requirements make sense, as well as "oceanic" + "not lake". On the other hand in "desert" + "not oceanic", latter is redundant (but it's acceptable if code accepts that case - we're more worried about false rejections here).

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 07 Apr 2013 10:30:09 AM UTC, comment #2:

Backport patches attached. Looking through them, I wonder if the local_reqs_of_type[] checks in S2_4 and trunk should be wrapped in a conditional like the reqs_of_type[] checks that follow. In the absence of such a conditional, it is potentially possible (depending on the ordering of individual requirements in the requirements_vector) that one of these might be triggered for a negated requirement (or am I reading the code wrong?). S2_3 needs no such adjustment, as it doesn't contain that class of check.

(file #17699, file #17700)

Emmet Hikory <persia>
Project Member
Sun 07 Apr 2013 07:50:43 AM UTC, comment #1:

Can you make version(s) for stable branches too? It's rather serious bug that ruleset sanity checking is giving false negatives, rejecting rulesets that actually are legal.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 05 Apr 2013 11:22:55 AM UTC, original submission:

The current sanity checking for negated requirements isn't quite satisfactory. It is possible to specify precisely opposite requirements (You must have tea and no tea to build this road), or to fail with apparently sensible requirements (this base cannot be built on mountains or hills).

The attached patch is a first step towards fixing that, which addresses the two issues listed above. It is not an attempt to generally fix requirements checking, so it is still possible to generate conflicting requirements (e.g. "Terrain", "Grassland", "Local", FALSE; "TerrainClass", "Land", "Local", TRUE), which can never be achieved (this may only be built on grassland but never built on land). Having richer requirements checking involves having the entire requirements vector available at the time each requirement is checked, which can't happen with the current code. I am not inclined to refactor it until the repository is open for wild changes, as it would be unreasonably painful to do prior to merging the requirements processing for effects into the generalised mechanisms (which change is large and would need lots of testing).

Note that the attached patch does not attempt to address the use of negated in effects requirements: for now it is much safer to encourage the use of nreqs vectors for negated requirements for effects, as the code paths differ, and the precise implications may require significant investigation.

As part of the implementation and testing, I became rather annoyed with the semantics of requirement->negated. It feels backwards to me, both in terms of the double-negative thinking involved in the use of !preq->negated and the awkward reading of reqs lists (where the items marked "TRUE" need to not be met, and the items marked "FALSE" need to be met). If swapping the sense of this is also something that could fit into the near term, I would be very happy to prepare a patch. If not, I would like to do that as part of the larger requirements modifications mentioned above.

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 #17939:  negated-requirement-sanity+present.patch added by persia (4kB - application/octet-stream)
file #17699:  negated-requiement-sanity.S2_3.patch added by persia (3kB - application/octet-stream)
file #17700:  negated-requiement-sanity.S2_4.patch added by persia (4kB - application/octet-stream)
file #17680:  negated-requiement-sanity.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 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 30 May 2013 03:41:55 PM UTCcazfiStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Tue 21 May 2013 09:02:58 PM UTCcazfiPlanned Release2.3.5, 2.4.0, 2.5.0=>2.3.5, 2.4.0, 2.5.0, 2.6.0
    Tue 21 May 2013 09:02:02 PM UTCcazfiStatusNone=>Ready For Test
      Assigned toNone=>cazfi
    Thu 09 May 2013 08:34:36 AM UTCpersiaAttached File-=>Added negated-requirement-sanity+present.patch, #17939
    Sun 07 Apr 2013 10:30:09 AM UTCpersiaAttached File-=>Added negated-requiement-sanity.S2_3.patch, #17699
      Attached File-=>Added negated-requiement-sanity.S2_4.patch, #17700
    Sun 07 Apr 2013 07:50:43 AM UTCcazfiPlanned Release=>2.3.5, 2.4.0, 2.5.0
    Fri 05 Apr 2013 11:22:55 AM UTCpersiaAttached File-=>Added negated-requiement-sanity.patch, #17680
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup