bugFreeciv - Bugs: bug #21115, [metaticket] Negated requirements...

 
 
Show feedback again

bug #21115: [metaticket] Negated requirements ('negated'=TRUE, 'present'=FALSE) do not work reliably

Submitted by:  Marko Lindqvist <cazfi>
Submitted on:  Sat 07 Sep 2013 04:05:21 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Priority: 5 - NormalStatus: In Progress
Assigned to: NoneOpen/Closed: Open
Release: Operating System: None
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 30 Apr 2014 04:24:22 AM UTC, comment #18:

And I've completed the review of my rebase, with the following results:

(1) was mitigated with patch #4451, but has other issues. Bug #21991 raised for that, but not a dependency of this bug, as the issue is more general than just present==FALSE.

(2) is ideally a no-op given empty nreqs. Included in the wider patch #4679 (which is mostly irrelevant to present==FALSE, except for one comment change, and the underlying motivation)

During preparation of patch #4679, found another case of dependency on "nreqs", rather than wider requirement processing, raised as bug #21992 (which this depends upon).

(3) patch #4451 addressed this in a better way (although not textually near my earlier changes)

(4) root issue raised as bug #21982 (dependency of this bug)

I did not find anything else in my prior notes that appears to remain unaddressed by the current codebase.

Emmet Hikory <persia>
Project Member
Sun 20 Apr 2014 08:03:04 PM UTC, comment #17:

I rebased an old WIP branch for this cleanup this weekend, and it seems that vast majority of the issues I had previously identified have been addressed. The only remaining parts in my branch were:

1) aicity:affected_unit_class(), which needs to return a list, rather than a unit_class (with matching adjustments for all callers), in order to support requirements sets that match multiple unit classes.

2) removing is_effect_disabled from the public effects API, and removing the section using it from aicity.c

3) aicity:adjust_improvement_wants_by_effects(), which needs to consider that the improvement under consideration might be present==FALSE

4) effects:is_effect_useful(), which needs to consider that VUT_IMPROVEMENT may have present==FALSE

That said, the notes on my branch indicated there were also problems in helptext (most of which appear to be addressed by bug #21454, patch #4400 or elsewhere). I can't remember if I completed an exhaustive listing of the issues in the notes for this patch, so the above may not be complete. I don't quite trust my rebase, and know I hadn't subjected the prior branch to significant testing, so will want to reinvestigate these changes before filing bugs for them.

Emmet Hikory <persia>
Project Member
Sun 20 Apr 2014 11:57:41 AM UTC, comment #16:

Recent discussion in patch #4401 highlights that there are still known outstanding issues in the code, not yet raised as separate tickets.

Jacob Nevins <jtn>
Project Administrator
Sat 04 Jan 2014 08:21:31 PM UTC, comment #15:

> Just to make sure we don't do duplicate work: have you done
> any work to convert our rulesets?

No.

Jacob Nevins <jtn>
Project Administrator
Sat 04 Jan 2014 08:01:49 PM UTC, comment #14:

> Hm, I'm not wild about potentially disallowing rulesets on
> stable branches.


True, that would be quite blatantly against datafile format freeze. log_error() (limited to one client popup even if there's multiple negated reqs) sounds sensible - in most cases it just makes ruleset author to fix the ruleset (should be clearly instructed in the message)

> (I suppose autogames with rulesets defined each way would be
> one way to get a clue...)


Just to make sure we don't do duplicate work: have you done any work to convert our rulesets? If not, I'll create patch for that (not to be committed yet, obviously, but to be used in testing).

> There's a weak argument which says that since the syntax on
> 2.5 and 2.6 will be different (negated=TRUE vs present=FALSE)


It's quite perfect counter-argument to the main argument for using negated = TRUE as default; that it will save ruleset authors from updating requirements when 2.6 comes out.

Marko Lindqvist <cazfi>
Project Administrator
Sat 04 Jan 2014 07:48:21 PM UTC, comment #13:

> S2_4: nreqs must be used, patch ruleset sanity checking to
> disallow negated = TRUE reqs
> Oh, if we are still going to release 2.3.5, sanity check patch
> planned for S2_4 should go to S2_3 too.

Hm, I'm not wild about potentially disallowing rulesets on stable branches. An ultra-stable 2.3 update is not much use if the ruleset you were previously happy with (didn't tickle any bugs) now doesn't load at all.
I'd go for emitting log_error() on these branches, at most -- that allows us to communicate that it's not recommended, and shows up fairly obviously in the client. (Similar to what we do when savegame loading goes a bit wonky.)
On 2.5 and 2.6 we still have time to set a hard policy IMO...

> What about S2_5? Is it transition version where both are
> supposed to work?

That really depends on whether we can make present=FALSE reliable in time. Right now I don't think we've scoped out how much work there is to get there from here -- all the stuff I've fixed, I happened to spot on the way somewhere else, I haven't done an exhaustive survey.
(I suppose autogames with rulesets defined each way would be one way to get a clue...)

> maybe it's simply too late in stabilization to make
> present = FALSE the default now?)

There's a weak argument which says that since the syntax on 2.5 and 2.6 will be different (negated=TRUE vs present=FALSE), we should only start encouraging this style once the syntax has settled (i.e. in 2.6) to avoid two lots of ruleset rewriting for third parties.

Jacob Nevins <jtn>
Project Administrator
Sat 04 Jan 2014 08:41:27 AM UTC, comment #12:

Oh, if we are still going to release 2.3.5, sanity check patch planned for S2_4 should go to S2_3 too.

Marko Lindqvist <cazfi>
Project Administrator
Sat 04 Jan 2014 08:27:56 AM UTC, comment #11:

For effect reqs & nreqs, I think the plan now is:

S2_4: nreqs must be used, patch ruleset sanity checking to disallow negated = TRUE reqs
2.6: nreqs are to be removed (or only deprecated?). present = FALSE reqs must be used.

What about S2_5? Is it transition version where both are supposed to work? Or do we handle it like S2_4? If both are supported, which method is the preferred one (especially for new rulesets, should we recommend safe-and-reliable nreqs, or less-work-in-future-update present = FALSE. Also, supplied rulesets should use what ever is the preferred method - maybe it's simply too late in stabilization to make present = FALSE the default now?)

Marko Lindqvist <cazfi>
Project Administrator
Fri 03 Jan 2014 01:33:05 PM UTC, comment #10:

Expanding this metaticket to be about all bugs in negated requirements, because that's what I've already hooked up, on the understanding that bugs in non-effects negated requirements are more severe. We can split it again later if necessary.

Jacob Nevins <jtn>
Project Administrator
Fri 03 Jan 2014 01:31:30 PM UTC, comment #9:

All fair enough.
Now I've realised the impact of bug #21417 was less than I thought, it's more plausible that we don't need the client/server compatibility check.

(FWIW, I've reproduced the original issue of this ticket with Tile_Workable with current svn code, and confirmed that bug #21417 fixes the symptom. Doubtless bug #21144 wasn't helping.)

Jacob Nevins <jtn>
Project Administrator
Fri 03 Jan 2014 05:41:43 AM UTC, comment #8:

> Should we try to fix negated requirements on stable S2_4 branch?


As far as we speak about effect requirements, nreqs vector is the right way to implement them in 2.4 rulesets (I'd actually add ruleset loading time sanity check preventing using other kind of negated requirements for effects in 2.4)

Requirements for other things should have broken negative requirements considered bugs. I'm not for adding ruleset capability. 1) rulesets are supposed to be compatible within major version, sans the behavior caused by bugs themselves. 2) rulesets created for 2.4.0 and 2.4.1 already use negated requirements as much as they need.

Marko Lindqvist <cazfi>
Project Administrator
Fri 03 Jan 2014 05:30:58 AM UTC, comment #7:

Should we try to fix negated requirements on stable S2_4 branch? Fixes so far are probably relatively unintrusive (don't know about the AI yet).

If we do, can we add an optional ruleset capability "negated_requirements_really_work", so that rulesets which use them aren't loaded onto old servers where they won't work?
(May also want some capability handshaking with clients, as some of the fixes are client-side. Which is all an argument that maybe it has to be a 2.5 feature.)

Jacob Nevins <jtn>
Project Administrator
Thu 02 Jan 2014 08:13:05 PM UTC, comment #6:

At this point I would consider this one meta-ticket of all the related problems. Buf #21144 was certainly the one I were experiencieng at the time I opened this ticket.

This ticket can be closed if someone tests that negated effect requirements now work. AI must be checked - it's not easy to see from its behavior if it ever considers requirements negated, or does it research tech that will disable the building it desperately wants...

Marko Lindqvist <cazfi>
Project Administrator
Thu 02 Jan 2014 08:00:42 PM UTC, comment #5:

...oh, perhaps the original report of a 'client only problem' can be explained by bug #21417?

Jacob Nevins <jtn>
Project Administrator
Thu 02 Jan 2014 04:34:16 PM UTC, comment #4:

> This isn't just bug #21144, is it?

I haven't tested, but will close this assuming it's a duplicate of the other bug soon, if no-one objects.

Jacob Nevins <jtn>
Project Administrator
Sun 03 Nov 2013 03:32:50 PM UTC, comment #3:

This isn't just bug #21144, is it?

Jacob Nevins <jtn>
Project Administrator
Sat 07 Sep 2013 09:09:42 PM UTC, comment #2:

Chaning this ticket to handle effects related negated requirements.
At least one problem preventing us from simply negating result from normal requirement check is RPT_POSSIBLE. If we don't know if requirement is fulfilled, RPT_POSSIBLE check will return TRUE. If we then just negated that for negated requirement, we get FALSE. But in fact it's also possible that negated requirement is the one being fulfilled.

Marko Lindqvist <cazfi>
Project Administrator
Sat 07 Sep 2013 04:20:36 PM UTC, comment #1:

Well, I forgot that's to be known problem - for effects there's nreqs, and using that works.

Marko Lindqvist <cazfi>
Project Administrator
Sat 07 Sep 2013 04:05:21 PM UTC, original submission:

With:

[effect_tile_workable]
type = "Tile_Workable"
value = 1
reqs =
{ "type", "name", "range", "present"
"Terrain", "Unaccessible", "Local", FALSE
}

client shows all tiles except "Unaccessible" as available in city dialog, and won't allow setting worker to them. This seems to be client only problem as server automatically places workers to sensible places.

Marko Lindqvist <cazfi>
Project Administrator

 

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

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Digest:
   bug dependencies, patch dependencies.

Digest:
   patch dependencies.

 

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

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 01 May 2014 07:16:23 AM UTCpersiaDependencies-=>Depends on bugs #21999
    Wed 30 Apr 2014 04:12:53 AM UTCpersiaDependencies-=>Depends on bugs #21992
    Mon 28 Apr 2014 04:30:51 AM UTCpersiaDependencies-=>Depends on bugs #21982
    Sun 20 Apr 2014 11:45:48 AM UTCjtnPlanned Release=>2.6.0
    Sun 20 Apr 2014 11:37:45 AM UTCjtnDependencies-=>Depends on patch #4451
    Fri 03 Jan 2014 03:50:32 PM UTCjtnDependencies-=>Depends on bugs #21430
    Fri 03 Jan 2014 03:50:08 PM UTCjtnDependencies-=>Depends on bugs #21432
    Fri 03 Jan 2014 01:33:05 PM UTCjtnSummary[metaticket] Negated requirements for effects (\'negated\'=TRUE, \'present\'=FALSE) do not work=>[metaticket] Negated requirements ('negated'=TRUE, 'present'=FALSE) do not work reliably
    Fri 03 Jan 2014 01:31:36 PM UTCjtnAssigned tojtn=>None
    Fri 03 Jan 2014 05:27:54 AM UTCjtnDependencies-=>Depends on patch #3841
    Fri 03 Jan 2014 05:27:34 AM UTCjtnDependencies-=>Depends on bugs #21425
    Fri 03 Jan 2014 05:27:18 AM UTCjtnDependencies-=>Depends on bugs #21424
    Fri 03 Jan 2014 05:27:00 AM UTCjtnDependencies-=>Depends on bugs #21417
    Fri 03 Jan 2014 05:26:10 AM UTCjtnDependencies-=>Depends on bugs #21420
    Fri 03 Jan 2014 05:25:55 AM UTCjtnDependencies-=>Depends on bugs #21144
    Fri 03 Jan 2014 05:25:39 AM UTCjtnSummaryNegated requirements for effects do not work=>[metaticket] Negated requirements for effects ('negated'=TRUE, 'present'=FALSE) do not work
    Thu 02 Jan 2014 08:13:05 PM UTCcazfiStatusNeed Info=>In Progress
    Thu 02 Jan 2014 04:34:16 PM UTCjtnStatusNone=>Need Info
      Assigned toNone=>jtn
    Sat 07 Sep 2013 09:09:42 PM UTCcazfiSummaryClient gets negated "Tile_Workable" effect requirement wrong=>Negated requirements for effects do not work
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup