Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Feature #768292

Add /playernation command for setting nation and leader name

Added by Zoltán Žarkov about 4 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Category:
Server
Sprint/Milestone:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

Lt.org does its own workaround for setting player nations in the pregame, so this will let them get rid of that.

0001-Add-playernation-command-for-setting-nation-and-lead.patch (5 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch Zoltán Žarkov, 2018-08-06 10:11 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch (5.04 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch master Zoltán Žarkov, 2018-08-07 08:45 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch (5.28 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch Zoltán Žarkov, 2018-08-08 10:06 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch (5.34 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch Zoltán Žarkov, 2018-08-09 09:56 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch (5 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch Zoltán Žarkov, 2018-08-09 10:14 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch.master (5.35 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch.master Zoltán Žarkov, 2018-08-09 10:23 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch.S3_0 (5.31 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch.S3_0 Zoltán Žarkov, 2018-08-09 10:23 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch.S3_0 (5.31 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch.S3_0 Zoltán Žarkov, 2018-08-09 10:48 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch.master (5.35 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch.master Zoltán Žarkov, 2018-08-09 10:48 PM
0009-Add-playernation-command-for-setting-nation-and-lead.patch (5.39 KB) 0009-Add-playernation-command-for-setting-nation-and-lead.patch master Marko Lindqvist, 2018-08-09 10:55 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch (5.34 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch S3_0 Marko Lindqvist, 2018-08-09 10:56 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch.master (6.92 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch.master Zoltán Žarkov, 2018-08-10 05:30 PM
0001-Add-playernation-command-for-setting-nation-and-lead.patch.s3_0 (6.88 KB) 0001-Add-playernation-command-for-setting-nation-and-lead.patch.s3_0 Zoltán Žarkov, 2018-08-10 05:30 PM
0011-Add-playernation-command-for-setting-nation-and-lead.patch (7.15 KB) 0011-Add-playernation-command-for-setting-nation-and-lead.patch Marko Lindqvist, 2020-04-16 08:07 PM

Related issues

Related to Freeciv - Feature #855495: Set default player names, nations and AI difficultyClosed

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>
Related to Freeciv - Feature #866262: Server command(s) to create players and assign nationsClosed

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>
Blocks Freeciv - Task #673656: S3_1 datafile format freeze (d3f)Closed

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>

History

#1 Updated by Zoltán Žarkov about 4 years ago

I targeted this against S2_6 even though it's format frozen and will make the necessary patches for S3_0 and master later.

#2 Updated by Anonymous about 4 years ago

Maybe google about goto and C ?

#3 Updated by Zoltán Žarkov about 4 years ago

find . -name "*.c" | xargs grep "goto cleanup" | wc -l
113

#4 Updated by Marko Lindqvist about 4 years ago

  • Category set to Server
  • Sprint/Milestone set to 3.0.0

#5 Updated by Anonymous about 4 years ago

C style guidelines strictly forbid use of goto with some exceptions
doc/CodingStyle
@ - In the rare case that you actually use goto, the label should be all
capitals and "out-dented" in the block in which it is contained:

static int frob(int n) {
int i, j;
for (i = 0; i < n; i++) {
for (j = i; j < n; j++) {
if (some_condition(i, j)) {
goto EXIT_LOOP;
}
}
}
EXIT_LOOP:
return 123;
}@

If someone with "mad skils :D" commited crime with goto, it doesn't mean that everyone can spam gotos now.

#6 Updated by Zoltán Žarkov about 4 years ago

It's a general principle of readability that you match the style of surrounding code.

#8 Updated by Marko Lindqvist almost 4 years ago

1. It does not compile! (there's at least two errors)

2.

N_("playernation <player-name> [nation [leader-name]]",

I believe the syntax we have used for other cases with multiple optional arguments is simply [] []

3.

if (!client_can_pick_nation(pnation) || pnation->player != player) {

Is that 'player' supposed to be 'pplayer'? Anyway, shouldn't this test that the nation is free to take, and not that it's already taken by specific player?

4. The 'goto' issue.
4a) Generally new code should not use 'goto'
4b) I would allow it in a case where one does small modification to a function that already uses it, and avoiding additional goto would require bigger refactoring of the function. Here you are adding completely new function, so you have chance to make it right from the beginning.
4c) It does fit the pattern used by other similar functions in stdinhand.c, so there is a good argument for consistency.
4e) As CodingStyle says, in the rare case you use goto, the label should be uppercase.

Overall, I'm undecided about the acceptability of the goto here. I do require minimum change of making the label uppercase, but for now abstain vote about whether goto can be used at all. Ideally there would be another patch to clean out all those existing gotos, and then this could be made both without goto and still consistent with other functions.

#10 Updated by Marko Lindqvist almost 4 years ago

Parameter 'check' is not handled - if it's TRUE, no actual actions should be taken (should only check command syntax)

#11 Updated by Marko Lindqvist almost 4 years ago

  • Blocks Task #656466: S3_0 datafile format freeze (d3f) added

#13 Updated by Marko Lindqvist almost 4 years ago

It still claims (cmd_reply) that it has done the things when only 'check'ed.

#15 Updated by Zoltán Žarkov almost 4 years ago

Disregard previous

#17 Updated by Marko Lindqvist almost 4 years ago

  • Status changed from New to Resolved
  • Assignee changed from Zoltán Žarkov to Marko Lindqvist

#20 Updated by Anonymous almost 4 years ago

Have you even tested that patch ?

You are logged in as 'mirex' connected to Mirex.
/playernation mirex Aztec XX
/playernation mirex
Response: /playernation: No player by the name of 'mirex'.
(Im player mirex

if correct behaviour is to do:

/playernation Xx
/playernation: Nation of player XX reset.
Nothing changed. Im still player mirex, with leadername Xx and nation Aztec

starting game:

AddressSanitizer:DEADLYSIGNAL =================================================================
15142ERROR: AddressSanitizer: SEGV on unknown address 0x000000000040 (pc 0x7f7138b3d2c6 bp 0x7ffe04a36ff0 sp 0x7ffe04a36768 T0)
15142The signal is caused by a READ memory access.
15142Hint: address points to the zero page.

Why response even starts with /playernation ?

When I pick nation by hand I get normal reply:
mirex is the Babylonian ruler Mirex.

Do you know what are you doing here?
if (ntokens 0) {
....
free_tokens(token, ntokens); /* ? */

if (ntokens  3) {
server_player_set_name(pplayer, token[2]);
} else { /* ;) :P :D */
server_player_set_name(pplayer, token[0]);
}

What about total rewrite with 'switch' ?

#22 Updated by Marko Lindqvist almost 4 years ago

  • Assignee deleted (Marko Lindqvist)

#23 Updated by Marko Lindqvist almost 4 years ago

Any news about the problems mirex was seeing?

#24 Updated by Zoltán Žarkov almost 4 years ago

Latest patches fixed those issues.

#25 Updated by Marko Lindqvist almost 4 years ago

  • Status changed from Resolved to In Progress

set nationset all
create cazfi
playernation cazfi irish

->

1: in fc_rand_debug() [../../../src/utility/rand.c::87]: assertion 'rand_state.is_init' failed.

#26 Updated by Marko Lindqvist almost 4 years ago

Marko Lindqvist wrote:

1: in fc_rand_debug() [../../../src/utility/rand.c::87]: assertion 'rand_state.is_init' failed.

Maybe it's best to resolve this in two phases. To get this ticket resolved (phase 1), just make all the parameters mandatory, so nothing gets determined by fc_rand().
Future ticket (phase 2) may then implement proper way of getting randomized leader name and sex (most likely by introducing special value "random" for them, which will be turned to actual sex or name only when game starts and fc_rand() is initialized)

#27 Updated by Marko Lindqvist about 3 years ago

  • Sprint/Milestone deleted (3.0.0)

#28 Updated by Marko Lindqvist about 3 years ago

  • Blocks deleted (Task #656466: S3_0 datafile format freeze (d3f))

#29 Updated by Marko Lindqvist over 2 years ago

  • Blocks Task #673656: S3_1 datafile format freeze (d3f) added

#30 Updated by Marko Lindqvist over 2 years ago

  • Related to Feature #855495: Set default player names, nations and AI difficulty added

#31 Updated by Louis Moureaux over 2 years ago

  • Related to Feature #866262: Server command(s) to create players and assign nations added

#32 Updated by Marko Lindqvist over 2 years ago

Marko Lindqvist wrote:

To get this ticket resolved (phase 1), just make all the parameters mandatory, so nothing gets determined by fc_rand().

Attached version of the patch goes to that direction as much as necessary; it makes is-male mandatory when setting the nation.
Also fixes a bug that leader name is also set when there's more parameters than up to leader name.

#33 Updated by Marko Lindqvist over 2 years ago

  • Status changed from Resolved to Closed
  • Assignee set to Marko Lindqvist

Also available in: Atom PDF