Beginning of initialization code was send.
PS: By initializing memory do you mean default value for some C type (especially 0 value for pointers/int) or default value of counter type?
This is, as you say, just a beginning.
You still introduce white space errors. Use git log -p to see them.
The counter values are *not* intended to remain server only.
Use the default value for the counter type of each defined counter.
Why not initialize earlier like in create_city_virtual()? This is a real question and not me telling you to change it in a nice way.
Why the name counter_actor_value? Actor already has a meaning - it means the entity performing an action. Could you rename it to avoid confusion?
Tip: Use git blame to see previous examples of changes to the area you work on and learn from the.
Reply To kvilhaugsvik
This is, as you say, just a beginning.
In case it isn't obvious: I need functions to access a counter's value for a city. One to set the value, one to read it. I suggest taking city and counter (or counter ID) - not counter type - as parameters. Implementing those may inspire you to switch the memory from a vector to a simple array where each counter value is stored by the id of its counter type.
Reply To kvilhaugsvik
This is, as you say, just a beginning. You still introduce white space errors. Use git log -p to see them. The counter values are *not* intended to remain server only. Use the default value for the counter type of each defined counter. Why not initialize earlier like in create_city_virtual()? This is a real question and not me telling you to change it in a nice way.
Because this is good programming approach. I thought these functions shouldn't initialize values by values related to current ruleset. Also. I decided to put counter values related to city not at highest city structure level, but on union (more strictly server-side struct of this union.). create_city_virtual are inside common directory, so I cannot initialize counters inside it, because it would write to client data on client process. But If client should known counter values, I will change my plans. I didn't known counter values should been displayed in UI. I will correct this.
Why the name counter_actor_value? Actor already has a meaning - it means the entity performing an action. Could you rename it to avoid confusion?
I really select bad name. Actor means system, user or time. Better will be entity, but I will search for better name.
Tip: Use git blame to see previous examples of changes to the area you work on and learn from the.
Ok. I correct styling of code (0009-Repair-code-style), squash changes and do some other things.
You can apply my patch on latest master. You may apply one single RC-01 file from ticket 41115 and next single RC-01 file from this thread.
White space error:
$ git am /tmp/RC-01-Added-routines-to-obtain-number-of-counters-for-give.patch Applying: - Added routines to obtain number of counters for given scope /home/sveinung/Shared/freeciv/.git/worktrees/src/rebase-apply/patch:51: trailing whitespace.
/home/sveinung/Shared/freeciv/.git/worktrees/src/rebase-apply/patch:72: trailing whitespace.
/home/sveinung/Shared/freeciv/.git/worktrees/src/rebase-apply/patch:80: trailing whitespace. void city_set_counter_value(struct city *pcity, /home/sveinung/Shared/freeciv/.git/worktrees/src/rebase-apply/patch:82: trailing whitespace. int city_read_counter_value(struct city *pcity, warning: 4 lines add whitespace errors.
Sorry, I think I repaired syntax errors.
I'm currently in the process of porting my test patch.
Line length is max 77 chars city_read_counter_value(): it would be nice if you cold declare the parameters const as you don't modify them. This allows passing const variables as arguments.
It compiles. Given that it had to build your code to compile the test patch we can conclude that you don't have errors hidden by compiler optimizations.
But it crashes on loading a game with a city because counters_get_city_counters_count() returns 3 while I only have 2 counters. You forgot to initialize number_city_counters in counters_init(). It keeps its old value. Just set it to 0.
pcity->counter_values = fc_malloc(sizeof(int) * counters_get_city_counters_count());
As you never free the memory it is a memory leak. Maybe use a regular array rather than allocating (and freeing) memory dynamically?
Reply To kvilhaugsvik
You forgot to initialize number_city_counters in counters_init(). It keeps its old value. Just set it to 0.
Corrected to: It keeps its old value. (The old value is increased on each ruleset load.)
Update: posted on the wrong patch
fc_assert_ret_val(NULL == name, NULL);
Asserts that the value is NULL. You should assert that it *isn't* NULL.
Reply To kvilhaugsvik
pcity->counter_values = fc_malloc(sizeof(int) * counters_get_city_counters_count()); As you never free the memory it is a memory leak. Maybe use a regular array rather than allocating (and freeing) memory dynamically?
Once we read the counters from the ruleset, their number will not be known at compile time, so maybe it's a good idea to have them allocated dynamically already.
Remember to set default in the hard coded counter.
{ "Owned", COUNTER_OWNED, CTGT_CITY, ...
With those issues fixed: It works! I was able to use it to implement the server side of airlift.
Reply To cazfi
Once we read the counters from the ruleset, their number will not be known at compile time, so maybe it's a good idea to have them allocated dynamically already.
Since counters_free() is already is defined and hooked up it won't be as hard for him to free the memory as I believed it would be.
Which of the patche here are really implementing the ticket? Most of them seem to do completely unrelated things, and doing so in incompatible way with the plans forward (other tickets from the metaticket)
The last patches. Those that start with RC.
I don't (at least yet) see the need for 'def' value. Why would counting start from value other than zero?
Reply To cazfi
I don't (at least yet) see the need for 'def' value. Why would counting start from value other than zero?
Counting from different value from 0 is necessary to save memory and made everything simpler. When forcing ruleset authors to declare counters with default value 0, we force they to add new event, which will change counter value - for example game start. Event contains lua event reference, requirement list, step, etc. We will lost some memory and force ruleset authors to do not trival task to set default value. Imagine, how to set default value of city, when it's created? It is only look simple, but you must listen for create_city event and check if city is build in this turn. There's probably no requirement for checking city is built in this turn, so you will introduce new counter turn_exist and step at next_turn by 1. There's a lot of complexity.
I see that *if* you want to set a default value, it's easier to have it in counter structure, but my question was when do you need to start counting from (hardcoded) value other than zero. What counter are you thinking?
Also, 'start' would be a better name for a counter start value than 'def)ault'.
Reply To cazfi
I see that *if* you want to set a default value, it's easier to have it in counter structure, but my question was when do you need to start counting from (hardcoded) value other than zero. What counter are you thinking?
For example to give happiness effect to a city, when it was built. In this case we would test counter is other than zero and counting down to 0, so at beginning it must be greater than 0.
Of course, if we don't use counter value of multiplier, we could use two counters - one counted from zero to inf and second first is greater than some value and reset in this case, so effect could check this counter is zero in req list. This requires a lot more complexity. Somebody suggest to add fire_at_value field and we can discuss about if default field or fire_at_value is better, but we must have at least one. Also, we can provide condition list (new class - not requirement list) for check counter value and add requirement item to check if condition are meet.
Please apply only RC-02. Sorry for I remove my patch from the list, but I forgot checking it apply without errors, I am lazy. I think I addressed all errors/warnings. Sorry I deliver patches, with does not apply or do not work. I am beginner and often creating single-person projects.
So, as I understood I must add memory freeing routines.
Reply To lachu
So, as I understood I must add memory freeing routines.
Umh, for what? You already free counter values in destroy_city_virtual().
- Leave city_set_counter_value() and city_read_counter_value() out from this patch. They are out of scope of the ticket, and I don't think they are ever going to appear in that form.
- city.h includes: counters.h should be (in alphabetical order) before fc_types.h
- counters_init(): As both old 'city_i' and new 'number_city_counters' variable hold exactly the same value, you should get rid of the local 'city_i' and to use 'number_city_counters' instead of it
- There seem to be some changes unrelated to this ticket that are already in. So obviously the patch needs rebasing.
I send new patch.
Looks good already. Just two things to make it perfect:
- In city_create_virtual(), assign value from counters_get_city_counters_count() to a variable before the initialization loop, and compare index variable (i) to that variable instead of calling counters_get_city_counter_count() on every round of the loop (that has function call overhead)
- In counters.h, add an empty line between 'int counters_get_city_counters_count(void);' and '#ifdef cplusplus'
git am complains about trailing whitespace. Can somebody handle this error? I open this file in kate and go to 119 line and there is no whitespace (excluding \n) at end of line.
I think the trailing whitespace was in the empty line in counter structure definition (between 'def' and 'index' lines). Should be fixed in attached version.
Allocate and initialize memory for counter values in a city structure when city is created.