Wesnoth: 1.15 changed behavior of affect_allies

[stevecotton]

Commit 2b65a8c5c8dfa0e58061d78f208499304805078a changed the meaning of abilities' affect_allies attribute, this issue is to work out what should be done with the affect_allies and affect_enemies attributes.

Before then, affect_allies was a tristate rather than a boolean before that change: it has a different meaning if it's unset vs "yes" or "no". The value determines both whether the ability affects units on the same side, and units on allied sides, as shown the diff of affects_side:
https://github.com/wesnoth/wesnoth/commit/2b65a8c5c8dfa0e58061d78f208499304805078a#diff-b1309a16dace4f5829b1ccb49d1d2ce5

[soliton-]

What is this issue for compared to #4258? Is there any question whether the unintentional change should be corrected?

[ProditorMagnus]

This task is to revert/split affect_allies behavior.

4258 is whether to keep current unintentional state for leadership only.

[gfgtdf]

Before then, affect_allies was a tristate rather than a boolean before that change: it has a different meaning if it's unset vs "yes" or "no".

This behaviour is not documented in the wiki isn't it? I think that if we want to keep the tristate behviour it'd be better to name the tree state explicitly (and opf course to document them properly). Alterntiveley we coudl also add a new key somethign like affect_own_side that' also allow us to create abilities that affect allies only but not the same team (altough that shoudl alos be possibel with extded filters already.)

[stevecotton]

Or a key that takes a list, something like 'affects=self, own_side, allied_sides, enemies'.

[jostephd]

There's another instance here:

https://github.com/wesnoth/wesnoth/blob/d3db9237ac68fd0f141d0a76d3ed0675e9a0c6f9/src/units/types.cpp#L166

but that struct member is unused, as far as I can tell.

Making the WML key a tristate or a list is a good idea, but in the meantime I think the following would restore the old behavior (there's just no reason to change it, even if it's undocumented):

diff --git a/src/units/abilities.cpp b/src/units/abilities.cpp
index 049c23f8575..c2fd4fe7f62 100644
--- a/src/units/abilities.cpp
+++ b/src/units/abilities.cpp
@@ -131,8 +131,10 @@ bool affects_side(const config& cfg, std::size_t side, std::size_t other_side)
    // display::get_singleton() has already been confirmed valid by both callers.
    const team& side_team = display::get_singleton()->get_disp_context().get_team(side);

-   if(side == other_side || !side_team.is_enemy(other_side)) {
+   if(side == other_side) {
        return cfg["affect_allies"].to_bool(true);
+   if(!side_team.is_enemy(other_side)) {
+       return cfg["affect_allies"].to_bool(false);
    } else {
        return cfg["affect_enemies"].to_bool();
    }

OK to push that?

[jostephd]

but that struct member is unused, as far as I can tell.

Confirmed that it's unused: I removed it from the hpp and from the constructor and wesnoth bult cleanly.

Pushed the patch with minor changes to make the diff from the pre-2b65a8c5c8dfa0e58061d78f208499304805078a state even smaller. This is the result:

$ git diff 2b65a8c5c8dfa0e58061d78f208499304805078a^ -- src/units/abilities.cpp
diff --git a/src/units/abilities.cpp b/src/units/abilities.cpp
index a688c0bba7c..e8f0fed2821 100644
--- a/src/units/abilities.cpp
+++ b/src/units/abilities.cpp
@@ -124,11 +126,14 @@ A poisoned unit cannot be cured of its poison by a healer, and must seek the car

 namespace {

-bool affects_side(const config& cfg, const std::vector<team>& teams, std::size_t side, std::size_t other_side)
+bool affects_side(const config& cfg, std::size_t side, std::size_t other_side)
 {
-   if (side == other_side)
+   // display::get_singleton() has already been confirmed valid by both callers.
+   const team& side_team = display::get_singleton()->get_disp_context().get_team(side);
+
+   if(side == other_side)
        return cfg["affect_allies"].to_bool(true);
-   if (teams[side - 1].is_enemy(other_side))
+   if(side_team.is_enemy(other_side))
        return cfg["affect_enemies"].to_bool();
    else
        return cfg["affect_allies"].to_bool();