Freeciv

Hi. Counter type have had default value field currently. Remove fields not mentioned here?

I mean, ticked contained fields name in description. Default is not mentioned. Should I remove this field?

Reply To lachu

Hi. Counter type have had default value field currently. Remove fields not mentioned here? I mean, ticked contained fields name in description. Default is not mentioned. Should I remove this field?

Yeah, 'default value' is something that needs to come from the ruleset, if it's to be kept. It just wasn't part of the plan when I originally opened this ticket.

I only asks, because you add to base idea checkpoint object. I think default value could be maybe removed, cause we have checkpoint.

Reply To lachu

I only asks, because you add to base idea checkpoint object. I think default value could be maybe removed, cause we have checkpoint.

Certainly, if even you now think so. You were the one who wanted it added when I saw no need for it.

Reply To cazfi

Reply To lachu

Hi. Counter type have had default value field currently. Remove fields not mentioned here? I mean, ticked contained fields name in description. Default is not mentioned. Should I remove this field?

Yeah, 'default value' is something that needs to come from the ruleset, if it's to be kept. It just wasn't part of the plan when I originally opened this ticket.

I send basic changes. I do not check code-style yet. First patch change some data types (C enum -> FC specenum). Second add code to ruleset loader routines. Third patch is only for testing purposes. It change civ2civ3 ruleset.

Reply To lachu

I send basic changes. I do not check code-style yet. First patch change some data types (C enum -> FC specenum). Second add code to ruleset loader routines. Third patch is only for testing purposes. It change civ2civ3 ruleset.

Reply To cazfi

Reply To lachu

I send basic changes. I do not check code-style yet. First patch change some data types (C enum -> FC specenum). Second add code to ruleset loader routines. Third patch is only for testing purposes. It change civ2civ3 ruleset.

We can split this ticket further, if it seems like a sensible thing to get this kind of changes in first, separately.

Ok. So I am waiting. 1. (C enum -> FC specenum) 2. add code to ruleset loader routines; patch is only for testing purposes. It change civ2civ3 ruleset.

Reply To (Anonymous)

Reply To cazfi

We can split this ticket further, if it seems like a sensible thing to get this kind of changes in first, separately.

Ok. So I am waiting.

Reply To cazfi

Reply To (Anonymous)

Reply To cazfi

We can split this ticket further, if it seems like a sensible thing to get this kind of changes in first, separately.

Ok. So I am waiting.

As this ticket has some patches already, split it so that the new ticket is about loading the values from the ruleset -> #44345 Haven't looked the patches yet - currently rather busy with the 3.0.1 release and S3_1-d3f stuff. Sorry.

I added comment for new routine.

To me the patches here seems like something that applies on top of the patch that actually should be here, but isn't. Where do you turn counter_type to a specenum? Apparently you have also renamed it as counter_behavior?

Last patch is: Change counter helper types definition

Problem was I firstly start developing whole ruleset reader (with changes in counters.ch), but realized I need to change some types, so commit changes (there were changes in counters.ch and rulesets.c). Next, I prepared two patch on top of these patches and put it there. I just forgot about this one commit, sorry. I do git clone --shared ./freeciv ./freeciv-tests, next reset --hard in freeciv-test, do git am, build and tests. All should worked.

Reply To cazfi

To me the patches here seems like something that applies on top of the patch that actually should be here, but isn't. Where do you turn counter_type to a specenum? Apparently you have also renamed it as counter_behavior?

Read message above, please. And again, sorry. That is I should always do (test if changes apply), but thought it should works.

Patch file still doesn't quite make sense. As an unrelated change you remove all functionality from counters_init() and counters array entry for, and introduce function named attach_city_counter() that is called from nowhere. Seems to me that even the existing Owned -counter functionality can't work any more with this.

- counter_type -> counter_behavior rename both makes and doesn't make sense. I have no strong feelings either way, though reserving word "behavior" for this might bite us later. Don't know if "counter_source" (thing being counted) would make sense?
- You seem to have habit of using prefix operators, while coding style says: "Use the postfix operator instead of the prefix operator when either will work. That is, write "a++" instead of "++a"." - pay attention to those.
- Update also the comments after copy-pasting code :-)
- "OWNED" as a user-facing string (not name of an macro) should not be all-capital: "Owned"
- You turn also counter_target as a specenum. Do you think there's need for specenum functionality for it coming up? I don't really know what you would need it for.

Reply To cazfi

Patch file still doesn't quite make sense. As an unrelated change you remove all functionality from counters_init() and counters array entry for, and introduce function named attach_city_counter() that is called from nowhere. Seems to me that even the existing Owned -counter functionality can't work any more with this. - counter_type -> counter_behavior rename both makes and doesn't make sense. I have no strong feelings either way, though reserving word "behavior" for this might bite us later. Don't know if "counter_source" (thing being counted) would make sense?
- You seem to have habit of using prefix operators, while coding style says: "Use the postfix operator instead of the prefix operator when either will work. That is, write "a++" instead of "++a"." - pay attention to those.
- Update also the comments after copy-pasting code :-)
- "OWNED" as a user-facing string (not name of an macro) should not be all-capital: "Owned"
- You turn also counter_target as a specenum. Do you think there's need for specenum functionality for it coming up? I don't really know what you would need it for.

About function called from nowhere - it is called from ruleset loading part ( load_ruleset_game , so it is not part of this patch. Should I move it to patch related about ruleset-loading routines?

Reply To lachu

Reply To cazfi

Patch file still doesn't quite make sense. As an unrelated change you remove all functionality from counters_init() and counters array entry for, and introduce function named attach_city_counter() that is called from nowhere. Seems to me that even the existing Owned -counter functionality can't work any more with this.

>

About function called from nowhere - it is called from ruleset loading part ( load_ruleset_game , so it is not part of this patch. Should I move it to patch related about ruleset-loading routines?

- counter_type -> counter_behavior rename both makes and doesn't make sense. I have no strong feelings either way, though reserving word "behavior" for this might bite us later. Don't know if "counter_source" (thing being counted) would make sense?

This is not source. It is for example for Owned. While nobody (I?) implemented a way to change value of counter, we need to define some behavior. And I do not suggest this. I follow somebody else suggestion and implement event-driven value update way.

- You seem to have habit of using prefix operators, while coding style says: "Use the postfix operator instead of the prefix operator when either will work. That is, write "a++" instead of "++a"." - pay attention to those.
- Update also the comments after copy-pasting code :-)
- "OWNED" as a user-facing string (not name of an macro) should not be all-capital: "Owned"
- You turn also counter_target as a specenum. Do you think there's need for specenum functionality for it coming up? I don't really know what you would need it for.

I load it from ruleset, so I need simple way to convert string into value of this type. Why duplicate C code?

Reply To lachu

Reply To cazfi

Apply: Many changes related to spilt code accross patches. I do not seen prefix/posfix notation problems in this series of patch/ticket.

1. Function to add counter to city's array moved to different ticket

2. Initialize function restored

3. Restored initial value of counters array

4. OWNED -> Owned

Reply To lachu

2. Initialize function restored

The way you change its indentation seems wrong. (Do you read your patches - or more easily; 'git diff' output - to see what you are going to submit?)

(about counter_target)

I load it from ruleset

I'm not sure if that will make sense in the future, but at least currently the target is a function of the behavior ("how many turns *city* has been owned" can't be anything but city targeted). So there's no point in exposing it to the ruleset format as if ruleset author could somehow decide what the value is - instead you are forcing him/her to set the value you already know to be the only possible one.

I do some changes.

Reply To cazfi

Reply To lachu

2. Initialize function restored

The way you change its indentation seems wrong. (Do you read your patches - or more easily; 'git diff' output - to see what you are going to submit?)

It was corrected/repaired/fixed.

(about counter_target)

I load it from ruleset

I'm not sure if that will make sense in the future, but at least currently the target is a function of the behavior ("how many turns *city* has been owned" can't be anything but city targeted). So there's no point in exposing it to the ruleset format as if ruleset author could somehow decide what the value is - instead you are forcing him/her to set the value you already know to be the only possible one.

Normal enum restored in one case. Only counter_behavior is as specenum.

I think this part break things:

-/* Space for one counter of each type */
-#define MAX_COUNTERS COUNTER_COUNT
+#define MAX_COUNTERS 20

You make all the iterations over counters, including their creation, to go up to 20. That means there's 20 identical Owned counters instead of just 1.

Reply To cazfi

I think this part break things: -/* Space for one counter of each type */
-#define MAX_COUNTERS COUNTER_COUNT
+#define MAX_COUNTERS 20
You make all the iterations over counters, including their creation, to go up to 20. That means there's 20 identical Owned counters instead of just 1.

Repaired. I also removed comments created by copy operation, which I missed.

Reply To lachu

Reply To cazfi

I think this part break things: -/* Space for one counter of each type */
-#define MAX_COUNTERS COUNTER_COUNT
+#define MAX_COUNTERS 20
You make all the iterations over counters, including their creation, to go up to 20. That means there's 20 identical Owned counters instead of just 1.

Repaired. I also removed comments created by copy operation, which I missed.

I don't see that change. The latest version still sets MAX_COUNTERS to 20. Did you send wrong file?

Reply To cazfi

I think this part break things: -/* Space for one counter of each type */
-#define MAX_COUNTERS COUNTER_COUNT
+#define MAX_COUNTERS 20
You make all the iterations over counters, including their creation, to go up to 20. That means there's 20 identical Owned counters instead of just 1.

Repaired. I also removed comments created by copy operation, which I missed. Reply To cazfi

Reply To lachu

Reply To cazfi

I think this part break things: -/* Space for one counter of each type */
-#define MAX_COUNTERS COUNTER_COUNT
+#define MAX_COUNTERS 20
You make all the iterations over counters, including their creation, to go up to 20. That means there's 20 identical Owned counters instead of just 1.

Repaired. I also removed comments created by copy operation, which I missed.

I don't see that change. The latest version still sets MAX_COUNTERS to 20. Did you send wrong file?

Sorry, but I thought changing first value of enum to 1 instead of 0 repair the problem. Now, when C initializes int variable with default value (0), it do not collide with 1.

Repaired. I also removed comments created by copy operation, which I missed. Reply To cazfi

I don't see that change. The latest version still sets MAX_COUNTERS to 20. Did you send wrong file?

Sorry, but I thought changing first value of enum to 1 instead of 0 repair the problem. Now, when C initializes int variable with default value (0), it do not collide with 1.

I don't really get what you mean by that? The problem I'm talking is that you change MAX_COUNTERS telling how many counters are supported, from COUNTER_COUNT (1) to 20, while we still support just 1 counter (of each type -> COUNTER_COUNT)

Reply To cazfi

Repaired. I also removed comments created by copy operation, which I missed. Reply To cazfi

I don't see that change. The latest version still sets MAX_COUNTERS to 20. Did you send wrong file?

Sorry, but I thought changing first value of enum to 1 instead of 0 repair the problem. Now, when C initializes int variable with default value (0), it do not collide with 1.

I don't really get what you mean by that? The problem I'm talking is that you change MAX_COUNTERS telling how many counters are supported, from COUNTER_COUNT (1) to 20, while we still support just 1 counter (of each type -> COUNTER_COUNT)

I done this little change.

Repaired. I also removed comments created by copy operation, which I missed. Reply To cazfi

I don't see that change. The latest version still sets MAX_COUNTERS to 20. Did you send wrong file?

Sorry, but I thought changing first value of enum to 1 instead of 0 repair the problem. Now, when C initializes int variable with default value (0), it do not collide with 1.

I don't really get what you mean by that? The problem I'm talking is that you change MAX_COUNTERS telling how many counters are supported, from COUNTER_COUNT (1) to 20, while we still support just 1 counter (of each type -> COUNTER_COUNT)

Reply To cazfi

Repaired. I also removed comments created by copy operation, which I missed. Reply To cazfi

I don't see that change. The latest version still sets MAX_COUNTERS to 20. Did you send wrong file?

Sorry, but I thought changing first value of enum to 1 instead of 0 repair the problem. Now, when C initializes int variable with default value (0), it do not collide with 1.

I don't really get what you mean by that? The problem I'm talking is that you change MAX_COUNTERS telling how many counters are supported, from COUNTER_COUNT (1) to 20, while we still support just 1 counter (of each type -> COUNTER_COUNT)

Hi. Did you miss this message: 2022-04-30 17:38 Updated by: lachu ? I post patch adjusted to your suggestions.