bugFreeciv - Bugs: bug #20695, Disaster nreq processing causes...

 
 
Show feedback again

bug #20695: Disaster nreq processing causes disasters mostly not to happen

Submitted by:  Emmet Hikory <persia>
Submitted on:  Mon 01 Apr 2013 03:06:35 AM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Release: 2.4.99-r22636Operating System: None
Planned Release: 2.5.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Sun 07 Apr 2013 10:33:23 PM UTC, SVN revision 22688:

Removed separate nreqs vector for disasters, and instead define related
requirements as negated in normal reqs vector.

Patch by Emmet Hikory

See bug #20695

(Browse SVN revision 22688)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 01 Apr 2013 09:10:32 AM UTC, comment #2:

Only for nreqs for effects, which I would expect to be removed in the spirit of having one correct function to do each thing that needs doing. The place it might be would be sanity_check_req_set, but that doesn't seem to be aware of the idea of negated requirements (and, further, is likely to choke on some negated requirements sets because of the limit on multiple requirements of the same type).

I believe the general activity of sorting that to be outside the scope of this bug, but it is certainly within the scope of one of my TODO items (positive disjunctive requirement specification), so I'll be sure to check for such impossible conditions with that set of patches.

Emmet Hikory <persia>
Project Member
Mon 01 Apr 2013 08:05:41 AM UTC, comment #1:

Going with nreqs removal patch.

One tangential came to mind when reading the patch: does ruleset sanity checking consider negated effect requirements? (at worst lack of this means it rejects rulesets thinking requirements to be conflicting when they are not, but also fails to spot req and same req negated to be conflicting)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 01 Apr 2013 03:06:35 AM UTC, original submission:

The current code handling nreqs for disasters causes disasters without defined nreqs not to happen. Specifically, can_disaster_happen() depends on !are_reqs_inactive(... nreqs ...). If there are nreqs defined, this happens to be the correct logic, but in the case of an empty nreq vector, are_reqs_active() never iterates, so always returns TRUE, causing can_req_happen() to return FALSE. This affects the following disasters in shipping rulesets:

alien: Earthquake, Fire, Radiation Burst, Rebels, Alien Microbes
civ1: Earthquake
civ2civ3: Nuclear Accident
classic: Earthquake, Plague, Fire, Nuclear Accident
experimental: Earthquake, Fire

This can be fixed by adding a disjunctive test to requirements.c and calling that disjunctive test from can_disaster_happen(), as illustrated in attached fix-disaster-nreqs.patch, but this is a less ideal way to do it, as we currently have two different means of expressing nreqs: one used for effects.c and the other used for everything else (aside from diassters, which doesn't work). In the everything else case, the correct representation is with the use of "negated" to negate a requirement (and thereby make it an nreq). This works because "A and not (B or C)" is equivalent to "A and not B and not C", and doesn't offer any richer syntax because "A and not (B or not C)" is equivalent to "A and C and not B".

So, remove-disaster-nreqs.patch attached, which rewrites the civ1 ruleset to use "negated", and removes all mention of and support for disaster nreqs (this requires a change to the capabilities string, as the network protocol is affected). Since disasters don't appear to exist in S2_4, I don't believe there is any need to backport the fix-disaster-nreqs solution.

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 #17640:  fix-disaster-nreqs.patch.DONTAPPLY added by persia (4kB - application/octet-stream)
file #17641:  remove-disaster-nreqs.patch added by persia (14kB - 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 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 07 Apr 2013 10:33:38 PM UTCcazfiStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Mon 01 Apr 2013 10:30:44 PM UTCcazfiPlanned Release=>2.5.0
    Mon 01 Apr 2013 08:05:41 AM UTCcazfiStatusNone=>Ready For Test
      Assigned toNone=>cazfi
    Mon 01 Apr 2013 03:06:35 AM UTCpersiaAttached File-=>Added fix-disaster-nreqs.patch.DONTAPPLY, #17640
      Attached File-=>Added remove-disaster-nreqs.patch, #17641
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup