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

Scramble combat bonus

Added by Alexandro Ignatiev about 1 year ago. Updated 20 days ago.

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

0%

Estimated time:

Description

A way to better CivII compatibility that seems to be not so difficult to make: introduce a combat bonus that works like "DefenseMultiplier[Pct]" but only in cities. Apply it to civ2 [Stealth] Fighters (defense x4 against bombers, x2 against other fighters).

Problems left: CivII fighters don't profit from SAM battery unless against missiles.

Related discussion: http://forum.freeciv.org/f/viewtopic.php?t=318

scramble.patch (11.7 KB) scramble.patch Alexandro Ignatiev, 2021-03-06 02:48 PM
3.1-scramble-bonuses.patch (20.1 KB) 3.1-scramble-bonuses.patch Alexandro Ignatiev, 2021-03-19 09:04 PM
3.1-scramble.patch (22.4 KB) 3.1-scramble.patch Alexandro Ignatiev, 2021-03-22 10:18 PM
3.2-scramble.patch (22.4 KB) 3.2-scramble.patch Alexandro Ignatiev, 2021-03-22 10:18 PM
0024-Add-CityDefensePct-combat-bonus-type.patch (22.5 KB) 0024-Add-CityDefensePct-combat-bonus-type.patch master Marko Lindqvist, 2021-03-22 10:39 PM
0025-Add-CityDefensePct-combat-bonus-type.patch (22.6 KB) 0025-Add-CityDefensePct-combat-bonus-type.patch S3_1 Marko Lindqvist, 2021-03-22 10:39 PM
0019-Add-CityDefensePct-combat-bonus-type.patch (23 KB) 0019-Add-CityDefensePct-combat-bonus-type.patch master Marko Lindqvist, 2021-03-23 10:07 PM
0020-Add-CityDefensePct-combat-bonus-type.patch (23 KB) 0020-Add-CityDefensePct-combat-bonus-type.patch S3_1 Marko Lindqvist, 2021-03-23 10:07 PM

Related issues

Related to Freeciv - Bug #865085: civ2 ruleset: give Fighters defense bonus against bombersClosed

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>
Related to Freeciv - Feature #865086: Specific IgWalls combat bonusNew

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>
Blocked by Freeciv - Bug #922007: Give more space for unit type bonus value in the protocolClosed

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>
Blocks Freeciv - Feature #923193: civ2 ruleset: give fighters scramble bonusesClosed

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

History

#1 Updated by Alexandro Ignatiev about 1 year ago

  • Related to Bug #865085: civ2 ruleset: give Fighters defense bonus against bombers added

#2 Updated by Alexandro Ignatiev about 1 year ago

#3 Updated by Alexandro Ignatiev about 1 month ago

Made a patch, seems to work. "CityDefensePct" is the name of the percentage bonus. Patch for civ* rulesets coming.

#4 Updated by Marko Lindqvist about 1 month ago

  • Blocked by Bug #922007: Give more space for unit type bonus value in the protocol added

#5 Updated by Marko Lindqvist about 1 month ago

  • Category set to General
  • Status changed from New to In Progress

I like the basic idea. Will try to comment the implementation in a couple of days.

#6 Updated by Alexandro Ignatiev about 1 month ago

Maybe the idea would be good for v.2.5 but as we are at 3.1 stage it should be more general...
Like: "Attack" action result launches a selector for best "Defense" auto-performer among affected units, the auto-performers take in their reqs the attack action:
  • a.-p. "Scramble 2x" requires action "Fighter attack" (that requires actor flag "Fighter") and performer's flag "Fighter"
  • a.-p. "Scramble 4x" requires "Bomber attack" (actor flag "Bomber") and performer's flag "Fighter"
  • a.-p. "Ground defense" requires action "Ground attack" (actor class "Land") and performer's class "Land"
  • a.-p. "Unbonused defense" is used against action "Attack" (requires negated all aforementioned) and requires any of the following: performer has no "Unreachable" flag, performer is in a city, performer is in a native base, special auto-performer enabler considering performer and trigger is active (insto targets list);
  • if nothing of it fired, the attack action is rendered not possible even if it has a valid enabler (you can't strike a stack of unreachable units),

Auto-performer with a result "Defense" is the thing that activates the combat itself, and it is passed as an action (or a separate parameter we don't have yet) to "Defend_Bonus" effect.
But I don't understand the developers' plans for actions and whatever-looks-like-an-action-but-is-not-player-initiateds well enough, neither do I have concrete ideas how to write the code.

#7 Updated by Marko Lindqvist about 1 month ago

First comment: Is that addition of get_unittype_bonus(pplayer, ptile, att, EFT_DEFEND_BONUS); in aitech.c to be considered some kind of bugfix? It affects CBONUS_DEFENSE_MULTIPLIER and not the new CBONUS_SCRAMBLES_PCT. If so, it should go to a bug ticket of its own, targeted to all relevant branches. (I haven't yet reviewed that in context to see if it's a right thing to do or not)

#8 Updated by Alexandro Ignatiev about 1 month ago

No, it's not a bugfix. The defend bonus was out before because all units had equal one, now we need to compare it to scramble bonus. The variables we cange this way are only compared to each other, so rulesets wthout scrambling units should not be affected.

A bugfix probably needed back in 2.6 is that the algorithm does not take into account that not all units can fortify, but it's really anoter ticket.

#9 Updated by Sveinung Kvilhaugsvik about 1 month ago

Alexandro Ignatiev wrote:

But I don't understand the developers' plans for actions and whatever-looks-like-an-action-but-is-not-player-initiateds well enough, neither do I have concrete ideas how to write the code.

A possible small comfort: You personally have forced many items from my private notes to the issue tracker.

#10 Updated by Marko Lindqvist about 1 month ago

- defender_type_handled[utype_index(utype)] = FALSE;
+ defender_type_handled[idx] = 0;

Why changing FALSE to 0? The type is still bool.

--

daimilitary.c changes are mostly within "def->cache.max_defense_mp_pct > 0" so scrambling are only considered if the unit also has multiplier bonuses (that the cache variable name does not indicate it's only the bonus part is another matter -> should open a separate ticket). That multiplier bonus doesn't even have any effect, it just needs to exist!

--

if (coeff && best_non_scramble[idx] >= 0) {
best_non_scramble[idx] = defense_bonuses_pct[idx];

That doesn't seem right. defense_bonuses_pct might be scramble value. In fact, I don't see what the point of this assignment is in general, as best_non_scramble is set when ever there is non scramble case handled.

--

Documentation in rulesets missing

#11 Updated by Marko Lindqvist about 1 month ago

Marko Lindqvist wrote:

that the cache variable name does not indicate it's only the bonus part is another matter

-> Feature #922840

#12 Updated by Alexandro Ignatiev about 1 month ago

Marko Lindqvist wrote:

- defender_type_handled[utype_index(utype)] = FALSE;
+ defender_type_handled[idx] = 0;

Why changing FALSE to 0? The type is still bool.

Truly, I should not.

daimilitary.c changes are mostly within "def->cache.max_defense_mp_pct > 0" so scrambling are only considered if the unit also has multiplier bonuses (that the cache variable name does not indicate it's only the bonus part is another matter -> should open a separate ticket). That multiplier bonus doesn't even have any effect, it just needs to exist!

I thought I put max of scrambling OR regular type bonus into this cache.,, but I don't. Ah, I seemingly did it in unittype.c. Surely one can set regular bonus -50 and scramble bonus +100 and deceive it, it's said that negative bonuses are not handled by AI correctly, but let's correct it.

if (coeff && best_non_scramble[idx] >= 0) {
best_non_scramble[idx] = defense_bonuses_pct[idx];

That doesn't seem right. defense_bonuses_pct might be scramble value. In fact, I don't see what the point of this assignment is in general, as best_non_scramble is set when ever there is non scramble case handled.

Neither do I... Likely, we delete it...

Documentation in rulesets missing

Yup. I considered all ruleset changes another patch.
Will correct soon.

#13 Updated by Alexandro Ignatiev 25 days ago

Edited. I slightly changed how it calculates cache.max_defense_mp_pct for a sensible work of dai_unit_defence_desirability(). I edited comments in rulesets, the civ2 edit itself needs a separate patch.

#15 Updated by Alexandro Ignatiev 25 days ago

  • Blocks Feature #923193: civ2 ruleset: give fighters scramble bonuses added

#16 Updated by Marko Lindqvist 24 days ago

- In combat.c it seems like scramble_bonus variable could be declared within the "if (NULL != att_type) {" -block.
I were sure CodingStyle says that variables should be declared within the innermost block possible, like we have been doing in practice, but it seems not -> I'll open a new ticket about CodingStyle update. Given that it was not in CodingStyle, I would let this pass if you don't want to fix it.

- unittype.c: missing empty line between int emax declaration and following code line

- Isn't ruleset comment "In a city, instead of "Defend_Bonus" effect uses this bonus." wrong? The Defend_Bonus effect applies like usually, but it's the DefenseMultiplier and DefenseMultiplierPct combat bonuses that do not.

- Ruleset comment is missing at least for granularity ruleset

- Ruleset comment should be updated also in ruledit/comments-3.1 / ruledit/comments-3.2

#17 Updated by Marko Lindqvist 24 days ago

I were sure CodingStyle says that variables should be declared within the innermost block possible, like we have been doing in practice, but it seems not -> I'll open a new ticket about CodingStyle update.

-> Feature #923438

#18 Updated by Alexandro Ignatiev 24 days ago

Marko Lindqvist wrote:

- Isn't ruleset comment "In a city, instead of "Defend_Bonus" effect uses this bonus." wrong? The Defend_Bonus effect applies like usually, but it's the DefenseMultiplier and DefenseMultiplierPct combat bonuses that do not.

No. The thing this bonus do is overriding common "Defend_Bonus". The multipliers are multiplicative with it.

#19 Updated by Marko Lindqvist 24 days ago

Alexandro Ignatiev wrote:

Marko Lindqvist wrote:

- Isn't ruleset comment "In a city, instead of "Defend_Bonus" effect uses this bonus." wrong? The Defend_Bonus effect applies like usually, but it's the DefenseMultiplier and DefenseMultiplierPct combat bonuses that do not.

No. The thing this bonus do is overriding common "Defend_Bonus". The multipliers are multiplicative with it.

Right, so it seems when I reread the code. Maybe we should still list also those multiplier combat bonuses as replaced things in those ruleset comments? Their own documentation does not mention that they apply specifically via Defend_Bonus.

#20 Updated by Alexandro Ignatiev 24 days ago

Marko Lindqvist wrote:

Maybe we should still list also those multiplier combat bonuses as replaced things in those ruleset comments? Their own documentation does not mention that they apply specifically via Defend_Bonus.

Sorry, hard to understand. DefenseMultiplier[Pct] multiplies the defense, so does the "Defend_Bonus" effect, it's guessable they are multiplicative, no?

#21 Updated by Marko Lindqvist 24 days ago

Alexandro Ignatiev wrote:

Marko Lindqvist wrote:

Maybe we should still list also those multiplier combat bonuses as replaced things in those ruleset comments? Their own documentation does not mention that they apply specifically via Defend_Bonus.

Sorry, hard to understand. DefenseMultiplier[Pct] multiplies the defense, so does the "Defend_Bonus" effect, it's guessable they are multiplicative, no?

Yes, but there is a non-zero defense value even when there is no (Defend_Bonus) bonuses to it, and it's not clear that DefenseMultiplier does not multiply that.

#22 Updated by Alexandro Ignatiev 24 days ago

Marko Lindqvist wrote:

Alexandro Ignatiev wrote:

Marko Lindqvist wrote:

Maybe we should still list also those multiplier combat bonuses as replaced things in those ruleset comments? Their own documentation does not mention that they apply specifically via Defend_Bonus.

Sorry, hard to understand. DefenseMultiplier[Pct] multiplies the defense, so does the "Defend_Bonus" effect, it's guessable they are multiplicative, no?

Yes, but there is a non-zero defense value even when there is no (Defend_Bonus) bonuses to it, and it's not clear that DefenseMultiplier does not multiply that.

Again, I can't clearly understand. The comments speak about multiplying "defense", or "defense value", not defense bonus. So, a fortified pikeman (def=3, bonus against horses +200%) has 1.5x3x3x1=13.5 defense against Horsemen in open plain or unwalled plain city and 1.5x3x3x3=40.5 in a walled plain city, if there is a "berserk pikeman" unit with the same stats but CityDefensePct=100 for horses, he will have the same def in open plain but in any plain city 1.5x3x3x2=27. What you think needs clarification?

#23 Updated by Marko Lindqvist 24 days ago

I read the code again. Then I wrote a comment explaining what's wrong with it. Then I read the code n:th time, and finally saw how it works. So my mistake. The documentation and implementation match.

Maybe add comments that multipliers are already included in the scramble specific variables in

int def = deftype->defense_strength
        * (scramble ? scramble : defbonus * mp_pct) / div_bonus_pct;
and
if (scramble_bonus) {
defensepower = defensepower * scramble_bonus / 10000;
defensepower = MAX(0, defensepower);
} else {
int defense_multiplier_pct = 100
+ def_type->cache.defense_mp_bonuses_pct[utype_index(att_type)];

#24 Updated by Alexandro Ignatiev 22 days ago

Edited, send patches. Added more comments in the definition of cache.scramble_coeff[] and in other places, hope it is more comprehensive now.

#25 Updated by Marko Lindqvist 22 days ago

../../../../src/ai/default/daimilitary.c: In function ‘assess_danger’:
../../../../src/ai/default/daimilitary.c:576:14: error: declaration of ‘i’ shadows a previous local [-Werror=shadow]
576 | for (int i = 0; i < U_LAST; i++) { | ^
../../../../src/ai/default/daimilitary.c:490:7: note: shadowed declaration is here
490 | int i;

#26 Updated by Marko Lindqvist 22 days ago

- Fixed --enable-debug compile

#27 Updated by Marko Lindqvist 22 days ago

Needs also network capstr bump when committing.

#28 Updated by Alexandro Ignatiev 22 days ago

And need to adapt effect_cumulative_max() syntax to new HEAD.

#30 Updated by Marko Lindqvist 20 days ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF