Combat spells not working since rev 830

One of the changes in r830 broke spells.
In CGameHandler::makeCustomAction, the test

(h->valOfBonuses(HeroBonus::BLOCK_SPELLS_ABOVE_LEVEL) <= s->level)

is always false because the bonus is initialized at 0.

A fix would be to have

const max_spell_level = 5
...
   giveArtBonus(83,HeroBonus::BLOCK_SPELLS_ABOVE_LEVEL, 2);//Recanter's Cloak
   giveArtBonus(126,HeroBonus::BLOCK_SPELLS_ABOVE_LEVEL, max_spell_level);//Orb of Inhibition

and the test would become

(s->level - h->valOfBonuses(HeroBonus::BLOCK_SPELLS_ABOVE_LEVEL) <= 0)

Using valOfBonuses method is wrong, it sums all bonuses of given types. If hero has two artifacts (or, more generally, bonuses) and first blocks spells above level 3 (4,5) and second above level 1 (2,3,4,5) then end result will be 4 (only level 5 will get blocked) which is senseless. Instead, we should take the lowest value.

So the fix would be doing something like that:

int levelLimit = SPELL_LEVELS;
const CGHeroInstance *h1 = gs->getHero(gs->curB->hero1);
if(h1)
	for(std::list<HeroBonus>::const_iterator i = h1->bonuses.begin(); i != h1->bonuses.end(); i++)
		if(i->type == HeroBonus::BLOCK_SPELLS_ABOVE_LEVEL)
			amin(levelLimit, i->val);

//same for hero2, or maybe it will be better to add for that a new 
//method: CGHeroInstance::minOfBonuses(type, subtype]) -> int

if(...
  || s->level > levelLimit
   ...

Right, it’s not cumulative.

What about adding a new variable per hero that contains it’s current max spell level ? And add a method to compute it when the hero levels up or add/remove an artefact. It would be better than compute that value every time the hero casts a spell.

I’d rather put such variable somewhere in the BattleInfo structure, since it’s global variable for given battle and it doesn’t necessarily come from heroes’ artifacts. Objects (Antimagic garrison) and Magical Terrain (Cursed Grounds) can also block spells. And in the further future scripts should be given such possibility as well.

I think that a special method for it in BattleInfo would be the best solution - it wouldn’t need to be updated :slight_smile: . I’ll fix that in a moment.

I removed the two offending lines in r861.

Also, since the value of battleMaxSpellLevel doesn’t change during a combat, it should be cached somewhere.

It can be calculated quite fast, we don’t use it often (about once per spell cast). Furthermore, in the future it may be possible that it changes during battle. I think that caching it will cause more trouble than it’s worth.