[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Update of patch #3835 (project freeciv): Status: Ready For Test = Done Open/Closed:Open = Closed ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Update of patch #3835 (project freeciv): Status:None = Ready For Test Assigned to:None = cazfi ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Update of patch #3835 (project freeciv): Planned Release: 2.3.5, 2.4.0, 2.5.0 = 2.3.5, 2.4.0, 2.5.0, 2.6.0 ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Follow-up Comment #11, patch #3835 (project freeciv): 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. ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Follow-up Comment #12, patch #3835 (project freeciv): Adding a new patch for S2_6 changing negated to present for application above patch #3879 (file #17939) ___ Additional Item Attachment: File name: negated-requirement-sanity+present.patch Size:3 KB ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Follow-up Comment #9, patch #3835 (project freeciv): 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. ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Follow-up Comment #10, patch #3835 (project freeciv): 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.) ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Update of patch #3835 (project freeciv): Planned Release: = 2.3.5, 2.4.0, 2.5.0 ___ Follow-up 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. ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Follow-up Comment #2, patch #3835 (project freeciv): 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) ___ Additional Item Attachment: File name: negated-requiement-sanity.S2_3.patch Size:3 KB File name: negated-requiement-sanity.S2_4.patch Size:3 KB ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Follow-up Comment #4, patch #3835 (project freeciv): 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). ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Follow-up Comment #5, patch #3835 (project freeciv): 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. ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Follow-up Comment #6, patch #3835 (project freeciv): 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. ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Follow-up Comment #7, patch #3835 (project freeciv): 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. ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
Follow-up Comment #8, patch #3835 (project freeciv): 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. ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [patch #3835] Negated requirements sanity checking improvements
URL: http://gna.org/patch/?3835 Summary: Negated requirements sanity checking improvements Project: Freeciv Submitted by: persia Submitted on: Fri 05 Apr 2013 08:22:55 PM JST Category: general Priority: 5 - Normal Status: None Privacy: Public Assigned to: None Originator Email: Open/Closed: Open Discussion Lock: Any Planned Release: ___ Details: 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. ___ File Attachments: --- Date: Fri 05 Apr 2013 08:22:55 PM JST Name: negated-requiement-sanity.patch Size: 4kB By: persia http://gna.org/patch/download.php?file_id=17680 ___ Reply to this item at: http://gna.org/patch/?3835 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
