I doubt it was good idea to copy counters mechanism from cities. Some counters may be not be related to some unit type/classes/etc. For example priest convert action should be related only to priest. But counters for cities implementation is quite simple, so it could be copied 1:1.
Reply To cazfi
Add initial support for unit counters. No need to implement any counter behavior in this ticket - current counters code should be flexible enough to work (iterate over) zero counter behaviors and counter instances. --- - Add counter_target type "Unit"
- Add counters_get_unit_counters_count()
- Implement unit_counters_iterate(pcount) macro (mostly by copying ciyt_counters_iterate() and modifying a bit)
- Add "counter_values" to unit structure
Savegame part likely isn't as trivial "copy from city counters implementation" -> let's leave that to another ticket.
Sorry I do not reply, but create new post. Read above post, please.
Reply To lachu
I doubt it was good idea to copy counters mechanism from cities.
Not to copy, but to use existing generic code.
I think, we should:
1. Define new range for counter - unit
2. Allow to assign unit counter to unit type
- [unit_warriors]
- name = _("Warriors")
- class = "Land"
- ;reqs
- obsolete_by = "Musketeers"
- graphic = "u.warriors"
- graphic_alt = "-"
- sound_move = "m_warriors"
- sound_move_alt = "m_generic"
- sound_fight = "f_warriors"
- sound_fight_alt = "f_generic"
- build_cost = 10
- pop_cost = 0
- attack = 1
- defense = 1
- hitpoints = 10
- firepower = 1
- move_rate = 1
- vision_radius_sq = 2
- transport_cap = 0
- fuel = 0
- uk_happy = 1
- uk_shield = 1
- uk_food = 1
- uk_gold = 1
- tp_defense = "Alight"
- flags = "Capturer", "Fortress", "FleshBased"
- roles = "DefendOk", "DefendOkStartUnit", "FirstBuild"
- helptext = _("\
- This unit may be built from the start of the game. It is the\
- weakest offensive unit.\
- ")
- counters = "Belief_spread"
3. On ruleset loading routine, load each counter name, load each unit and do sanity checks (test if each unit counter is assigned to at least one unit type - warriors have Belief_spread for example). If there exist dangling unit counter, stop loading ruleset and display error.
4. Unit type struct will contain pointers to counters definitions
5. Unit struct will contain counters' values
6. There could still exist global counters array for units type (entire set of unit types), for example for provide help text - maybe it would be necessary, but I am not sure, so 6 point maybe will be removed in future
Reply To cazfi
Add initial support for unit counters. No need to implement any counter behavior in this ticket - current counters code should be flexible enough to work (iterate over) zero counter behaviors and counter instances.
If you want to start with really minimal patch, I think you can do just
- Add counter_target type "Unit"
...and to make sure it does not break existing city counter usage.
Reply To cazfi
Reply To cazfi
Add initial support for unit counters. No need to implement any counter behavior in this ticket - current counters code should be flexible enough to work (iterate over) zero counter behaviors and counter instances.
If you want to start with really minimal patch, I think you can do just
- Add counter_target type "Unit"
...and to make sure it does not break existing city counter usage.
Ok. I see implementing in my way could cause problems, but I am not sure. I see we use index/number and one is kept in counter structure, pointing position of counter in counter-type-related-array. When implementing as I suggested, we have separate array for each unit type, so my suggestion should be well-tested/though again.
I would take this in one step at a time. I'm not convinced that we could do radical restructuring of the parts of the counters code we already implemented in 3.2 and then build complex new unit counters on top of that would finish in time for 3.3. Let's try to get the basic use-case of the same counters applying to all units done first. Besides, ruleset would need to use a lot of different unit counters for the benefits of not storing them all for all units would outweigh the overhead of figuring out which counters each unit has and where (same counter could be in different index for different units, depending what other counters they have)
Reply To cazfi
I would take this in one step at a time. I'm not convinced that we could do radical restructuring of the parts of the counters code we already implemented in 3.2 and then build complex new unit counters on top of that would finish in time for 3.3. Let's try to get the basic use-case of the same counters applying to all units done first. Besides, ruleset would need to use a lot of different unit counters for the benefits of not storing them all for all units would outweigh the overhead of figuring out which counters each unit has and where (same counter could be in different index for different units, depending what other counters they have)
Sorry it takes too long. I had stupid typo (closing parenthesis placement bug, so some values was filled only if there was no counter name).
Reply To cazfi
I would take this in one step at a time. I'm not convinced that we could do radical restructuring of the parts of the counters code we already implemented in 3.2 and then build complex new unit counters on top of that would finish in time for 3.3. Let's try to get the basic use-case of the same counters applying to all units done first. Besides, ruleset would need to use a lot of different unit counters for the benefits of not storing them all for all units would outweigh the overhead of figuring out which counters each unit has and where (same counter could be in different index for different units, depending what other counters they have)
Looking the source code (files 12039 + 12082. Is that what you mean as the change to the repo?) it looks very good already. Only now that 'target' is read from the rulesets:
- Documentation comments in rulesets & ruledit/comments-3.3.txt need updating
- Rulesave needs to save it
- I think rulecompat needs to add "City" target to all counters loaded from 3.2 rulesets
Reply To cazfi
Looking the source code (files 12039 + 12082. Is that what you mean as the change to the repo?) it looks very good already.
Yes, but in reverse order.
Reply To cazfi
Looking the source code (files 12039 + 12082. Is that what you mean as the change to the repo?) it looks very good already. Only now that 'target' is read from the rulesets: - Documentation comments in rulesets & ruledit/comments-3.3.txt need updating
- Rulesave needs to save it
- I think rulecompat needs to add "City" target to all counters loaded from 3.2 rulesets
Reply To cazfi
Looking the source code (files 12039 + 12082. Is that what you mean as the change to the repo?) it looks very good already. Only now that 'target' is read from the rulesets: - Documentation comments in rulesets & ruledit/comments-3.3.txt need updating
- Rulesave needs to save it
- I think rulecompat needs to add "City" target to all counters loaded from 3.2 rulesets
There still many things missing, like network protocol support for unit's counters, etc.
"if (info->version > RSFORMAT_3_2) {"
Use ">= RSFORMAT_3_3)" instead. Old RSFORMAT_3_2 macro might get removed soon, and when we clean compat code of the previous version away, we will be searching for RSFORMAT_3_3.
- I don't think ruledit_disabled is in any way relevant in rulecompat.
+ if (pcount->ruledit_disabled) {
+ continue;
+ }
+ pcount->target = CTGT_CITY;
Reply To cazfi
"if (info->version > RSFORMAT_3_2) {" Use ">= RSFORMAT_3_3)" instead. Old RSFORMAT_3_2 macro might get removed soon, and when we clean compat code of the previous version away, we will be searching for RSFORMAT_3_3. --- - I don't think ruledit_disabled is in any way relevant in rulecompat. {{{ + if (pcount->ruledit_disabled) { + continue; + } + pcount->target = CTGT_CITY; }}}
- Patch does not apply (any more?)
- Missing ruleset documentation about "target" field
- Following check is not strict enough. What if target was given as "blabla"? Even in compat mode, we should only accept the case where the "target" is completely missing:
if (!counter_target_is_valid(ct)) {
...
+ if (compat->version > RSFORMAT_3_2) {
+ ruleset_error(NULL, LOG_ERROR,
+ "\"%s\" unknown counter target \"%s\".",
+ filename, counter_type);
+ ok = FALSE;
+ break;
+ }
+ ct = CTGT_CITY;
}
2023-05-23 02:55 Updated by: lachu
I send to check if problems were fixed. It still has one problem: when we load ruleset from previous games and set counter_type to unit, it will load it without panic. I must check if ruleset version is older and check if counter_type is different than city.
Reply To cazfi
- Patch does not apply (any more?)
- Missing ruleset documentation about "target" field
- Following check is not strict enough. What if target was given as "blabla"? Even in compat mode, we should only accept the case where the "target" is completely missing: {{{ if (!counter_target_is_valid(ct)) { ... + if (compat->version > RSFORMAT_3_2) { + ruleset_error(NULL, LOG_ERROR, + "\"%s\" unknown counter target \"%s\".", + filename, counter_type); + ok = FALSE; + break; + } + ct = CTGT_CITY; }
>
0001-OSDN-47292-S-awomir-Lach-slawek-lach.art.pl.patch(8KB)
Reply To cazfi
- Patch does not apply (any more?)
- Missing ruleset documentation about "target" field
- Following check is not strict enough. What if target was given as "blabla"? Even in compat mode, we should only accept the case where the "target" is completely missing: {{{ if (!counter_target_is_valid(ct)) { ... + if (compat->version > RSFORMAT_3_2) { + ruleset_error(NULL, LOG_ERROR, + "\"%s\" unknown counter target \"%s\".", + filename, counter_type); + ok = FALSE; + break; + } + ct = CTGT_CITY; } }}}
2023-05-27 01:35 Updated by: lachu
I noticed small problem. When modder set invalid, but not blank target for newer ruleset, one condition check to not bypass, but should and else statement-related-instruction was executed.
Still, during trying to handle this issue, unconditionally I badly noticed some other error (it shown error that counter type is null), so I though It wrote that counter target is null and try to solve that, so (maybe?) code is ugly. Tell me if I should clean-up-it. It uses entry struct instead of testing if string is null from secfile_lookup_str_default function.
Sorry the review took so long. (But maybe you too should concentrate primarily to those patches that are targeted to 3.2 already, S3_2 d3f is in a year)
- There's one use of "malloc()" that should be "fc_malloc()"
- In load_ruleset_game() reading the counter target entry twice (once with direct secfile_entry_lookup() and once through secfile_lookup_str_default() ) doesn't serve any function, I think. At least we don't do that for anything else. I would drop counter_type_entry
- It would be better to set target correctly for 3.2 counters as soon as they are loaded, and not to all of them after ruleset has been loaded completely. It would make fixing bugs easier in this patch already, and likely becomes only more important during freeciv-3.3 development. Implement something like:
ct = counter_target_by_name(counter_target_3_3(compat, counter_type), fc_strcasecmp);where counter_target_3_3() will return either counter_type back, or possibly "City" in case of compatibility mode 3.2 ruleset loading.
+ }
+ else if (CTGT_CITY != ct) {
- "ruleset in format 30" - Use freeciv version numbers instead of format numberDidn't look at the documentation yet. If you provide multiple patches to apply, could you at least have different names for them (not overwriting each other when downloaded). Likely the simplest way is to use 'git format-patch' over the entire branch to get "0001-....patch" and "0002-....patch"
Add initial support for unit counters. No need to implement any counter behavior in this ticket - current counters code should be flexible enough to work (iterate over) zero counter behaviors and counter instances.
- Add counter_target type "Unit"
- Add counters_get_unit_counters_count()
- Implement unit_counters_iterate(pcount) macro (mostly by copying ciyt_counters_iterate() and modifying a bit)
- Add "counter_values" to unit structure
Savegame part likely isn't as trivial "copy from city counters implementation" -> let's leave that to another ticket.