Hi. Maybe set default value of Owned counter to 5? When City is conquered or transfer in other way, the value is set to 5, so we had extra unhappiness. But we can avoid unhappy for newly created cities, by setting this value to 5 by default.
H Reply To cazfi
Add a new requirement type 'Counter'. Name of the requirement is name of the counter. Requirement works only at City range for now. Add new 'checkpoint' int field to struct counter, and set it e.g. 5 for the City Owned counter. Requirement is fulfilled if the value of the requirement is at least checkpoint. Test for example by introducing a ruleset rule that when city has been owned less than those 5 turns, it suffers extra unhappiness.
I provided changes. It works. I tested in this way: 1. Ran game 2. Add empty enemy city (big city, with 7 population) 3. Add some my units (tanks) 4. Conquer city 5. Open conquered city's window 6. Keeping this window open (always on top), skip five turns 7. See people stop be unhappy
- You should not set checkpoint to 5 for ALL counters in counters_init(), but set for "Owned" counter only when initializing 'static struct counter counters'
- Document the new requirement type in README.effects ("Requirement types and supported ranges" section)
- Use uppercase boolean macro, instead of assuming actual boolean type, in C-code: false -> FALSE
- req_text_insert(): This should use translated name, not rule name. Same in universal_name_translation(). Unfortunately we don't have translated name yet -> I'll open a new ticket about that in a moment
- Add empty line between variable declarations and code, i.e., after "struct counter *count = req->source.value.counter;"
- No space after ! (not): "if (! target_city) {" -> "if (!target_city) {"
- Indentation wrong in worklist_item_post()
alienvalkyrie This patch is not up to date with more recent changes to master; in is_req_active(), you'll want to use context->city rather than target_city (since #43809). I only skimmed the patch and didn't test it, so there might be other things too; you'll likely want to rebase your local changes onto a current version of master. (If you don't know how to do that – stash or commit your local changes, then git pull --rebase; if there are any conflicts, you'll be prompted to resolve them on a patch-by-patch basis.)
Reply To cazfi
- You should not set checkpoint to 5 for ALL counters in counters_init(), but set for "Owned" counter only when initializing 'static struct counter counters'
Done
- Document the new requirement type in README.effects ("Requirement types and supported ranges" section)
Will be done in separate patch
- Use uppercase boolean macro, instead of assuming actual boolean type, in C-code: false -> FALSE
Done
- req_text_insert(): This should use translated name, not rule name. Same in universal_name_translation(). Unfortunately we don't have translated name yet -> I'll open a new ticket about that in a moment
Not done at the moment. I use invocation of universal_untranslated_name instead of returning rulename directly from struct
- Add empty line between variable declarations and code, i.e., after "struct counter *count = req->source.value.counter;"
Done
- No space after ! (not): "if (! target_city) {" -> "if (!target_city) {"
Done
- Indentation wrong in worklist_item_post()
Done
Sorry you must wait.
Sorry you must wait. Reply To cazfi
- You should not set checkpoint to 5 for ALL counters in counters_init(), but set for "Owned" counter only when initializing 'static struct counter counters'
- Document the new requirement type in README.effects ("Requirement types and supported ranges" section)
- Use uppercase boolean macro, instead of assuming actual boolean type, in C-code: false -> FALSE
- req_text_insert(): This should use translated name, not rule name. Same in universal_name_translation(). Unfortunately we don't have translated name yet -> I'll open a new ticket about that in a moment
- Add empty line between variable declarations and code, i.e., after "struct counter *count = req->source.value.counter;"
- No space after ! (not): "if (! target_city) {" -> "if (!target_city) {"
- Indentation wrong in worklist_item_post()
Probably done.
I think, why nobody response. I see I miss one problem: I use false statement instead of FALSE macrodefinition present in older version of C language.
alienvalkyrie Sorry for radio silence; happens sometimes.
counters.c: I believe cazfi meant to adjust the definition in the counters array at the very top of the file to set the checkpoint, rather than in counters_init(). In fact, because a new member is added to the counter struct, that definition needs to be adjusted anyway. It might also be sensible to use designated initialization a la { .rule_name = "Owned", .type = COUNTER_OWNED, /* etc */ }, since there are now two integer members that could cause confusion.
requirements.c, is_req_active(): Set eval variable rather than returning directly, and use tristate values, i.e. BOOL_TO_TRISTATE macro for the comparison and TRI_MAYBE when the city is null (to make sure negated requirements and different requirement problem types are handled correctly).
rssanity.c: Please mind the line length; try to keep lines below 77 characters (also see the doc/CodingStyle document).
README.effects is still missing info on Counter requirements (or rather, only exists in a separate patch – we'd like everything to be in one patch).
requirements.c, universal_name_translation() and reqtext.c, req_text_insert(): Since we are currently missing counter_name_translation() those can't be covered yet ~> #43989 must be resolved first.
alienvalkyrie)
Reply To alienvalkyrie
I think everything is fine now.
alienvalkyrie In common/requirements.c, req_from_str(), the first switch (to find a default range) is missing a VUT_COUNTER case. Tip: configure with --enable-debug to get compile errors for missing cases.
In ai/default/daieffects.c, dai_can_requirement_be_met_in_city(): The VUT_COUNTER case should probably be grouped together with the other cases without sensible handling at the bottom.
In common/reqtext.c, req_text_insert(): Use the translated name, and make sure there's a blank line before and after your case (in style with surrounding code). There also needs to be different text for present and negated requirements. I also think it might be sensible to have the word "counter" in there, since a counter name by itself might be ambiguous/hard to understand; smth like "Requires %s counter at %d or higher." and Requires %s counter below %d.", respectively.
In common/requirements.c, universal_number(): Should return the unique ID of the counter, so it can be retrieved via universal_by_number()/counter_by_id(). It would probably be sensible to add a function called counter_id or counter_number to counters.[ch] for this purpose.
In common/requirements.c, req_from_str(), in the second switch (valid ranges), the indentation is off.
In common/requirements.c, is_req_active(): Can you bring the VUT_COUNTER case in line with the surrounding code?
Tip: You can look at the other cases (e.g. government or style) to see what the code should look like.
In common/requirements.c, universal_name_translation() should use the translated counter name, as well as return the buffer (like the other cases).
In server/cityturn.c, worklist_item_postpone_req_vec(), counter requirements aren't handled.
In server/rssanity.c, sanity_check_req_set(), don't delete the newline after the previous case.
doc/README.effects is STILL missing info on Counter requirements in this patch.
In general, commit messages should mention the relevant ticket (i.e. this one) as well as the person who reported a bug or requested a feature (when applicable).
For the future, please look at how similar things are done in other places (e.g. surrounding code), to make sure you're not missing anything.
Reply To alienvalkyrie
In ai/default/daieffects.c, dai_can_requirement_be_met_in_city(): The VUT_COUNTER case should probably be grouped together with the other cases without sensible handling at the bottom.
Thanks. It should and I made an mistake. Maybe if somebody else will change code in other part, it will causes error.
In common/requirements.c, universal_number(): Should return the unique ID of the counter, so it can be retrieved via universal_by_number()/counter_by_id(). It would probably be sensible to add a function called counter_id or counter_number to counters.[ch] for this purpose.
Thanks. Big my mistake. I decided to ask you, what do you mean, but while typing this ,message, I saw (I mean look inside the code - English is not my strong skill) to code and repair bug.
I will send patch in few seconds.
Reply To alienvalkyrie
Maybe I do not told clear. I think all problems were addressed by latest patch.
- is_req_active(): "} else {" should be one line
- is_req_active(): add empty line between "struct counter *count = ..." and "eval ="
- worklist_item_postpone_req_vec(): comment talking about "techs" when you are handling counters
Reply To cazfi
- is_req_active(): "} else {" should be one line
- is_req_active(): add empty line between "struct counter *count = ..." and "eval ="
- worklist_item_postpone_req_vec(): comment talking about "techs" when you are handling counters
I done three changes you suggested.
Reply To cazfi
Reply To lachu
Reply To cazfi
- is_req_active(): "} else {" should be one line
- There is another instance of multi-line "} else {" in req_text_insert()
Repaired
More importantly, you need to rebase the patch against current git master HEAD. It doesn't apply in its current form.
Rebased
Build fails (while building ruledit, do you have Qt >= 5.11 so you could enable it?)
../../../../src/tools/ruledit/univ_value.c: In function ‘universal_value_initial’:
../../../../src/tools/ruledit/univ_value.c:42:3: error: enumeration value ‘VUT_COUNTER’ not handled in switch (-Werror=switch)
../../../../src/tools/ruledit/univ_value.c: In function ‘universal_kind_values’:
../../../../src/tools/ruledit/univ_value.c:238:3: error: enumeration value ‘VUT_COUNTER’ not handled in switch (-Werror=switch)
cc1: all warnings being treated as errors
On my system exist Qt 5.15.2 . I do not enable Qt, but when enabling ruleup, everything does compile.
Reply To lachu
On my system exist Qt 5.15.2 . I do not enable Qt, but when enabling ruleup, everything does compile.
It's ruledit failing, not ruleup. As you have Qt5 instead of Qt6, you need to use --with-qtver=qt5 to be able to build any of our Qt-based programs (Qt-client, Qt modpack installer, Ruledit)
I think it should work. But I had message patch do not apply, when I try to apply it.
EDIT:
After
git reset --hard HEAD~1
git am ./patch_name
apply
I previously test on other copy
I'm going to do some testing on this in a moment, but one thing missed so far is that req_text_insert() has a bug, I think. For !present requirement it says "at maximum %d value", but if the counter value is exactly the checkpoint it's going to meet 'present' (like text for 'present' correctly indicates), not '!present' requirement. So should be "less than %d"
Reply To cazfi
For !present requirement it says "at maximum %d value", but if the counter value is exactly the checkpoint it's going to meet 'present' (like text for 'present' correctly indicates), not '!present' requirement. So should be "less than %d"
Is there a new patch version coming? (I know it's been only ten days - just confirming that you noticed this)
Reply To cazfi
Reply To cazfi
For !present requirement it says "at maximum %d value", but if the counter value is exactly the checkpoint it's going to meet 'present' (like text for 'present' correctly indicates), not '!present' requirement. So should be "less than %d"
Is there a new patch version coming? (I know it's been only ten days - just confirming that you noticed this)
I do not even start. I wait when you do tests and return from holidays few days ago. I will introduce this little change tomorrow.
Reply To cazfi
Reply To cazfi
For !present requirement it says "at maximum %d value", but if the counter value is exactly the checkpoint it's going to meet 'present' (like text for 'present' correctly indicates), not '!present' requirement. So should be "less than %d"
Is there a new patch version coming? (I know it's been only ten days - just confirming that you noticed this)
As I promised, you can check my patch: "Merge with upstream and repair wrong helptext message" .
Reply To cazfi
For !present requirement it says "at maximum %d value", but if the counter value is exactly the checkpoint it's going to meet 'present' (like text for 'present' correctly indicates), not '!present' requirement. So should be "less than %d"
Is there a new patch version coming? (I know it's been only ten days - just confirming that you noticed this)
alienvalkyrie is_req_active() has a return TRI_MAYBE; instead of eval = TRI_MAYBE;
Reply To alienvalkyrie
is_req_active() has a return TRI_MAYBE; instead of eval = TRI_MAYBE;
True. That's a bug. Also, when looking at it, noticed that indentation there is wrong (3 spaces instead of 2)
Reply To cazfi
Reply To alienvalkyrie
is_req_active() has a return TRI_MAYBE; instead of eval = TRI_MAYBE;
True. That's a bug. Also, when looking at it, noticed that indentation there is wrong (3 spaces instead of 2)
Done.
I've tested that the latest patch compiles in my "standard setup". Will try to test with some more exotic build configurations too while it's in my stack of patches awaiting pushing in (tests to the entire stack, that is)
Add a new requirement type 'Counter'. Name of the requirement is name of the counter. Requirement works only at City range for now.
Add new 'checkpoint' int field to struct counter, and set it e.g. 5 for the City Owned counter. Requirement is fulfilled if the value of the requirement is at least checkpoint.
Test for example by introducing a ruleset rule that when city has been owned less than those 5 turns, it suffers extra unhappiness.