bugFreeciv - Bugs: bug #20577, new parameter gameloss_style:...

 
 
Show feedback again

bug #20577: new parameter gameloss_style: GameLoss player's property is redistributed instead of disappearing

Submitted by:  Not Given <imhotep>
Submitted on:  Wed 27 Feb 2013 06:18:26 PM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Release: Operating 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.

 

(Jump to the original submission Jump to the original submission)

Thu 04 Apr 2013 07:29:17 PM UTC, SVN revision 22667:

Added ruleset option "gameloss_style" to control what effects in addition
to player death loss of gameloss unit causes.

Patch by and myself

See bug #20577

(Browse SVN revision 22667)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon 01 Apr 2013 08:47:17 PM UTC, comment #22:

- Use give_midgame_initial_units() (from patch #3812) to give units for newly created player. This also corrects behavior so that Leader units are given instead of GameLoss ones (if this ever makes any difference)

I now also tested this. It works at least technically - I'm not sure if everything makes sense for the gameplay, but I consider this ready for initial commit. We can improve it later.

(file #17648)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 29 Mar 2013 11:40:40 PM UTC, comment #21:

patch #3812 provides new function to use for giving leader units midgame.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 28 Mar 2013 10:47:30 PM UTC, comment #20:

- Added cleartext names for values to be used in ruleset
- Added gameloss_style entries to rulesets, with comments documenting it
- More minor style fixes
- Detect the case when there's more tokens in ruleset than maximum handled
- Do not log "Empire is too small for civil war" when in fact civilwar effect for gameloss was not active
- Changed wording of log message when no more tech loot is available - it used to imply that no techs were got even if some, but not requested number, were got

(file #17585)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun 24 Mar 2013 10:15:34 PM UTC, comment #19:

- Various style fixes
- Function header for give_distorted_map() reorganized
- Some variable declarations moved from the middle of the block to the beginning
- Empty lines added between variable declarations and first line of actual code
- Some block beginning braces moved to same line with "if"
- From "if (barbarians) {} else {}" moved parts present in both blocks (=executed always) outside to avoid code duplication
- In case of illegal value in ruleset, return error from ruleset loading (was just logging error and continuing loading)

(file #17536)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 21 Mar 2013 09:24:07 PM UTC, comment #18:

> - Updated to apply on svn HEAD (r22531)


Thank you for that. I guess it would have been exceedingly more trouble for me than it was for you.
(I have found other ways tho apply patches than "svn patch" but, I haven't tried them yet and I see a risk to mess up my working copy with all of my current changes.)

So now, how do we proceed? I see you fixed that capstr, too.

Can I do anything now on this topic, or should I wait for your comments after you did some testing?

(It worked for me on many games, but I don't have the latest version from trunk, and I have quite a lot of other changes in my working copy by now. There was a typo in my last post, instead of "I have /no/ more than one patch" it should read "I have /now/ more than one patch" ... .)

Not Given <imhotep>
Wed 20 Mar 2013 06:32:25 AM UTC, comment #17:

- Updated to apply on svn HEAD (r22531)

(file #17453)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue 19 Mar 2013 12:12:02 PM UTC, comment #16:

OK, here comes the next try.

It still doesn't contain the capstr thing, and still is against r22446.

Last time I updated to the trunk version it took me short of a day to cope with all the changes and to get everything working again, so I am very reluctant here.

Further, I have no more than one patch and many debugs in my working copy, so I have to clean each svn diff manually, which naturally is error prone. (No, svn patch does not work on my system.)

(file #17450)

Not Given <imhotep>
Tue 19 Mar 2013 10:32:32 AM UTC, comment #15:

It's generated file (that's what "_gen" part in its name tries to tell). You shouldn't edit it directly (as any changes will be overwritten when it next gets regenerated) but common/packets.def

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue 19 Mar 2013 10:19:07 AM UTC, comment #14:

Excuse me, I had a concussion and still have trouble concentrating, so I probably make even more mistakes as from natural dumbness alone.

I try to include missing packets_gen but fail, because it "is not under version control", not even when I use the original code:

$ cp common/packets_gen.h common/Imhotep_packets_gen.h
$ svn up -r22446 packets_gen.h
At revision 22446.
$ svn diff server/unittools.c server/ruleset.c server/maphand.c server/maphand.h server/barbarian.c server/barbarian.h server/plrhand.c common/packets_gen.h common/fc_types.h > gamelossStyle22446_v4.patch
svn: 'common/packets_gen.h' is not under version control

Not Given <imhotep>
Tue 19 Mar 2013 08:58:12 AM UTC, comment #13:

game.info is sent to client side, so server and client have to agree on what it contains. If new server would send gameloss_style to old client, client would be expect that value to be used for something completely different purpose.

I tried to make some testign for the first time, but:
- ruleset.c part does not apply to current svn HEAD. That was trivial to fix. In the process I noticed that game.info.gameloss_style is first set to GAMELOSS_STYLE_CLASSICAL and then values are orred to it. I'd recommend setting it to explicit 0 first and orring on top of that. Set to default value GAMELOSS_STYLE_CLASSICAL only if ruleset gives no value

But then compiler errors begun:
src.patched/server/plrhand.c: In function 'kill_player':
src.patched/server/plrhand.c:159:17: error: 'struct packet_game_info' has no member named 'gameloss_style'
src.patched/server/plrhand.c:159:35: error: 'GAMELOSS_STYLE_CWAR' undeclared (first use in this function)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 13 Mar 2013 06:46:08 PM UTC, comment #12:

OK, I see we were talking about different things.

But I still have no idea how this patch would affect any client side capabilities.

All new features, as far as I can see, are handled exclusively on the server side.

Or has it to do something with the synchronizing issue I mentioned?

Not Given <imhotep>
Wed 13 Mar 2013 05:57:28 PM UTC, comment #11:

> So as I see it, a new capsrting will only harass any modpack
> developers, being forced to do replacements in many files,
> untimely, and, without any benefit for the final standard users.


There's many kind of capstrings. We were not talking about ruleset one, but network protocol one. It might not be worth maintaining in the patch as it sometimes changes several times a day, but it must be updated when committing - incompatible server & client won't work correctly together, leading hard-to-debug-problems, potentially to crashes, so they must know their incompatibility from the start not to even start the game.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 13 Mar 2013 05:44:28 PM UTC, comment #10:

This "submit" is clearly an error, I fixed it in my working copy.

The effect is that the cities submitting are not randomly chosen, as intended, but, after the first one, picked in the sequence order the iteration provides.
So the game is playable as it is.

I will submit the correction together with potential future corrections or for this change aalone, if anyone urges me to.

I doubt if the capstr change is really worth it.

The patch is on an already old version of the quickly changing trunk, probably the whole patch will need extensive rework for the version it will finally be merged to.
(The trouble I had upgrading to r22446 was nearly more than the time it took to write the code. (I'm working on a separate modpack.))

The change is downward compatible in that a missing parameter just works out as if 0 was given, for the "classic" style.
Old versions will silently ignore this option.

So as I see it, a new capsrting will only harass any modpack developers, being forced to do replacements in many files, untimely, and, without any benefit for the final standard users.

I am more concerned about the synchronizing issue. An observer would not see a new palace nor any new GameLoss units until the next turn.

Not Given <imhotep>
Wed 13 Mar 2013 05:10:42 AM UTC, comment #9:

Network capstr is defined in fc_version. You just need to change the mandatory capability to mark version prior and after this patch incompatible with each other.

I still have not tested this patch, but one thing i noticed while reading through it:
"submit" is set to FALSE before cities are iterated, but never inside the iteration. Once it gets set for one city, it remains set for all the following cities.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 09 Mar 2013 11:10:20 PM UTC, comment #8:

And again a new version.

On server start, not only the requested ruleset is loaded but another one before that. I was not aware of that and don't know if this is a bug, but it really did cost me a lot of time.

I still have no idea what to do with "network capstr bump needed".

Palace is removed when the cities are transferred. There is code to build a new one for the new player, along with any GameLoss units. New Palace is even built for barbarians, and I'm not definitive if this is really a good idea.

One known bug is that the Palace as well as the GameLoss unit(s) are only visible next turn. They are effectively there, but invisible. I haven't yet fond the proper functions to call for synchronization.

The player was killed ("pplayer->is_alive = FALSE;") even before I touched the code. But the civil_war-method won't work for a dead player.

The entry in game.ruleset now looks like

gameloss_style = "GAMELOSS_STYLE_BARB|GAMELOSS_STYLE_LOOT|GAMELOSS_STYLE_CWAR"

and I don't know how to get rid of the "GAMELOSS_STYLE_" prefix in the ruleset but keep it in the code (without introducing something new that probably already exists somewhere).

(file #17415)

Not Given <imhotep>
Thu 07 Mar 2013 08:20:00 PM UTC, comment #7:

> As new value is transferred to client side, this changes network protocol -> network capstr bump needed

I don't have even an idea on where to look for a clue on this ... is there a working example that I can study?

Meanwhile, I did a lot of changes to my working copy, not only on the ticket here.

Latest thing that drives me mad is that game.info.gameloss_style is set correctly in ruleset.c, but when lightning strikes the enemy leader it evaluates to 0, and no effect takes place at all.

I guess I have to do a whole lot of time consuming try&error debugging.

In the faint hope that anyone out there has had this problem before I post all of my diffs as a "patch".

(file #17405)

Not Given <imhotep>
Thu 07 Mar 2013 08:45:34 AM UTC, comment #6:

- As new value is transferred to client side, this changes network protocol -> network capstr bump needed

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 07 Mar 2013 07:31:45 AM UTC, comment #5:

- No changes to ruleset.c? I thought the value was supposed to be read from the ruleset.
- Don't use magic numbers "gameloss_style & 4". In fact you should probably make gameloss styles bitwise specenum
- Spaces between many elements missing "while (n<0)" -> "while (n < 0)"
- Block starting brace should not be in new line
"while ()
{"
->
"while () {"

- Braces needed even when block following "if" is just one line (so if another line gets ever added, things will not blow up)
"if (!fc_rand(3))
break; /* Out of luck */
" ->
"if (!fc_rand(3)) {
break; /* Out of luck */
}"

- Empty line needed between variable declararions and code
- "if (evcsize < 3) evcsize = 0; else evcsize -=3;" ->
"if (evcsize < 3) {
evcsize = 0;
} else {
evcsize -= 3;
}"

- Decrlaration of "int conqsize" after previous code (not in the beginning of block)
- What happens to palace if it's in transfered cities? You disable "savepalace" for a while so it won't be moved from city to city multiple times, but should you finally rebuild it somewhere?
- Comment type "palyer" -> "player"

- "+ /* out of sheer cruelty we reanimate the player
+ * so he can behold what happens to his empire */
+ pplayer->is_alive = TRUE;"
sounds worrisome. Should't the player stay alive all the time instead (I'm worried what may happen while (s)he is temporarily marked dead) Actually, from the patch it seems that this gets overwritten with "pplayer->is_alive = FALSE;" anyway

- As create_barbarian_player() may return existing barbarian with already established capital, you should protect against trying to give him second one.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 06 Mar 2013 09:33:08 AM UTC, comment #4:

ouch, yes, right one comes here

(file #17399)

Not Given <imhotep>
Wed 06 Mar 2013 05:50:25 AM UTC, comment #3:

> here is a new version of the patch


Doesn't look like correct patch for this ticket...

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue 05 Mar 2013 07:12:29 PM UTC, comment #2:

here is a new version of the patch

I did a lot of debugging on other issues and I hope I did not mix in other changes, or leave out something.

There is still a known bug: neither the new GameLoss unit nor the new Palace are visible immediately but only after the next turn.

I would like a feedback soon because I am on other things and want to settle this matter to "clear my table" before differences to the trunk will make new changes necessary, and, before I get confused with the many changes on the my local version.

(file #17395)

Not Given <imhotep>
Thu 28 Feb 2013 03:25:00 AM UTC, comment #1:

While it still needs some work, it looks promising first version.

- In the future, please submit new features as "patch" and not "bug"

- Debug stuff currently just commented out needs to be removed
- Most log_normal() calls should be made log_debug() if not removed completely
- I don't point out all the individual style issues from this version, please check it against doc/CodingStyle and once you submit updated version I can point out the remaining ones
- You should define default value of its own to gameloss_style, not to use RS_DEFAULT_BASE_POLLUTION
- gameloss_style should be documented in ruleset comments

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 27 Feb 2013 06:18:26 PM UTC, original submission:

When I tried to make a new modpack based on Ancients, I stumbled on an option "gameloss_style".
According to the comment, cities conquered from players still alive were given back to the previous owner. (So far, I found this already coded).
Depending on the parameter value, the remaining cities are either destroyed or fall to the hands of some barbarians.

On a local copy based on version 2.4.99-dev, revision 22214, I implemented some code to do this.

There is also code to have civil war (before any cities fall to barbarians or vanish without a trace).
The new player gets a palace of its own, and, a Leader (it would be unfair if the AI had no GameLoss unit to trigger all this features when we have chased them down).

Also, I coded some benefits for the player "who liberates the oppressed people from the enemy leader":

up to 3 techs are stolen

a random amount of gold is taken ("in compensation of the war efforts"), from 0 to the amount of gold the victim has had

using some interrogation, we press a (distorted) map from the captured leader

a random number of cities (about a quarter on average, with lesser probability on higher numbers) can be convinced to "join the righteous cause" of the victor

It worked on my copy on a few games, but probably has lots of yet unknown bugs (I'm new to the code and this is my first try).

Not Given <imhotep>

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #17648:  GamelossStyle-4.patch added by cazfi (20kB - text/x-diff)
file #17585:  GamelossStyle-3.patch added by cazfi (20kB - text/x-diff)
file #17536:  GamelossStyle-2.patch added by cazfi (13kB - text/x-diff)
file #17453:  GamelossStyle.patch added by cazfi (14kB - text/x-diff)
file #17405:  diff22446.patch added by imhotep (28kB - text/x-patch)
file #17366:  gamelossStyle22446.patch added by imhotep (11kB - text/x-patch)

 

Depends on the following items

Digest:
   patch dependencies.

Items that depend on this one: None found

 

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

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 04 Apr 2013 07:29:28 PM UTCcazfiStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Mon 01 Apr 2013 08:47:17 PM UTCcazfiAttached File-=>Added GamelossStyle-4.patch, #17648
    Sat 30 Mar 2013 12:31:48 PM UTCjtnSummarynew parameter gameloss_style in game.ruleset=>new parameter gameloss_style: GameLoss player's property is redistributed instead of disappearing
    Fri 29 Mar 2013 11:40:40 PM UTCcazfiDependencies-=>Depends on patch #3812
    Thu 28 Mar 2013 10:47:30 PM UTCcazfiAttached File-=>Added GamelossStyle-3.patch, #17585
    Sun 24 Mar 2013 10:15:34 PM UTCcazfiAttached File-=>Added GamelossStyle-2.patch, #17536
      StatusNone=>Ready For Test
      Assigned toNone=>cazfi
      Release22446=>
      Planned Release=>2.5.0
    Wed 20 Mar 2013 06:32:25 AM UTCcazfiAttached File-=>Added GamelossStyle.patch, #17453
    Tue 19 Mar 2013 12:12:02 PM UTCimhotepAttached File-=>Added gamelossStyle22446_v4.patch, #17450
    Sat 09 Mar 2013 11:10:20 PM UTCimhotepAttached File-=>Added gamelossStyle22446_v3.patch, #17415
    Thu 07 Mar 2013 08:20:00 PM UTCimhotepAttached File-=>Added diff22446.patch, #17405
    Wed 06 Mar 2013 09:33:08 AM UTCimhotepAttached File-=>Added gamelossStyle22446_v2.patch, #17399
    Tue 05 Mar 2013 07:12:29 PM UTCimhotepAttached File-=>Added gameloss_defend_22446.patch, #17395
    Thu 28 Feb 2013 03:25:00 AM UTCcazfiCategoryNone=>general
    Wed 27 Feb 2013 06:18:27 PM UTCimhotepAttached File-=>Added gamelossStyle22446.patch, #17366
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup