patchFreeciv - Patches: patch #3974, Replace terrain.ruleset's...

 
 
Show feedback again

patch #3974: Replace terrain.ruleset's (pollution|fallout)_(food|shield|trade)_penalty with yet another output effect

Submitted by:  Sveinung Kvilhaugsvik <sveinung>
Submitted on:  Fri 05 Jul 2013 06:45:01 PM UTC  
 
Category: generalPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
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)

Tue 23 Jul 2013 09:11:37 PM UTC, comment #11:

Committed, though I noticed that documentation in README.effects could be better - Output_Per_Tile and Output_Tile_Punish_Pct have just opposite wording with no indication why they are both needed instead of giving just negative value to the other.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue 23 Jul 2013 09:02:40 PM UTC, SVN revision 23099:

Replaced penalties defined for Polluted and Fallout tiles with new Output_Tile_Punish_Pct
effect.

Patch by Sveinung Kvilhaugsvik

See patch #3974

(Browse SVN revision 23099)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 19 Jul 2013 09:50:37 AM UTC, comment #9:

Patch #3990 changed code used as context for this patch. Make it apply again. No other changes.

(file #18348)

Anonymous
Fri 19 Jul 2013 02:09:22 AM UTC, comment #8:

> Of those options I think one that uses nreqs exclusively makes things more obvious,

Agreed.

Tested all rule sets in autogames. civ2civ3 had a different result sometime after turn 600. multiplayer wasn't completed but was the same (except the order of the stored vars) at turn 600. The rest had the same result.

Anonymous
Tue 16 Jul 2013 09:20:05 PM UTC, comment #7:

Of those options I think one that uses nreqs exclusively makes things more obvious, both for human reader and maybe for helptext generation if we implement it for this feature.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 11 Jul 2013 02:57:45 PM UTC, comment #6:

I don't mind changing it. (I stopped believing that using a minus sign would be easier to read when you missed it)

Attached are two alternatives. One use nreqs exclusively. The other use nreqs and addition.

I haven't tested them in auto games yet. I can do that for the one you prefer.

(file #18254, file #18255)

Sveinung Kvilhaugsvik <sveinung>
Project Member
Wed 10 Jul 2013 07:31:08 PM UTC, comment #5:

Sorry I missed the minus sign. For question if nreqs or current method of subtraction should be used I think I would slightly prefer nreqs for consistency - that's the way other similar situations in our rulesets have been handled, so it's what people are used to and expect (just like I did). But as "slight" preference, any opposing opinion expressed is likely to get honoured.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 10 Jul 2013 04:15:45 PM UTC, comment #4:

> If tile has both Pollution and Fallout, all three effects are active, resulting in 50% + 50% + 25% = 125% penalty?

That is 50% + 50% + -25% = 75% penalty.

Fun fact: I started using nreqs. Then I rewrote the effect definitions using negative numbers. I believed that would be easier to read.

Should I rewrite the effect definitions using nreqs and no addition or subtraction at all?

Sveinung Kvilhaugsvik <sveinung>
Project Member
Wed 10 Jul 2013 02:03:12 PM UTC, comment #3:

Looking the effects definitions I wonder why there's no nreqs for anything. If tile has both Pollution and Fallout, all three effects are active, resulting in 50% + 50% + 25% = 125% penalty? If effect that has only "Pollution" as req would have nreq "Fallout" (or vice versa), this would be 50% + 25% = 75% instead.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 08 Jul 2013 01:44:33 AM UTC, comment #2:

> On the contrary, this is needed as part of generic extras to get rid of the way pollution is hardcoded to be S_POLLUTION & S_FALLOUT.

Good. I hoped that would be the case.

> I saw only one coding style issue: "output" was declared in the middle of the block it now is in (variables must be declared before any code lines in the same block)

Fixed in attached version.

It has been tested by running an autogame with and without the patch until the game ended or turn 500 (or above) was reached. The end save files were then compared. The following rule sets was tested: alien, civ2, civ2civ3, classic and multiplayer. Civ1 refused to run auto games with and without it.

(file #18208)

Sveinung Kvilhaugsvik <sveinung>
Project Member
Sat 06 Jul 2013 07:04:38 AM UTC, comment #1:

> If this path delays the extra generalization feel free to
> reject it.


On the contrary, this is needed as part of generic extras to get rid of the way pollution is hardcoded to be S_POLLUTION & S_FALLOUT.

I saw only one coding style issue: "output" was declared in the middle of the block it now is in (variables must be declared before any code lines in the same block)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 05 Jul 2013 06:45:01 PM UTC, original submission:

Replace terrain.ruleset's (pollution|fallout)_(food|shield|trade)_penalty with yet another output effect and remove the hard coded reference to pollution and fallout when applying the output penalty.

To avoid having to rebalance rule sets the new effect is in the location where the current pollution penalty is applied. To promote the same goal it decides how much to subtract in stead of how much to keep. (I have a version where the effect decides how much to keep. That results in 3 becoming 1 in stead of 3 becoming 2)

The current settings are applied in two steps: pollution and fallout. The effect is applied in one step. Because conversions to integer only happens once a tile that has pollution + fallout may produce a different result than the current code depending on the percentage to subtract and the amount of output.

This patch applies on top of patch 3965 and patch 3967. It has been tested using the experimental rule set. An auto game gave the same save file. The pollution, fallout and their combination were verified manually.

If this path delays the extra generalization feel free to reject it. If not let me know so I can test it for the other rule sets as well.

Sveinung Kvilhaugsvik <sveinung>
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 #18348:  pollutedTile-5.patch added by None (28kB - text/x-patch)
file #18255:  pollutedTile-nreqs.patch added by sveinung (28kB - text/x-patch)
file #18208:  pollutedTile-2.patch added by sveinung (25kB - text/x-patch)
file #18205:  pollutedTile.patch added by sveinung (25kB - text/x-patch)

 

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 sveinung (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 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 23 Jul 2013 09:11:37 PM UTCcazfiStatusReady For Test=>Done
      Assigned toNone=>cazfi
      Open/ClosedOpen=>Closed
    Fri 19 Jul 2013 09:50:37 AM UTCNoneAttached File-=>Added pollutedTile-5.patch, #18348
    Tue 16 Jul 2013 09:20:05 PM UTCcazfiCategoryNone=>general
      StatusNone=>Ready For Test
      Planned Release=>2.6.0
    Thu 11 Jul 2013 02:57:45 PM UTCsveinungAttached File-=>Added pollutedTile-addition-nreqs.patch, #18254
      Attached File-=>Added pollutedTile-nreqs.patch, #18255
    Mon 08 Jul 2013 01:44:33 AM UTCsveinungAttached File-=>Added pollutedTile-2.patch, #18208
    Fri 05 Jul 2013 06:45:01 PM UTCsveinungAttached File-=>Added pollutedTile.patch, #18205
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup