bugFreeciv - Bugs: bug #21992, Useless but non-obsolete city...

 
 
Show feedback again

bug #21992: Useless but non-obsolete city improvements not redundant with present==FALSE effects requirements

Submitted by:  Emmet Hikory <persia>
Submitted on:  Wed 30 Apr 2014 04:11:19 AM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Jacob Nevins <jtn>Open/Closed: Closed
Release: Operating System: Any
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)

Wed 04 Jun 2014 12:45:04 PM UTC, comment #7:

Sorry to catch this so late: the attached patch doesn't solve the problem I intended to report, although it does improve things to some degree with the existing rulesets (so there's no point reverting it).

More specifically, it is possible to use PRESENT=FALSE as a means to allow semi-disjunction: given a set of values in a range, using FALSE for all the unacceptable values (making any acceptable values inherently true), so that one may have more than one acceptable value. This is semantically distinct from the use to indicate that something is obsolete or redundant.

The right solution (which belongs in another patch) is to evaluate the effect for the improvement, and determine if another improvement provides that effect in such a way that this improvement isn't useful.

To put this another way, we shouldn't impose semantic meaning on the notation n the ruleset (present=FALSE), but rather analyze the improvement to determine if it is redundant.

An example of how the current patch might go wrong: imagine an improvement that provides +1 happiness if there are less than 4 units in the city: with this patch, that would be marked redundant for overpopulated cities (and sold with "Sell All Redundant", whereas an nreqs-based implementation would use present=FALSE to indicate the effect shouldn't work, so that the nreqs weren't triggered (as the improvement becomes useful again as soon as there are fewer units in the city).

Emmet Hikory <persia>
Project Member
Mon 02 Jun 2014 10:33:25 PM UTC, SVN revision 25032:

Logic that checks whether an effect is prevented (in AI and UI) now
checks present=FALSE requirements as well as nreqs.

Reported by Emmet Hikory (persia@gna).

See gna bug #21992.

(Browse SVN revision 25032)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 02 Jun 2014 10:30:50 PM UTC, comment #5:

Oh! Of course, I forgot to think about the effects of moving to reqs outside my change. Thanks.

My other test indicates that I've caused no obvious regressions, and review time's up, so in it goes.


> What about making "present" tri-state value, with one value
> being equivalent to current TRUE, and the other two being like
> current FALSE but one indicating "replacement" and the other
> "prevention"?

Ah, yes, that's a better idea than keeping nreqs, if we need it. Any more thoughts in that direction should go to a new ticket.

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 02 Jun 2014 10:11:07 PM UTC, comment #4:

AI is handling nreqs and present=FALSE differently when evaluating effects provided by building to build.

Evaluation of present=FALSE should be improvement over nreqs. nreqs are not evaluated at all, as they are never included to the cache. present=FALSE effects are evaluated, and can provide either positive or negative value for the building.

Your case sounds like it used to ignore the fact that normal City Walls make Great Wall redundant, but with present=FALSE it notices that Great Wall would not provide extra value.

Marko Lindqvist <cazfi>
Project Administrator
Mon 02 Jun 2014 09:58:05 PM UTC, comment #3:

> (I suspect this isn't amenable to autogame testing, since the
> AI is expected to improve. Probably the correct test is against
> unmodified code and nreqs-based ruleset.)

Tested unmodified code with patch #4411 reverted (i.e. with nreqs) against patched code with current classic ruleset (with present=FALSE). Diverged at turn 36 when AI decided to build Great Library instead of Great Wall :(
Don't think this should happen, so holding patch back for now.

Fixed code with nreqs ruleset behaved the same as unfixed code with nreqs, interestingly.
Might run a couple of repeat autogames to rule out unrelated nondeterminism.

(I double-checked patch #4411 itself, and didn't spot any semantic changes.)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Mon 02 Jun 2014 09:40:42 PM UTC, comment #2:

This is something that I worried about already when we switched to effects system in 2.0. Freeciv-1.14 and earlier had separate "replaced_by" -field for buildings.

I don't think keeping nreqs for such an use that would be very hard to explain to non-coder modpack authors is the correct thing to do.
What about making "present" tri-state value, with one value being equivalent to current TRUE, and the other two being like current FALSE but one indicating "replacement" and the other "prevention"?

Marko Lindqvist <cazfi>
Project Administrator
Sat 31 May 2014 09:31:36 PM UTC, comment #1:

Lightly tested patch attached.
(I suspect this isn't amenable to autogame testing, since the AI is expected to improve. Probably the correct test is against unmodified code and nreqs-based ruleset.)

This shows up the one place where present=FALSE and nreqs were arguably not entirely equivalent: they could be used by ruleset authors to represent the distinction between a simple negated term in a boolean expression, and a requirement where something's presence is semantically an 'impediment' or 'block' to the effect, because is_effect_disabled() (and hence the UI and AI) would only take notice of the latter.
In my patch, it looks at both (and is renamed to is_effect_prevented()).

For most requirement types ("is this building present") you're unlikely to have wanted the former sense, but for numeric effects like MinSize it's more arguable. That said, I've failed to think of a realistic example where the new logic will prevent ruleset authors expressing something interesting.

This does mean we should take care to define new requirements with the right sense in future, because if ruleset authors are forced to routinely negate them, it might confuse the AI. I think defining numeric requirements in line with causality/progress -- e.g. MinYear, MinSize, MinCulture tend to become truer over time -- helps with that.

This might form an argument for keeping 'nreqs' around; but if it is one, then we'll have to reverse most of our translation of rulesets, since most of our current use of negation is in the 'impediment' sense. I'll assume we won't do that for now.

(file #20890)

Jacob Nevins <jtn>
Project AdministratorIn charge of this item.
Wed 30 Apr 2014 04:11:19 AM UTC, original submission:

In the absence of nreqs(), is_improvement_redundant fails to determine if a given improvement is not providing any useful effect (or cannot provide any useful effect in the future). This is most obviously visible as a lack of strikethrough in worklist management, but may also cause the AI to desire useless buildings.

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 #20890:  trunk-preventing-reqs-present.patch added by jtn (10kB - text/x-patch - trunk r24987)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

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

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 02 Jun 2014 10:34:40 PM UTCjtnStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sat 31 May 2014 09:31:36 PM UTCjtnAttached File-=>Added trunk-preventing-reqs-present.patch, #20890
      StatusNone=>Ready For Test
      Assigned toNone=>jtn
      Operating SystemNone=>Any
    Sun 25 May 2014 11:16:24 AM UTCjtnPlanned Release=>2.6.0
    Wed 30 Apr 2014 04:12:53 AM UTCpersiaDependencies-=>bugs #21115 is dependent
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup