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
...
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.
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.