Tíquete #41115

Counter getter functions

: 2021-01-08 05:46 Última Atualização: 2021-04-09 23:31

Relator:
Dono:
Tipo:
Estado:
Aberto [Owner assigned]
Componente:
Marcos:
Prioridade:
5 - Medium
Gravidade:
5 - Medium
Resolução:
Nenhum
Arquivo:
14

Details

These functions will be needed for building anything on counters module:

struct counter *counter_by_id(int id)

int counter_id(struct counter *pcount)

struct counter *counter_by_rule_name(const char *name)

const char *counter_rule_name(struct counter *pcount)

int counter_index(struct counter *pcount)

struct counter *counter_by_index(int index, enum counter_target target)

Ticket History (3/62 Histories)

2021-01-08 05:46 Updated by: cazfi
  • New Ticket "Counter getter functions" created
2021-01-08 05:51 Updated by: cazfi
  • Details Updated
  • Marco Update from (Nenhum) to 3.2.0
2021-01-08 05:55 Updated by: cazfi
  • Tipo Update from Bugs to Patches
2021-01-08 06:09 Updated by: cazfi
Comentário

I created metaticket listing all counters related work in osdn: Ticket #41119

2021-01-20 23:07 Updated by: lachu
  • File 0001-First-counters-related-patch.patch (File ID: 5923) is attached
2021-01-20 23:07 Updated by: lachu
Comentário
(This comment has been deleted)
2021-01-20 23:08 Updated by: lachu
  • File 0001-First-counters-related-patch.patch (File ID: 5923) is deleted
2021-01-20 23:09 Updated by: lachu
  • File 0001-Add-counters-module.patch (File ID: 5925) is attached
2021-01-20 23:11 Updated by: lachu
  • File 0001-Add-counters-module.patch (File ID: 5925) is deleted
2021-01-20 23:12 Updated by: lachu
Comentário

Sorry for messing in this thread. I send 0002-Added-missing-pieces-for-counters-implementation.patch, which implements very basics.

2021-01-23 12:02 Updated by: cazfi
Comentário

- counter.index is documented as "/* index in specific (city/player/world) array */" - your patch assumes it to be index in global array (actually; 'id' is an index in global array)

. The first "/***..." lines of function headers are too short and lack "*//**" required by doxygen in the end

- When both work equally well, prefer 'i++' over '++i'

- "fc_casestrcmp(name,countersi .rule_name))" -> "fc_casestrcmp(name, countersi.rule_name))" (one space added, another removed)

(Edited, 2021-01-23 12:02 Updated by: cazfi)
2021-01-23 19:23 Updated by: lachu
Comentário

I don't known how to distinguish array of index is related. I only copy your's function prototypes. I read some book about C in past and it explains you cannot substract two pointers if it don't belongs to the same array. Behavior of C language in this case is unspecified.

2021-01-24 11:07 Updated by: cazfi
  • Details Updated
Comentário

New version of https://www.hostedredmine.com/issues/876776 adds enum counter_target, and I updated counter_by_index() prototype here.

2021-01-30 20:01 Updated by: cazfi
Comentário

Reply To cazfi

New version of https://www.hostedredmine.com/issues/876776 adds enum counter_target, and I updated counter_by_index() prototype here.

Will you make a new version based on that?

2021-02-03 01:48 Updated by: None
Comentário

Yes. In this week (I think on weekend), I should use more time on programming.

2021-02-04 01:32 Updated by: lachu
Comentário

I added the work related to add counter_type above my work (git apply won't apply patch, so I made this by hand). It makes sense, because counter_by_index was changes to accept target of counter (for example).

2021-02-06 20:35 Updated by: cazfi
Comentário

Can you make this one patch that applies on top of current git HEAD?

2021-02-07 22:08 Updated by: lachu
Comentário

I don't known VCS (like git) much. Any suggestions? I will search on net, but if you can sent some private message with link or do it by yourselves?

(Edited, 2021-02-07 22:09 Updated by: lachu)
2021-02-08 17:39 Updated by: kvilhaugsvik
Comentário

Reply To lachu

I don't known VCS (like git) much. Any suggestions? I will search on net, but if you can sent some private message with link or do it by yourselves?

How I do it:

For small deviations: git checkout branch_to_put_it_on git am /tmp/patch_name.patch patch -p1 < /tmp/patch_name.patch # handle what wasn't merge by hand git add filename.c # as you are done git am --continue

For medium deviations: git checkout branch_to_put_it_on git am /tmp/patch_name.patch patch -mp1 < /tmp/patch_name.patch # tries to merge # handle what wasn't merge by hand git add filename.c # as you are done git am --continue

For large deviations: git checkout branch_to_put_it_on git am /tmp/patch_name.patch cp /the/files/that/wont/merge.c the/files/in/this/branch.c # a lot of manual porting in this step git add filename.c # as you are done git am --continue

I probably have some spelling eror in my commands so research them your self before you use them.

2021-02-11 01:11 Updated by: lachu
Comentário

I switch to master, apply changes (without 5 patch, because 5. patch was applied on master) and squash changes.

2021-02-14 19:16 Updated by: lachu
Comentário

So, Prepare patches related to next ticket meanwhile you test this changes?

2021-02-14 20:46 Updated by: cazfi
Comentário

- counter_by_index() is missing the target parameter. Did you forgot to squash in your latest changes?

- Add comment telling the directory "/* utility */" before fcintl.h include

- Many of the asserts seem reversed

You can work on multiple tickets at the same time. Only you need to be able to address issues found in earlier tickets, so you need to learn how to use git. Some instructions in http://www.freeciv.org/wiki/How_to_Contribute#How_to_create_patch_file_with_Git

(Edited, 2021-02-14 20:47 Updated by: cazfi)
2021-02-14 22:49 Updated by: lachu
Comentário

Start adding patches from 0001-All-changes-listed-below-are-related-to-counters-imp.patch to 0003-Added-missing-reference-to-utility-in-counters.c-fil.patch .

2021-02-20 23:47 Updated by: cazfi
Comentário

Can Sveinung take over as the maintainer handling the counters work? My hands are full with the preparations of the 3.0.0-beta1 release.

2021-02-21 01:39 Updated by: kvilhaugsvik
  • Dono Update from (Nenhum) to kvilhaugsvik
2021-02-21 01:52 Updated by: kvilhaugsvik
Comentário

lachu: https://git-scm.com/book/en/v2 is a great resource for learning git

2021-02-23 00:25 Updated by: kvilhaugsvik
Comentário

Superficial feed back just so you know that I have started: 1) I assume that 0001-All-changes-listed-below-are-related-to-counters-imp.patch 0002-Assertion-repair.patch and 0003-Added-missing-reference-to-utility-in-counters.c-fil.patch is supposed to be one patch. Squash them. (See git rebase --help You want git rebase -i and then using f or s to merge a patch into the patch above it) 2) Line length. See the coding style.

2021-03-01 01:29 Updated by: lachu
Comentário

Last patch (looking at bottom) is the latest version. I squash commits and repair too-long-line bug.

2021-03-12 00:09 Updated by: kvilhaugsvik
Comentário

Sending the feed back I already have written. I hope to implement something on top of your patch to test it.

Git gives me white space warning when I apply your patch. Don't add lines that are 100% white space.

Why the blank line at the start of a block? if (TRUE) {

code;

}

"After variables are declared, there should be an empty line before the rest of the function body."

Read the coding style. If your text editor formats code for you set it to GNU. It is close enough to Freeciv's style to make coding style fixes less work.

I will need a better commit message when I merge this. Do you want me to write it so you have an example to copy for your next patch?

2021-03-12 03:34 Updated by: kvilhaugsvik
Comentário

Reply To kvilhaugsvik

Sending the feed back I already have written. I hope to implement something on top of your patch to test it.

For that I need the ability to put a counter value in a city. I think that is #41116 Have you done it? If not: do you want me to do it? I'm fine with doing it myself. I'm also fine with you doing it before this patch is committed.

I will need a better commit message when I merge this. Do you want me to write it so you have an example to copy for your next patch?

My offer to write it for you was probably a mistake. You need to learn. Guidelines: The first line should be short and describe what you patch does. A soft limit is 50 chars lonng. People see this - not the full message - when they consider if they need to read the patch. The first line should be followed by a blank line, followed by a description. The description should be closer to "why" than "how". It can mention why a particular way of achieving the "why" was selected. It should contain enough "how" to let me know if something is wrong. The last line is See tracker #issuenumber so here See osdn #41115

Oh, and a warning: You are playing on a higher difficulty level than I believed you were. I found some old notes. I had the idea about counters when looking at how to make Airlift enabler controlled, something that was done in time for 3.0. This means that I have been working on my own idea of counters, forgotten about it, gotten the "new idea" again, worked on it some more and then again forgotten it - for years. This makes me a lot less open minded than I imagined I would be. So try to be extremely clear about what your code is intended to do. If not I'm afraid I'll misinterpret you and substitute what you are trying to say with my own idea. I'll try to read up on the discussion you had with Marko in the issue tracker but beware that I still may get you completely wrong.

if (countersi.id == id) {

Won't countersid always be the counter with the id of i?

2021-03-12 04:04 Updated by: cazfi
Comentário

Reply To kvilhaugsvik

I think that is #41116

I did a lot of thinking when setting the order of tickets listed in meta-ticket #41119 so that later tickets build on earlier ones and everything needed for proper functioning is always present (savegame handling has to come relatively early so that save+load cycle does not alter anything that matters), and not have dependencies the wrong way.

It's possible that I've missed something, but I think generally you should follow that order. And yes, #41116 would be next.

2021-03-12 04:11 Updated by: kvilhaugsvik
Comentário

Reply To cazfi

I did a lot of thinking when setting the order of tickets listed in meta-ticket #41119 so that later tickets build on earlier ones and everything needed for proper functioning is always present (savegame handling has to come relatively early so that save+load cycle does not alter anything that matters), and not have dependencies the wrong way.

Thank you for the link, Marko!

2021-03-13 20:55 Updated by: lachu
Comentário

Sorry for delay. I will resume developing in near week. If somebody would like to write some code on top of mine, it's fair. Especially savegame-related code or network code. That is two domains I'm not too good.

2021-03-15 03:56 Updated by: kvilhaugsvik
Comentário

Reply To lachu

Sorry for delay.

No problem. Remember who you are talking to. I just committed a patch that I procrastinated on completing for almost 10 years.

If somebody would like to write some code on top of mine, it's fair.

Great! The reason why I would prefer to have more than just this patch ready before I push it is that I'm not the greatest at spotting details. If I can get a patch series that forces the compiler to compile everything rather than skipping it as unused as an optimization the compiler will detect some of the details you may have gotten wrong and my testing will detect others.

2021-03-17 04:34 Updated by: lachu
Comentário

Reply To kvilhaugsvik

if (countersi.id == id) {

Won't countersid always be the counter with the id of i?

In current implementation, yes. We have only one counter type currently. It have kind/range city. Each city will contains a single value of this counter. In future, ruleset could define counters, so there could exist many counters of this kind (city range), player range, unit range, etc.

So static counters struct at top of counters.c contains only one counter currently and type do not mean kind/range. It mean number of counter in array. In future, we could remove enum counter_type and use ruleset defined counter types.

(Edited, 2021-03-17 04:39 Updated by: lachu)
2021-03-24 04:56 Updated by: lachu
Comentário

Did you receive message about update: https://osdn.net/projects/freeciv/ticket/41116 ?

2021-03-24 22:24 Updated by: kvilhaugsvik
Comentário

Reply To lachu

In current implementation, yes. We have only one counter type currently. It have kind/range city. Each city will contains a single value of this counter. In future, ruleset could define counters, so there could exist many counters of this kind (city range), player range, unit range, etc.

Exactly what is "id" supposed to mean? Counter type? Global counter id? City counter id?

2021-03-25 01:14 Updated by: cazfi
Comentário

Reply To kvilhaugsvik

Reply To lachu

In current implementation, yes. We have only one counter type currently. It have kind/range city. Each city will contains a single value of this counter. In future, ruleset could define counters, so there could exist many counters of this kind (city range), player range, unit range, etc.

Exactly what is "id" supposed to mean? Counter type? Global counter id? City counter id?

int id; /* id in global counters array */

So, global counter id. There is also "int index; /* index in specific (city/player/world) array */" and "enum counter_type type;"

2021-03-25 05:26 Updated by: kvilhaugsvik
Comentário

Reply To cazfi

So, global counter id.

That is my interpretation too but in that case it should be possible to just look it up in the array rather than looping. So I wonder what Lachu's interpretation of 'id' is.

2021-03-26 03:03 Updated by: lachu
Comentário

Creating this patch takes too long. I didn't create this structure by my own. As I now remind, there will exist separate global arrays for describing counters of city. It will contains pointers to array of all counters. Id will point the index in this main, global array.

global_array { COUNT_CITY_OWNED = { .id = 0 }, COUNT_UNIT_LIVE = { .id = 1} }

// Descriptions

city_array { &COUNT_CITY_OWNED }

unit_array { &COUNT_UNIT_LIVE }

And yet:

Unit_1 { counter_values = { 5 /* Unit Live for 5 years */ } }

City_1 { counter_values = { 5 /* City is conquered/created five turns ago */ } }

Sorry for misunderstanding.

(Edited, 2021-03-26 03:04 Updated by: lachu)
2021-03-26 17:47 Updated by: kvilhaugsvik
Comentário

Reply To lachu

Creating this patch takes too long.

I agree. A clarification to make sure it doesn't take longer: All I ask is that you optimize by looking up the counter by id in the table you can look up by global id rather than looping over it and that you fix the coding style. You can add an assertion that the counter you looked up has the id in case not checking that makes you uncomfortable.

2021-03-26 18:03 Updated by: lachu
Comentário

That is true I can return counter by index from global array :-) . I also do some code formatting fixes.

2021-03-30 09:55 Updated by: kvilhaugsvik
Comentário

One more thing: OSDN doesn't tell me when you upload a patch. I discovered this one now.

2021-04-08 21:32 Updated by: lachu
Comentário

Reply To kvilhaugsvik

Reply To kvilhaugsvik

Sending the feed back I already have written. I hope to implement something on top of your patch to test it.

For that I need the ability to put a counter value in a city. I think that is #41116 Have you done it? If not: do you want me to do it? I'm fine with doing it myself. I'm also fine with you doing it before this patch is committed.

I think 41119 is nearly ready.

2021-04-09 02:51 Updated by: kvilhaugsvik
Comentário

What patch is 0008-Code-style-changes-to-match-Freeciv-code-rules.patch supposed to apply on top of?

Do you know how to merge changes so they come in one patch?

2021-04-09 17:36 Updated by: None
Comentário

Reply To kvilhaugsvik

What patch is 0008-Code-style-changes-to-match-Freeciv-code-rules.patch supposed to apply on top of? Do you know how to merge changes so they come in one patch?\

Do you mean squashing changes? I known how to do it. I will prepare single path and send here, but firstly I will apply my changes at top master/trunk.

2021-04-09 20:12 Updated by: kvilhaugsvik
Comentário

Reply To (Anonymous)

Do you mean squashing changes?

Yes.

I known how to do it. I will prepare single path and send here, but firstly I will apply my changes at top master/trunk.

Thank you. A single patch for each issue. (One for this, one for #41116)

2021-04-09 23:31 Updated by: lachu
Comentário

You can apply my patch on latest master. You may apply one single RC- file.

Editar

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Login