wesnoth
wesnoth copied to clipboard
wesnoth
Generalize the use of max_value and add the min_value attribute
In the case of [leadership], the fact that the values of each ability can be added when cumulative=yes can justify the addition of limit values not to be exceeded, but this can also apply to heals and [regenerates] with the use of 'add' or 'multiply'.
As for [resistance], it already uses max_value, but if cumulative=yes, then all max_values can add up which could be problematic.
More generally, I would think that max_value and min_value should work the same for all abilities.
Why is there special handling for leadership and illuminates?
Illuminates has already own code for max_value and min_value and i fear to modify that, and for leadership i replace check bool by another with string comparaison
More generally, I would think that
max_valueandmin_valueshould work the same for all abilities.
the illuminated abilities affect both the beneficiary unit as well as the units adjacent to it, the value depending on the nature of the terrain and the time of day which must therefore be calculated hexagon by hexagon, it is therefore impossible to make the comparison between value and max_value cat the luminosity of the terrain must be integrated into the calculation which is done after effect::effect is called hence the need to prevent the use of min_value and max_value in effect:::effect for [illuminates ]
What I mean is that I'd expect they functionally work the same so that for any ability with a value attribute, the final value is restricted to the range specified by min_value and max_value (if those attributes are set).
What I mean is that I'd expect they functionally work the same so that for any ability with a value attribute, the final value is restricted to the range specified by
min_valueandmax_value(if those attributes are set).
No, it was not the case, and that precicely the motive to this PR
Alright. Can you rebase this against the current master branch?
it's done, cheking begin.
You can ignore the Windows failures. It's probably somehow related to the whole xz compromise:
error: https://github.com/tukaani-project/xz/archive/v5.4.4.tar.gz: failed: status code 404
close this PR, review https://github.com/wesnoth/wesnoth/pull/8722, please.
I've assumed that this is lower priority than the infinite recursion fixes.
My main question is, why are there still some abilities that aren't using this code to clamp the value? For example, illumination handles min_value and max_value, but doesn't use the codepath that's worked on here. git grep with this PR included:
src/actions/attack.cpp: unit_abilities::effect leader_effect(abil, 0, nullptr, unit_abilities::EFFECT_CUMULABLE);
src/actions/heal.cpp: unit_abilities::effect regen_effect(regen_list, 0, nullptr, unit_abilities::EFFECT_CLAMP_MIN_MAX);
src/actions/heal.cpp: unit_abilities::effect heal_effect(heal_list, 0, nullptr, unit_abilities::EFFECT_CLAMP_MIN_MAX);
src/gui/dialogs/attack_predictions.cpp: unit_abilities::effect dmg_effect(dmg_specials, weapon->damage());
src/tod_manager.cpp: unit_abilities::effect illum_effect(illum, terrain_light);
src/units/abilities.cpp: return unit_abilities::effect(abil_list, base_value, shared_from_this(), unit_abilities::EFFECT_CLAMP_MIN_MAX).get_composite_value();
src/units/types.cpp: unit_abilities::effect resist_effect(resistance_abilities, 100 - resistance);
src/units/unit.cpp: unit_abilities::effect defense_effect(defense_abilities, def);
src/units/unit.cpp: unit_abilities::effect resist_effect(resistance_list, 100-res, nullptr, unit_abilities::EFFECT_CLAMP_MIN_MAX);
I've assumed that this is lower priority than the infinite recursion fixes.
My main question is, why are there still some abilities that aren't using this code to clamp the value? For example, illumination handles min_value and max_value, but doesn't use the codepath that's worked on here.
git grepwith this PR included:src/actions/attack.cpp: unit_abilities::effect leader_effect(abil, 0, nullptr, unit_abilities::EFFECT_CUMULABLE); src/actions/heal.cpp: unit_abilities::effect regen_effect(regen_list, 0, nullptr, unit_abilities::EFFECT_CLAMP_MIN_MAX); src/actions/heal.cpp: unit_abilities::effect heal_effect(heal_list, 0, nullptr, unit_abilities::EFFECT_CLAMP_MIN_MAX); src/gui/dialogs/attack_predictions.cpp: unit_abilities::effect dmg_effect(dmg_specials, weapon->damage()); src/tod_manager.cpp: unit_abilities::effect illum_effect(illum, terrain_light); src/units/abilities.cpp: return unit_abilities::effect(abil_list, base_value, shared_from_this(), unit_abilities::EFFECT_CLAMP_MIN_MAX).get_composite_value(); src/units/types.cpp: unit_abilities::effect resist_effect(resistance_abilities, 100 - resistance); src/units/unit.cpp: unit_abilities::effect defense_effect(defense_abilities, def); src/units/unit.cpp: unit_abilities::effect resist_effect(resistance_list, 100-res, nullptr, unit_abilities::EFFECT_CLAMP_MIN_MAX);
Like illumination choose value in function of TOD, i prefer what it's own code remain for don't have bug later. defense abilities don't work, then why modify? and for gui dialogs and unit_type, i forget this and i will fix.
I've assumed that this is lower priority than the infinite recursion fixes.
My main question is, why are there still some abilities that aren't using this code to clamp the value? For example, illumination handles min_value and max_value, but doesn't use the codepath that's worked on here.
git grepwith this PR included:src/actions/attack.cpp: unit_abilities::effect leader_effect(abil, 0, nullptr, unit_abilities::EFFECT_CUMULABLE); src/actions/heal.cpp: unit_abilities::effect regen_effect(regen_list, 0, nullptr, unit_abilities::EFFECT_CLAMP_MIN_MAX); src/actions/heal.cpp: unit_abilities::effect heal_effect(heal_list, 0, nullptr, unit_abilities::EFFECT_CLAMP_MIN_MAX); src/gui/dialogs/attack_predictions.cpp: unit_abilities::effect dmg_effect(dmg_specials, weapon->damage()); src/tod_manager.cpp: unit_abilities::effect illum_effect(illum, terrain_light); src/units/abilities.cpp: return unit_abilities::effect(abil_list, base_value, shared_from_this(), unit_abilities::EFFECT_CLAMP_MIN_MAX).get_composite_value(); src/units/types.cpp: unit_abilities::effect resist_effect(resistance_abilities, 100 - resistance); src/units/unit.cpp: unit_abilities::effect defense_effect(defense_abilities, def); src/units/unit.cpp: unit_abilities::effect resist_effect(resistance_list, 100-res, nullptr, unit_abilities::EFFECT_CLAMP_MIN_MAX);
fixed except for illumination who shouldn't change of code.
If almost all callers pass EFFECT_CLAMP_MIN_MAX, then it might as well be the default code path. Instead of changing the callers, I'd like that to be the default for this function.
In a very quick test, illumination seems to work OK with EFFECT_CLAMP_MIN_MAX added to the call too. My preference would be to remove EFFECT_CLAMP_MIN_MAX from the EFFECT enum, and have EFFECT_DEFAULT clamp to min and max. Given that that's feature creep on the PR, I'd accept if the PR was force-pushed back to 4c32a3b, merged, and then I'd do the next bit; having 4c32a3b in the history would also document the change to leadership.
If almost all callers pass EFFECT_CLAMP_MIN_MAX, then it might as well be the default code path. Instead of changing the callers, I'd like that to be the default for this function.
In a very quick test, illumination seems to work OK with EFFECT_CLAMP_MIN_MAX added to the call too. My preference would be to remove EFFECT_CLAMP_MIN_MAX from the EFFECT enum, and have EFFECT_DEFAULT clamp to min and max. Given that that's feature creep on the PR, I'd accept if the PR was force-pushed back to 4c32a3b, merged, and then I'd do the next bit; having 4c32a3b in the history would also document the change to leadership.
in illuminates, this is highest "max_value" and lowest "min_value" who are used then what here it is lowes"max_value" and highest "min_value",same if a change encoding i must maiain exception for "illuminates" abilites
in illuminates, this is highest "max_value" and lowest "min_value" who are used then what here it is lowes"max_value" and highest "min_value"
Is there a reason for illuminates to be different like that? It would seem to prevent an ability such as "sprays fog into the air, limiting the maximum light to 10% lawful bonus" to override another illuminates ability with a 20% bonus.
There's a separate rule for terrains that illuminate, but that doesn't stack with abilities even in 1.18. Lava has light,max_light=25,35, but a drake hovering over a lava tile at dawn with a Mage of Light adjacent only gets a total of 25, not 35.
in illuminates, this is highest "max_value" and lowest "min_value" who are used then what here it is lowes"max_value" and highest "min_value"
Is there a reason for illuminates to be different like that? It would seem to prevent an ability such as "sprays fog into the air, limiting the maximum light to 10% lawful bonus" to override another illuminates ability with a 20% bonus.
There's a separate rule for terrains that illuminate, but that doesn't stack with abilities even in 1.18. Lava has
light,max_light=25,35, but a drake hovering over a lava tile at dawn with a Mage of Light adjacent only gets a total of 25, not 35.
i don't know reason but i don't want change existant behavior and remain careful.