I think we should move checkpoint onto separated entity, called trigger, which will contains counter, description, min and max fields. It would allow to do more flexible thinks. Checkpoint reqs will be related to trigger, not counter. It will allow us to be more flexible. Checkpoint will be fulfilled, when value of counter is inside min and max.
Reply To lachu
I think we should move checkpoint onto separated entity, called trigger, which will contains counter, description, min and max fields. It would allow to do more flexible thinks. Checkpoint reqs will be related to trigger, not counter. It will allow us to be more flexible. Checkpoint will be fulfilled, when value of counter is inside min and max.
And to do all that still within 3.2 cycle, instead of just adding the description field as I proposed?
Reply To cazfi
If we're going to show known counter values to the user on the client side, we should provide also longer explanation for them instead of just name. This ticket is about adding such explanation to the ruleset format, and communicating it to client.
0002-OSDN-TICKET-47697-S-awomir-Lach-s-awek-lach.art.pl.patch(2KB)
0003-OSDN-TICKET-47697-S-awomir-Lach-slawek-lach.art.pl.patch(3KB)
I think second and third could be applied.
Sorry it takes so long for me to get to these. Upcoming milestones from older branches keep me quite busy.
I had a bit different thing in my mind, but going to help system already is fine too. (I was thinking something like city dialog listing current counter values and a couple of words description what those values mean)
Also splitting this to two patches probably makes merging easier (one part at a time), so let's concentrate on the ruleset/server part here (0002... patch), and create a new ticket about the client / help system changes (0003... patch)
- The server side patch seems to be missing rulesave part
- Documentation comments in rulesets and data/ruledit/comments-3.x.txt missing
- I guess loading side should not make the help text mandatory (or if it does, then it should be added to all rulesets)
Reply To cazfi
Reply To cazfi
create a new ticket about the client / help system changes (0003... patch)
-> #47804
Reply To cazfi
Sorry it takes so long for me to get to these. Upcoming milestones from older branches keep me quite busy. I had a bit different thing in my mind, but going to help system already is fine too. (I was thinking something like city dialog listing current counter values and a couple of words description what those values mean) Also splitting this to two patches probably makes merging easier (one part at a time), so let's concentrate on the ruleset/server part here (0002... patch), and create a new ticket about the client / help system changes (0003... patch) --- - The server side patch seems to be missing rulesave part
- Documentation comments in rulesets and data/ruledit/comments-3.x.txt missing
- I guess loading side should not make the help text mandatory (or if it does, then it should be added to all rulesets)
0001-OSDN-47697-S-awomir-Lach-slawek-lach.art.pl.patch(2KB)
0001-OSDN-47697-S-awomir-Lach-slawek-lach.art.pl.patch(9KB)
Reply To cazfi
Sorry it takes so long for me to get to these. Upcoming milestones from older branches keep me quite busy. I had a bit different thing in my mind, but going to help system already is fine too. (I was thinking something like city dialog listing current counter values and a couple of words description what those values mean) Also splitting this to two patches probably makes merging easier (one part at a time), so let's concentrate on the ruleset/server part here (0002... patch), and create a new ticket about the client / help system changes (0003... patch) --- - The server side patch seems to be missing rulesave part
- Documentation comments in rulesets and data/ruledit/comments-3.x.txt missing
- I guess loading side should not make the help text mandatory (or if it does, then it should be added to all rulesets)
You are missing part of city dialog. If this ticket should only contains server-side patches, we lost track about client-side/city-dialog changes.
I add new ticket: #47887 for client-side (already only gtk3.22 client) implementation of checking counter value/ruleset-information for city.
Reply To lachu
You are missing part of city dialog. If this ticket should only contains server-side patches, we lost track about client-side/city-dialog changes.
There was #47804 split earlier, about help system changes. But it's ok if #47887 is about city dialog changes. Those two are quite distinct parts of the client, so deserve separate tickets.
- Seems to me that freeing (or destroying, bý strvec_destroy(), of the help texts is missing, as well as initializing them (these are related in that you don't know if non-NULL vector is to be destroyed or not, if it might be just uninitialized pointer)
Reply To cazfi
- Seems to me that freeing (or destroying, bý strvec_destroy(), of the help texts is missing, as well as initializing them (these are related in that you don't know if non-NULL vector is to be destroyed or not, if it might be just uninitialized pointer)
2023-04-23 21:24 Updated by: lachu
+void counters_free(void);
It's already declared in counters.h.
void counters_init(void)
{
+ if (0 < game.control.num_counters) {
+ counters_free();
+ }
I'm not sure if game.control.num_counters is initialized at all when _init() gets called the very first time. Also, I think counters follow (or they should follow) the common pattern that there's always matching _free() call for each _init(), so this kind of code should never be needed. One could make it an assert, if one suspects that _free() might have not been called properly: "fc_assert(game.control.num_counters == 0);"
Reply To cazfi
{{{ +void counters_free(void); }}} It's already declared in counters.h. --- {{{ void counters_init(void) { + if (0 < game.control.num_counters) { + counters_free(); + } }}} I'm not sure if game.control.num_counters is initialized at all when _init() gets called the very first time. Also, I think counters follow (or they should follow) the common pattern that there's always matching _free() call for each _init(), so this kind of code should never be needed. One could make it an assert, if one suspects that _free() might have not been called properly: "fc_assert(game.control.num_counters == 0);"
The code seems fine now -> adding to autogame test runners already.
On the documentation part, I noticed that there's "\n\" in the end of lines also in the actual rulesets. Those are needed in comments.txt to generate ruleset files, but not in the final rulesets (they have actual newlines in place already). Sorry for not noticing this in the earlier review.
Ruleup for S3_2 sandbox ruleset segfaults on main branch.
Program received signal SIGSEGV, Segmentation fault.
strvec_size (psv=0x0) at ../../../src/utility/string_vector.c:345
345 return psv->size;
(gdb) bt
#0 strvec_size (psv=0x0) at ../../../src/utility/string_vector.c:345
#1 0x000055555587951e in save_game_ruleset (
filename=filename@entry=0x7fffffffd030 "sandbox.ruleup/game.ruleset",
name=name@entry=0x555555bc3169 <game+233> "Sandbox ruleset")
at ../../../../src/tools/ruleutil/rulesave.c:1815
#2 0x000055555587affb in save_ruleset (path=path@entry=0x7fffffffd680 "sandbox.ruleup",
name=0x555555bc3169 <game+233> "Sandbox ruleset", data=data@entry=0x7fffffffd670)
at ../../../../src/tools/ruleutil/rulesave.c:3394
#3 0x000055555559b232 in main (argc=<optimized out>, argv=<optimized out>)
at ../../../src/tools/ruleup.c:226
Reply To cazfi
Ruleup for S3_2 sandbox ruleset segfaults on main branch. {{{ Program received signal SIGSEGV, Segmentation fault. strvec_size (psv=0x0) at ../../../src/utility/string_vector.c:345 345 return psv->size; (gdb) bt #0 strvec_size (psv=0x0) at ../../../src/utility/string_vector.c:345 #1 0x000055555587951e in save_game_ruleset ( filename=filename@entry=0x7fffffffd030 "sandbox.ruleup/game.ruleset", name=name@entry=0x555555bc3169 <game+233> "Sandbox ruleset") at ../../../../src/tools/ruleutil/rulesave.c:1815 #2 0x000055555587affb in save_ruleset (path=path@entry=0x7fffffffd680 "sandbox.ruleup", name=0x555555bc3169 <game+233> "Sandbox ruleset", data=data@entry=0x7fffffffd670) at ../../../../src/tools/ruleutil/rulesave.c:3394 #3 0x000055555559b232 in main (argc=<optimized out>, argv=<optimized out>) at ../../../src/tools/ruleup.c:226 }}}
2023-04-27 22:19 Updated by: lachu
I now looks good. I do not known (really), how to test it. I do not known, which parameters do you use to invoke ruleup.
Reply To cazfi
The code seems fine now -> adding to autogame test runners already. On the documentation part, I noticed that there's "\n\" in the end of lines also in the actual rulesets. Those are needed in comments.txt to generate ruleset files, but not in the final rulesets (they have actual newlines in place already). Sorry for not noticing this in the earlier review.
Reply To lachu
I do not known (really), how to test it. I do not known, which parameters do you use to invoke ruleup.
export FREECIV_DATA_PATH="/fast/freeciv/S3_2/src/data:/fast/freeciv/main/src/data" ./fcruleup -r sandbox
If we're going to show known counter values to the user on the client side, we should provide also longer explanation for them instead of just name.
This ticket is about adding such explanation to the ruleset format, and communicating it to client.