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 #876776

Counters' primary structures.

Added by Sławomir Lach over 1 year ago. Updated 9 months ago.

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

0%

Estimated time:

Description

This feature/ticket are open to polished counters struct/basic routines.

0001-Added-counter-s-base-definitions.patch (4.97 KB) 0001-Added-counter-s-base-definitions.patch Sławomir Lach, 2020-06-06 02:29 PM
0001-Added-counter-s-base-definitions.patch (5.05 KB) 0001-Added-counter-s-base-definitions.patch Sławomir Lach, 2020-06-10 03:14 PM
0003-Added-base-definitions-of-counters.patch (5.18 KB) 0003-Added-base-definitions-of-counters.patch Sławomir Lach, 2020-06-10 06:32 PM
0001-Added-counter-s-base-definitions.patch (5.25 KB) 0001-Added-counter-s-base-definitions.patch Sławomir Lach, 2020-06-10 06:32 PM
0002-Revert-dai_incident-assert-that-the-scope-is-handled.patch (959 Bytes) 0002-Revert-dai_incident-assert-that-the-scope-is-handled.patch Sławomir Lach, 2020-06-10 06:32 PM
0001-Define-counters-data-types.patch (7.39 KB) 0001-Define-counters-data-types.patch Sławomir Lach, 2020-06-16 02:45 PM
0001-Define-counters-data-types.patch (10.6 KB) 0001-Define-counters-data-types.patch Sławomir Lach, 2020-06-16 04:22 PM
autogame.bash (238 Bytes) autogame.bash Sveinung Kvilhaugsvik, 2020-06-16 05:21 PM
autogame_default.serv (208 Bytes) autogame_default.serv Sveinung Kvilhaugsvik, 2020-06-16 05:21 PM
rulesets_autogame.bash (516 Bytes) rulesets_autogame.bash Sveinung Kvilhaugsvik, 2020-06-16 05:21 PM
0002-Counters-test-patch.patch (9.53 KB) 0002-Counters-test-patch.patch Sveinung Kvilhaugsvik, 2020-06-16 07:01 PM
0001-Define-counters-data-types-Define-routines-to-obtain.patch (15.6 KB) 0001-Define-counters-data-types-Define-routines-to-obtain.patch Sławomir Lach, 2020-07-16 02:22 PM
0002-Apply-the-Added-test-patch.patch (8.58 KB) 0002-Apply-the-Added-test-patch.patch Changes needed to debug Sławomir Lach, 2020-11-06 03:51 PM
0001-Apply-the-Rebase-patch.patch (14.1 KB) 0001-Apply-the-Rebase-patch.patch Only my changes Sławomir Lach, 2020-11-06 03:51 PM
0034-Add-counters-module.patch (5.02 KB) 0034-Add-counters-module.patch Marko Lindqvist, 2021-01-07 09:41 PM
0001-Add-counters-module.patch (5.3 KB) 0001-Add-counters-module.patch Marko Lindqvist, 2021-01-08 08:04 PM
0002-Added-missing-pieces-for-counters-implementation.patch (3.65 KB) 0002-Added-missing-pieces-for-counters-implementation.patch Sławomir Lach, 2021-01-19 05:51 PM
0001-First-counters-related-patch.patch (4.75 KB) 0001-First-counters-related-patch.patch Sławomir Lach, 2021-01-19 05:51 PM
0001-Add-counters-module.patch (5.41 KB) 0001-Add-counters-module.patch Marko Lindqvist, 2021-01-24 03:03 AM

Related issues

Related to Freeciv - Feature #859061: Ruleset defined countersNew

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

History

#1 Updated by Sławomir Lach over 1 year ago

#2 Updated by Alexandro Ignatiev over 1 year ago

1) let's order enum counter_range sensibly, like unit<city<palyer<world. Some comparison operators may be simpler.
2) unitt -> unit_type or utype

#3 Updated by Sveinung Kvilhaugsvik over 1 year ago

Thank you for starting to break this up. Please break it up by functionality, not by file changed, by the order you did something or by how to reduce the number of large recompilations needed. If I apply the patch as it currently is I break Freeciv. (Try applying it, compiling and then create a CounterAboveZero requirement)

While I'm not convinced that CounterAboveZero belongs in the first patch here is some feed back on it: Don't define a struct just to contain an int.

Have you read the coding style? doc/CodingStyle See where includes belong.

Why isn't the scope "extern "C" {" closed?

Is game_counter_count supposed to be a union?

I'll get back to you with more feed back and a suggestion for how this can be broken up.

#4 Updated by Sveinung Kvilhaugsvik over 1 year ago

Sveinung Kvilhaugsvik wrote:

I'll get back to you with more feed back

Why are you including <stdbool.h>?

Why use lua in a data type name?

Why do you need something special like counter_effect to set a counter value? Isn't a normal effect good enough?

How do I get the current value of a counter for an entity? What is "int def;" in the counter struct?

What does the values in counter_type do?

Sveinung Kvilhaugsvik wrote:

a suggestion for how this can be broken up.

Let us start with just enough for me to move Airlift to be a counter.

#5 Updated by Alexandro Ignatiev over 1 year ago

I respond for some ideas that are mine or clear enough.
Sveinung Kvilhaugsvik wrote:

Sveinung Kvilhaugsvik wrote:
Why do you need something special like counter_effect to set a counter value? Isn't a normal effect good enough?

This is because of reasons you ask about in #859061:

Then, I think even on this early stage we should be ready that counter may change on more than one event

The description mentions changing counters because an action was performed. Must be flexible enough that an effect can be auto-upgraded to it.

Not only when an action was performed. Turn change is not an action, and many World-range counters are updated for it. An effect doing counter update is possible but it requires adding another parameter to is_req_active() to specify which counter is affected, that requires more code reworking and seems to me not efficient. The mechanism that sprang into my mind and to a much extent done by pan Lach is uniting counter updates with calling Lua callbacks as described in this post.

What is "int def;" in the counter struct?

A default value for all new instances.

#7 Updated by Sławomir Lach over 1 year ago

Sveinung Kvilhaugsvik wrote:

Have you read the coding style? doc/CodingStyle See where includes belong.

Many years ago, so sorry. Some project doesn't contains coding style, so I didn't find searching one for Freeciv as requirement.

Why isn't the scope "extern "C" {" closed?

Probably just my bug in code.

Is game_counter_count supposed to be a union?

I'll get back to you with more feed back and a suggestion for how this can be broken up.

No. struct game_counter_count is supposed to be game info struct part. Game info is send by server to client, when game starts and contains base ruleset definitions.

#8 Updated by Sławomir Lach over 1 year ago

Sveinung Kvilhaugsvik wrote:

Why use lua in a data type name?

Because it's related to this code:

   int i;
    intptr_t *a = get_params(psignal->nargs, args);
    for (i = 0; i < script_server_C_signal_handler_vector_size(&real_C_handlers); ++i) {

      handler = script_server_C_signal_handler_vector_get(&real_C_handlers, i);
      if (0 == fc_strcasecmp(signal_name, handler->name)) {

        handler->callback(psignal->nargs, a, psignal->arg_types, handler->data2, handler->data);
      }
    }
    free(a);


in common/scriptcore/luascript_signal.c . I added above code to common/scriptcore/luascript_signal.c, because we need to allow counters to bind they own to lua events.
>

Why do you need something special like counter_effect to set a counter value? Isn't a normal effect good enough?

Effects isn't good enough. Effects are calculated per turn or when client asks and in each invocation it's value is recalculated. You should think about counters like about culture - we must store last step and be able to recalculate it, but new value will depend of previous.

How do I get the current value of a counter for an entity? What is "int def;" in the counter struct?

It depend of kind of this entity. For player you got: player->counters[number].val . Def is, as Alexandro Ignatiev told, default value for new entity.

What does the values in counter_type do?

enum counter_type points to kind of actions, when lua_signal was fired and rest match to requirements of counter. You can thought about it like about lseek position calculation kind parameter, so we could told to set position to 1 from beginning, end (but substract instead of add) or current position.

Sveinung Kvilhaugsvik wrote:

a suggestion for how this can be broken up.

Let us start with just enough for me to move Airlift to be a counter.

#9 Updated by Sveinung Kvilhaugsvik over 1 year ago

Sławomir Lach wrote:

Sveinung Kvilhaugsvik wrote:

Have you read the coding style? doc/CodingStyle See where includes belong.

Many years ago, so sorry. Some project doesn't contains coding style, so I didn't find searching one for Freeciv as requirement.

Ill get back with more later but just a hint: if your editor has automatic coding style formatting for coding style and has the GNU coding style start the Freeciv coding style as a copy of it.

#10 Updated by Sławomir Lach over 1 year ago

I think these patches could be applied. I pulled changes from origin/master and merge it. I had small time of team work with coding, so I don't known much about git - sorry.

#11 Updated by Sławomir Lach over 1 year ago

This patch (0001-Define-counters-data-types.patch) replaces all previous patches. It adds basic data types and routines to read/write counters' values. I decided to replace previous patches to make things cleaner and more ordered - data types are connected with code, so this increase readability.
I think the API is very simple - we have separated routines for getting counter for given player/city/unit and for obtaining world counter. Each given routine takes name of counter as argument. We have also functions to set/get value of counter obtained from previously given call.

There's commit you should merge at top of it:
commit 19e44fd2a405d8b090d1f12fabb26afd2ea65ef3 (origin/master, origin/HEAD, master)
Author: Sveinung Kvilhaugsvik <>

#12 Updated by Sveinung Kvilhaugsvik over 1 year ago

Sławomir Lach wrote:

This patch (0001-Define-counters-data-types.patch) replaces all previous patches.

Than you for splitting it. I'll get back with deeper feed back later. Quick feed back from a first look:

#13 Updated by Sveinung Kvilhaugsvik over 1 year ago

Where in the header file are the function declaration for the functions you wish to export?

#14 Updated by Sławomir Lach over 1 year ago

I wish add it in future, sorry - more important is create code and I use static keyword.

#15 Updated by Sveinung Kvilhaugsvik over 1 year ago

Sławomir Lach wrote:

I wish add it in future, sorry - more important is create code and I use static keyword.

How do you test you patch? If you don't: Do you want me to create a "test patch" that you can use to test your patch?

If your git knowledge is limited I recommend putting my test patch at the bottom and then working on your stuff at the top. I can give you the exact git commands you will need.

#17 Updated by Sławomir Lach over 1 year ago

Sveinung Kvilhaugsvik wrote:

Sławomir Lach wrote:

I wish add it in future, sorry - more important is create code and I use static keyword.

How do you test you patch? If you don't: Do you want me to create a "test patch" that you can use to test your patch?

Ok. I will prepare automatic tests for my patches in future. I really not test it.

If your git knowledge is limited I recommend putting my test patch at the bottom and then working on your stuff at the top. I can give you the exact git commands you will need.

Thanks.

#18 Updated by Sveinung Kvilhaugsvik over 1 year ago

Great! I'll need functions to call to
  • define a named number (a counter) of the type int with a step of 1 for cities
  • set the value of a specific counter for a specific city from the C code
  • get the value of a specific counter for a specific city from the C code

define a named number (a counter)

I haven't found anything in your patch for this. I may be blind. Should I just invent a function name with arguments and pretend that it is there? Do you want to create and register in the same function or in two separate functions? The advantage of separation is that you can do stuff with it in ruledit etc without having to worry about removing it from the registry. The down side is that you have to manage memory. How well do you know C?

get the value of a specific counter
set the value of a specific counter

I guess this is get_counter_value() and set_counter_value() after get_counter_$range(). Using text as the main API isn't the greatest idea here. Maybe use the rule structure to identify the counter? Example:
int get_counter_value_city(const stuct counter *pcount, const struct city *pcity)
void set_counter_value_city(const stuct counter *pcount, const struct city *pcity, int value)

#19 Updated by Sławomir Lach over 1 year ago

Sveinung Kvilhaugsvik wrote:

Great! I'll need functions to call to
  • define a named number (a counter) of the type int with a step of 1 for cities
  • set the value of a specific counter for a specific city from the C code
  • get the value of a specific counter for a specific city from the C code

define a named number (a counter)

I haven't found anything in your patch for this. I may be blind.

You aren't blind. Just I don't understood what you asking for, sorry. I have English knowledge problems. So I will add functions to registry and add. Freeciv have fc_realloc and relloc is first think, but probably bad, because it's reinventing well. Freeciv supports spec_vector, so I will use this mechanism, obtain real usage at ruleset load ends and set size, so it automatically call realloc. And you could also register counter during game is running. Another approach is to check how many counters we have in ruleset and use C malloc, but then I must add possibility to register new counters during gameplay. There will be need to do so? I think no - we will register some builtin counters on ruleset loading (when counters.ruleset was loaded). If that's true, I will check how many counters *.ini section exists in file, add some fixed number and do malloc. But maybe there's need to register counter at game working, because you did mention about accessing counters from lua.

Should I just invent a function name with arguments and pretend that it is there? Do you want to create and register in the same function or in two separate functions? The advantage of separation is that you can do stuff with it in ruledit etc without having to worry about removing it from the registry. The down side is that you have to manage memory. How well do you know C?

Don't waste your time. I have some C knowledge, known some dynamic data structures (stack, tree, single-linked list, two-way linked list, etc.) . I will have fun and learn, but If you get frustrated and think you waste more time on correct my mistakes, then you can do this. I will test my code and read it more times, read this discussion to don't sending mess anymore. Sorry!

get the value of a specific counter
set the value of a specific counter

I guess this is get_counter_value() and set_counter_value() after get_counter_$range(). Using text as the main API isn't the greatest idea here. Maybe use the rule structure to identify the counter? Example:
int get_counter_value_city(const stuct counter *pcount, const struct city *pcity)
void set_counter_value_city(const stuct counter *pcount, const struct city *pcity, int value)

I think this could works, because it allows us move entities in memory, but we really need this? Counters description arrays are still fixed, so maybe it could works. If so, I will need only pcount - city_counters to get index and return (for example) pcity->counters[index].

#20 Updated by Sveinung Kvilhaugsvik over 1 year ago

Sławomir Lach wrote:

Ok. I will prepare automatic tests for my patches in future. I really not test it.

Just to make sure we understand each other: I'm not talking about unit testing here.

The first steps can be done before my test patch is ready.

Instructions:
You'll need a folder with the Freeciv source code checked out from git and a build directory. You can use the same folder for both. You can use a different Freeciv git clone than the one you develop in if you don't know how to find you way back to your work. I'll assume that you have a new clone of Freeciv. I assume that you use the scripts I attached.

git checkout origin/master
git log # to make sure you are on origin/master
mkdir build_test_freeciv && cd build_test_freeciv # could be the same folder that you have the git stuff in
../autogen.sh --disable-client --disable-fcmp --disable-ruledit --enable-testmatic=yes --enable-debug=yes && make

./rulesets_autogame.bash # or do the same thing they do.
  1. ./rulesets_autogame.bash civ2civ3 ## if you computer is slow.

You should now get save games from vanilla Freeciv to compare to the save game from your changes and my test patch. This takes time. I suggest running it while you sleep or are away from your computer.

I'm not claiming the bundled scripts are high quality but it gets the save game generation done. Change "/set endturn 1001" to "set endturn 101" for a quicker test.

You could also do the same stuff the scripts do if you, like I, don't like running random scripts from the internet.

I haven't tested these instructions. Let me know if they give you trouble.

#21 Updated by Sveinung Kvilhaugsvik over 1 year ago

Sławomir Lach wrote:

You aren't blind. Just I don't understood what you asking for, sorry. I have English knowledge problems. So I will add functions to registry and add.

Maybe just give a pointer to a static array. See action_by_number(). Actions are initialized on ruleset load via actions_init(). See action_new(). By "register" i mean to tell you system that the (rule object) counter now is there.

Another way of doing it is action_enabler_new(), action_enabler_free() and action_enabler_add().

If you get frustrated and think you waste more time on correct my mistakes, then you can do this.

Thank you. Things are OK at the moment.

#22 Updated by Sveinung Kvilhaugsvik over 1 year ago

Sławomir Lach wrote:

I think this could works, because it allows us move entities in memory, but we really need this? Counters description arrays are still fixed, so maybe it could works. If so, I will need only pcount - city_counters to get index and return (for example) pcity->counters[index].

A counter number rather than a structure works fine too. But beware! One of the reasons why actions aren't generalized yet is that I ended up hard coding action ID's everywhere to replace other hard coded stuff. (I am slowly making progress on separating action id from action result. They are even different enums now)

#23 Updated by Sławomir Lach over 1 year ago

@Sveinung Kvilhaugsvik:
You did mention about lua support. If one entity (not a class - each player for ex.) could have some counter, then these prototypes could conduit to bad code.
int get_counter_value_city(const stuct counter *pcount, const struct city *pcity)
void set_counter_value_city(const stuct counter *pcount, const struct city *pcity, int value)
Then better stay in what I proposed, but I think scripter could only define counter (description of course) for class of objects, such as each player, each city, each unit, etc.

#24 Updated by Sveinung Kvilhaugsvik over 1 year ago

I don't understand. Defining counters should not be done from Lua. Reading and writing a counter value for an entity may get Lua support. I'll use airlift capacity as an example: The definition of the airlift counter (the rule object) is one. Airlift is a city counter. Each city (entity) has their own counter value.

#25 Updated by Sveinung Kvilhaugsvik over 1 year ago

So: Lua may get the ability to read from and write to a city's airlift counter value. But it can't define Airlift_Landings. That should eventually be done in non Lua ruleset files.

Are we understanding each other?

#26 Updated by Sławomir Lach over 1 year ago

Sveinung Kvilhaugsvik wrote:

I don't understand. Defining counters should not be done from Lua. Reading and writing a counter value for an entity may get Lua support. I'll use airlift capacity as an example: The definition of the airlift counter (the rule object) is one. Airlift is a city counter. Each city (entity) has their own counter value.

Ok. Thanks. That makes sense. Defining new counter descriptor (name/restrictions/events) is not necessary from lua, because we have requirements list and lua event handler. Some pieces of concept are still missing from my head.

#27 Updated by Sławomir Lach over 1 year ago

Lua event handler doesn't mean function written by lua, but actions bind to lua events in counters.ruleset file.

#28 Updated by Sveinung Kvilhaugsvik over 1 year ago

I'm able to run my autogame for civ2civ3 for 100 turns with this without any difference using a dummy implementation.

Note that this patch makes Freeciv compile your patch. This will reveal a lot of errors.

#29 Updated by Sveinung Kvilhaugsvik over 1 year ago

Instructions for using the test patch:
1) checkout freeciv's master as a new git branch
git checkout origin/master -b counters_tester
2) apply my patch
git am /where/you/stored/0002-Counters-test-patch.patch
3) apply your own patch on top
git am /where/you/stored/0001-Define-counters-data-types.patch

Then build it for auto games with the same configuration as for regular Freeciv and run the auto games the same way.

You will have to fix errors that causes your code to not compile. You will have to implement the expected functions. Then the next step is bug hunting.

#30 Updated by Sveinung Kvilhaugsvik over 1 year ago

A tool I find useful for comparing auto game results is kdiff3

#31 Updated by Sveinung Kvilhaugsvik over 1 year ago

Once you stuff compiles:

Connect a client to a server with test patch. Do an airlift. Was airlift decreased as expected? Did the server crash?

When you get a crash:
FREECIV_DATA_PATH=".:data:../data" gdb --eval-command=run --args ./server/freeciv-server
provoke the crash and
bt full

#32 Updated by Sveinung Kvilhaugsvik over 1 year ago

Did I forget to mention running Valgrind for finding memory leaks?

Sławomir Lach wrote:

Don't waste your time. I have some C knowledge, known some dynamic data structures (stack, tree, single-linked list, two-way linked list, etc.) . I will have fun and learn, but If you get frustrated and think you waste more time on correct my mistakes, then you can do this. I will test my code and read it more times, read this discussion to don't sending mess anymore. Sorry!

To reciprocate: If you feel the initial patch is overwhelming we can split the work differently. I can probably do the initial support in a day. If you feel Feature #859052 is overwhelming I can complete the unfinished implementation I have. You could still do some follow up task like the move to the ruleset.

#33 Updated by Sveinung Kvilhaugsvik over 1 year ago

Sveinung Kvilhaugsvik wrote:

To reciprocate: If you feel the initial patch is overwhelming we can split the work differently.

Just brain storming here. Another possible way of splitting it: I could try to get a dummy counters module accepted. That means that I would take care of adding the module, making sure the header is OK (no broken C++ etc), documenting the functions I'll need and integrating the various hooks. The dummy would only do what I use it for. So only one counter of each entity type until your work is done. I could then make use of it in follow up patches to move existing counters to the new implementation. You would then have test patches already integrated in Freeciv, English documentation comments written for some of the functions and an already integrated module. You could then do this patch on top of that.

#34 Updated by Sławomir Lach over 1 year ago

RE UP:
Yes. I use KDIFF3. Only difference is in phase field of diplomacy message in one turn. It is normal? Game working, but there's warning/error about action. I must check, what exactly the message was. I submit current, squashed patch. Send archives with savegames?

PS: Sorry you must wait so long and now I understood my previous behavior - complaining my patches wasn't reviewed. I have a lot of different task in work.

#35 Updated by Sveinung Kvilhaugsvik over 1 year ago

Sławomir Lach wrote:

Only difference is in phase field of diplomacy message in one turn. It is normal?

Yes.

Game working, but there's warning/error about action. I must check, what exactly the message was.

Please do.

Send archives with savegames?

Not needed if I manage to have a deep look before tomorrow while I'm still on my fast computer. Will probably be needed if I'll look the next weeks after that.

PS: Sorry you must wait so long and now I understood my previous behavior - complaining my patches wasn't reviewed.

You are forgiven.

#36 Updated by Sławomir Lach over 1 year ago

I attached console output:

1: in action_hard_reqs_actor() [../../common/actions.c::2607]: assertion 'actor_city != ((void *)0)' failed.
>
1: Please report this message at https://www.hostedredmine.com/projects/freeciv

#37 Updated by Sveinung Kvilhaugsvik over 1 year ago

Sławomir Lach wrote:

I attached console output:

Thank you. This error message has nothing to do with this patch. It is already fixed in master.

#40 Updated by Sławomir Lach about 1 year ago

I wrote this post, because maybe not each developer get login daily. I send private message to Sveinung Kvilhaugsvik. The message was: should I post savegames here?

#41 Updated by Sveinung Kvilhaugsvik about 1 year ago

Sławomir Lach wrote:

I wrote this post, because maybe not each developer get login daily. I send private message to Sveinung Kvilhaugsvik. The message was: should I post savegames here?

No need. I tried to reply in a private message. It didn't work.

#42 Updated by Sławomir Lach about 1 year ago

Sveinung Kvilhaugsvik wrote:

Sławomir Lach wrote:

I wrote this post, because maybe not each developer get login daily. I send private message to Sveinung Kvilhaugsvik. The message was: should I post savegames here?

I tried to reply in a private message. It didn't work.

My patch didn't apply, Freeciv with my patch does not compile or only response for private message won't work. If prior or second case, could you told me more?

#43 Updated by Sławomir Lach about 1 year ago

I must ask a question. Do you expect my patch implements each features? I thought I must implement in iterative approach, where each part implement small set of features. First patch allows to add airlift counter and don't break game (program may not crash).

#44 Updated by Alexandro Ignatiev about 1 year ago

Likely, it's what was said by Sveinung before, and how patches should go in general.

#45 Updated by Sławomir Lach 12 months ago

Should I cut changes? I thought my patches are self-containing.

I send two patches and only it (these patches) you should apply.

I tested. It apply, compile and I was able to play few turns. Tomorrow I will ran self-test procedure, given by Sveinung Kvilhaugsvik's.

#46 Updated by Sławomir Lach 11 months ago

My latest patches uses 895b5b5e67a9a02936936af01fb40814b683fe92 as base.

commit 895b5b5e67a9a02936936af01fb40814b683fe92
Author: Marko Lindqvist <cazfi74@gmail.com>
Date:   Mon Oct 19 04:51:13 2020 +0300

    savegame3.c: Clean up unit changed_from_tgt loading

    Remove unnecessary compatibility code. Savegame3.c saves have never
    been saved with the outdated format that it was protecting against.

    See hrm Feature #824073

    Signed-off-by: Marko Lindqvist <cazfi74@gmail.com>

#47 Updated by Marko Lindqvist 10 months ago

  • Category set to General

As Sveinung seems to be away at the moment, and this seems to be quite stuck as a result, I'll try to help a bit. I hope Sveinung can still take main responsibility of integrating counters stuff when he gets back.

Do I understand correctly that this ticket is the first of the counters related changes, and the current patch to look at is 0001-Apply-the-Rebase-patch.patch ?

#48 Updated by Sławomir Lach 10 months ago

Marko Lindqvist wrote:

Do I understand correctly that this ticket is the first of the counters related changes, and the current patch to look at is 0001-Apply-the-Rebase-patch.patch ?

You understood correctly. Another tickets is created for implementations, which were rejected. 0001-Apply-the-Rebase-patch.patch is my patch. Another patch, added to the same post is test patch, which isn't my creation.

#49 Updated by Marko Lindqvist 10 months ago

Sławomir Lach wrote:

Another tickets is created for implementations

What's the ticket number?

#50 Updated by Sławomir Lach 10 months ago

#859061

I was trying to implement the feaure in given ticket, but Sveinung Kvilhaugsvik rejected it and asks me to split/join my patches and reject some assumptions. I decided to start first sub-task in current ticket.

#51 Updated by Marko Lindqvist 10 months ago

Can you make one more iteration of checking this patch's coding style against doc/CodingStyle document yourself? I think there's still too many style issues for me to point out each one individually.
Once I get familiar with the concept, I can create small iterative tickets for next steps.

#52 Updated by Sławomir Lach 10 months ago

Yes.

#53 Updated by Sławomir Lach 10 months ago

Could you provide link to some internet resource (or suggest a book available in Poland) about K&R style? I known, that K&R are names of authors of first unofficial C documentation (it was book if I remember), but I never read this material.

#54 Updated by Marko Lindqvist 10 months ago

For the freeciv style, doc/CodingStyle should have everything you need to know.

I now have quite a list of ideas of steps forward. The main problem is that counters are rather a big feature. As such I think it should be targeted to freeciv-3.2 instead of trying to squeeze it in to 3.1 at this point when I hope to branch S3_1 in a couple of months. It might be that we can't start merging stuff before S3_1 has been branched and master has gotten open for 3.2 development. I'll think this a bit more.

#55 Updated by Marko Lindqvist 10 months ago

I decided to write my own proposal for the initial counters module from scratch, to start iterative process of adding features to.

#56 Updated by Marko Lindqvist 10 months ago

For new tickets we probably should switch to using osdn tickets.

Needed next: https://osdn.net/projects/freeciv/ticket/41115

#57 Updated by Sławomir Lach 10 months ago

I don't fully understand what you're wrote.
We have counter type field, global index and size of global counter table set to COUNTER_COUNT.
As I look at this patch, there's redundant items.

#58 Updated by Marko Lindqvist 10 months ago

Sławomir Lach wrote:

I don't fully understand what you're wrote.
We have counter type field, global index and size of global counter table set to COUNTER_COUNT.
As I look at this patch, there's redundant items.

Yeah, it was just convenient to set size of the table to COUNTER_COUNT as there currently is exactly one counter of each type, but it's not necessarily semantically correct thing to do. We would introduce something like MAX_COUNTERS in a few patches in to the series (latest in osdn #41122 which require MAX_COUNTERS > COUNTER_COUNT)
Would you find it better if I did something like the following in this patch already:

/* Currently space for one counter of each type */
#define MAX_COUNTERS COUNTER_COUNT
...
static struct counter *counters_city[MAX_COUNTERS];

#59 Updated by Marko Lindqvist 10 months ago

Marko Lindqvist wrote:

/* Currently space for one counter of each type */
#define MAX_COUNTERS COUNTER_COUNT
...
static struct counter *counters_city[MAX_COUNTERS];

This version does that, and implements a bit more initializations in counters_init().

#60 Updated by Sławomir Lach 9 months ago

Ok. What means owned in relation to counter? Probably not property/private. So what?

I think, there should be city in place of owned.

#61 Updated by Marko Lindqvist 9 months ago

Sławomir Lach wrote:

Ok. What means owned in relation to counter?

It's the first (hardcoded) counter that tells how long the city has been owned by its current owner. Included here only because we can't have enum with zero values. It gets implemented in https://osdn.net/projects/freeciv/ticket/41118

#62 Updated by Sławomir Lach 9 months ago

https://osdn.net/projects/freeciv/ticket/41115

Is partially implemented. I must ask counter_by_id and counter_id should be related to internal (in structure) id field or to index. Also, I must add all needed assertions.
I think routines given above should look for internal id, because we will made possible to add/remove counters by lua script.

#64 Updated by Marko Lindqvist 9 months ago

Sławomir Lach wrote:

https://osdn.net/projects/freeciv/ticket/41115

Please comment and submit patches related to it to that ticket (you probably need to register to osdn)

#65 Updated by Alexandro Ignatiev 9 months ago

Sławomir Lach wrote:

Ok. No response.

I think you could look at current state.

Do the functions need some assertion that NULL is not passed?

#66 Updated by Sławomir Lach 9 months ago

They don't. As I wrote, I must added lines with fc_assert.

#67 Updated by Marko Lindqvist 9 months ago

- Added enum counter_target, currently always CTGT_CITY

#68 Updated by Marko Lindqvist 9 months ago

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

Also available in: Atom PDF