Freeciv

Sorry, but in this patch:

https://www.hostedredmine.com/attachments/393326

I don't see any new counter_by_index definition/declaration.

See Ticket #41115 description. I guess you meant this comment to that ticket in the first place.

Hi. Can anybody test?

I see problem, which must be introduced with my earlier patch. One city do not have increased counter value (with id 125?). I think it was not saved, but only zero is saved as this counter value.

- You should iterate over counters_city (so you would get only city targeted counters), and increase all with the type COUNTER_OWNED, not just one
- Find the counters to update by the type COUNTER_OWNED, not by the name that will be ruleset defined in the future and will be different for each counter even if they have the same type
- Style: 'a++' instead of '++a' when it makes no difference

Hi. Can anybody test?

I see problem, which must be introduced with my earlier patch. One city do not have increased counter value (with id 125?). I think it was not saved, but only zero is saved as this counter value. Reply To cazfi

- You should iterate over counters_city (so you would get only city targeted counters), and increase all with the type COUNTER_OWNED, not just one

Ok. But the index in city counter's array is the same for each city, so I will iterate once. Right?

PS: Take in mind, array of "counters" in city contains only value of counter. Other information are stored in global array and it was common for each counter stored in structure, which representing city.

- Find the counters to update by the type COUNTER_OWNED, not by the name that will be ruleset defined in the future and will be different for each counter even if they have the same type
- Style: 'a++' instead of '++a' when it makes no difference

Ok.

No test machinery is done for these macros.

Reply To cazfi

- You should iterate over counters_city (so you would get only city targeted counters), and increase all with the type COUNTER_OWNED, not just one
- Find the counters to update by the type COUNTER_OWNED, not by the name that will be ruleset defined in the future and will be different for each counter even if they have the same type
- Style: 'a++' instead of '++a' when it makes no difference

I only forgot to repair coding-style. And there is still error causes first city on list has save-game (or load-game) problems

Reply To cazfi

- You should iterate over counters_city (so you would get only city targeted counters), and increase all with the type COUNTER_OWNED, not just one
- Find the counters to update by the type COUNTER_OWNED, not by the name that will be ruleset defined in the future and will be different for each counter even if they have the same type
- Style: 'a++' instead of '++a' when it makes no difference

Now it works. Problem was inside srv_main.c after I mess it there. I put code responsible for increase city counters' value inside loop, which had continue instruction for living players. Tested, but not well. I only play few turns and do save game.

It was tested in few short games. It worked!

- That won't work if there is more than one COUNTER_OWNED type counters.
- You are missing the part setting those counters to zero when the city changes owner
- The city_counters_iterate() macros are missing (I assume this patch should add them). Should also be named like that (just one 'r')

0001-Introduce-city-s-counter-iterration.patch (File ID: 8537) is the last patch (I assemble it with previous changes).

Sorry for my previous patch (I forgot about resetting counter's value). I still do not understood why there should be many counter_owned counters? I known, there is type, but I thought it was temporary solution.;

I hope now it will work.

Reply To cazfi

- That won't work if there is more than one COUNTER_OWNED type counters.
- You are missing the part setting those counters to zero when the city changes owner
- The city_counters_iterate() macros are missing (I assume this patch should add them). Should also be named like that (just one 'r')

Thanks for help, Can you read my last post?

Reading (not yet actually testing) the patch, functionality looks good now.

The city_counters_iterate() macro could be improved a bit
- Style: "struct counter * pcount;" - you are not multiplying by 'pcount' but making it a pointer, so no space -> "struct counter *pcount;"
- You should not make a separate call to counters_get_city_counters_count() on every iteration. Call it once before the loop, assign value to variable, and compare against that variable on each iteration. Note next point about naming the variable
- Having static variable name '_i' means that within one city_counters_iterate() there cannot be another city_counters_iterate() (that would cause '_i' of the inner one to shadow one of the outer one). You could make name of the variable derived from pcounts name instead; '_i_##pcount'

Reply To cazfi

Reading (not yet actually testing) the patch, functionality looks good now. The city_counters_iterate() macro could be improved a bit
- Style: "struct counter * pcount;" - you are not multiplying by 'pcount' but making it a pointer, so no space -> "struct counter *pcount;"
- You should not make a separate call to counters_get_city_counters_count() on every iteration. Call it once before the loop, assign value to variable, and compare against that variable on each iteration. Note next point about naming the variable
- Having static variable name '_i' means that within one city_counters_iterate() there cannot be another city_counters_iterate() (that would cause '_i' of the inner one to shadow one of the outer one). You could make name of the variable derived from pcounts name instead; '_i_##pcount'

Done. Should I wrote some automatic tests? I test it by comparing counter section of two savegames made on the same turn. First before conquering two cities and second after conquering it.

The new '_ccounter_count' variable has the same shadowing issue that '_i' had earlier -> make it '_ccounter_count_##pcount'