0002796: Merging of secondary skill bonuses does not check limiters/propagators/updaters

View Issue Details [ Jump to Notes ]

[ Issue History ] [ Print ]

Project

Category

View Status

Date Submitted

Last Update

0002796

VCMI

Mechanics - Other

public

2017-09-19 07:07

2018-03-12 21:08

Reporter

hkoehler

Assigned To

hkoehler

Priority

normal

Severity

minor

Reproducibility

always

Status

resolved

Resolution

fixed

Platform

OS Version

Product Version

1.next

Target Version

1.next

Fixed in Version

Summary

0002796: Merging of secondary skill bonuses does not check limiters/propagators/updaters

Description

In CGHeroInstance.cpp, bonuses from secondary skills are merged as follows:

void CGHeroInstance::updateSkill(SecondarySkill which, int val)
{
    auto skillBonus = (*VLC->skillh)[which]->getBonus(val);
    for (auto b : skillBonus)
    {
        // bonuses provided by different levels of a secondary skill are aggregated via max (not + as usual)
        // different secondary skills providing the same bonus (e.g. ballistics might improve archery as well) are kept separate
        std::shared_ptr<Bonus> existing = getBonusLocalFirst(
            Selector::typeSubtype(b->type, b->subtype).And(
            Selector::source(Bonus::SECONDARY_SKILL, b->sid).And(
            Selector::valueType(b->valType))));
        if(existing)
            vstd::amax(existing->val, b->val);
        else
            addNewBonus(std::make_shared<Bonus>(*b));
    }
    CBonusSystemNode::treeHasChanged();
}

This will cause bugs when a skills grants multiple bonuses differing on values not checked.

Steps To Reproduce

Example that produces bug:

"core:firstAid" : {
    "base" : {
        "effects" : {
            "dura" : {
                "type" : "STACK_HEALTH",
                "valueType" : "PERCENT_TO_BASE",
                "val" : 300,
                "limiters": [
                    {
                        "type" : "CREATURE_TYPE_LIMITER",
                        "parameters" : [ "firstAidTent" ]
                    }
                ]
            }
        }
    },
    "basic" : {
        "description" : "{Basic First Aid}\n\nGrants manual control of the first aid, increases healing to 50 points, and increases tent durability.",
    },
    "advanced" : {
        "description" : "{Advanced First Aid}\n\nFirst aid tent heals 75 points, and troops have 10% more health.",
        "effects" : {
            "xtra" : {
                "type" : "STACK_HEALTH",
                "val" : 10,
                "valueType" : "PERCENT_TO_BASE"
            }
        }
    },
    "expert" : {
        "description" : "{Expert First Aid}\n\nFirst aid tent heals 100 points, and troops have 20% more health.",
        "effects" : {
            "xtra" : {
                "type" : "STACK_HEALTH",
                "val" : 20,
                "valueType" : "PERCENT_TO_BASE"
            }
        }
    }
}

Additional Information

The approach of updating existing bonus isn't reliable. Instead, existing bonuses from secondary skill should be stripped, and replaced by new bonuses.

Notes
(0007499) hkoehler (developer) 2018-03-12 21:08	Fixed and merged.

Issue History
Date Modified	Username	Field	Change
2017-09-19 07:07	hkoehler	New Issue
2017-09-19 07:07	hkoehler	Status	new => assigned
2017-09-19 07:07	hkoehler	Assigned To	=> hkoehler
2018-03-12 21:08	hkoehler	Note Added: 0007499
2018-03-12 21:08	hkoehler	Status	assigned => resolved
2018-03-12 21:08	hkoehler	Resolution	open => fixed

Relationships