Weapon skills lost on load if multiple skills call unequip and equip in onAdded

[lordmidas]

The add function of skill_container does not check for !skill.isGarbage() for skills in the SkillsToAdd array, causing it to return from the function too early if a skill exists in SkillsToAdd which is garbage. This didn’t use to be an issue because skills in SkillsToAdd aren’t expected to be garbage, but, it is now possible.

Take a look at the vanilla skill orc_warlord_potion_effect which calls unequip and then equip on the character’s Mainhand item in its onAdded function. This doesn’t cause any issues right now as it is the only skill in the game that does this. However, even if ONE other such skill exists which does the same thing, this will result in the character losing the equipped weapon skills upon loading as saved game. The user will then be required to unequip and re-equip the weapon manually to get the skills.

The reason it happens is the following:
1. Upon loading a saved game, the character equips the weapon he was saved with. This adds the weapon’s skills to the character.
2. During skill_container deserialization, the skills are added to the character. This calls onAdded on all such skills.
3. During the onAdded function of a skill, the character unequips the weapon. This sets the weapon’s skills currently in the Skills array of the skill_container to Garbage.
4. Then the skill calls equip on the weapon. This adds the weapon’s skills to the SkillsToAdd array of the character’s skill_container because the skill_container’s IsUpdating boolean is true during deserialization.
5. Now, imagine we have another skill that does the same process of calling unequip and equip in its onAdded function.
6. The character again unequips the weapon. This calls <skill_container>.remove( _skill ) function for all the skills of the weapon. As the parameter _skill is passed by reference, it sets the skills to Garbage even while they are in the SkillsToAdd array from point 4.
7. The skill now calls equip on the weapon. But the skills are not added to the character as they already exist in SkillsToAdd and the function returns because of the following code (marked by a comment):

function add( _skill, _order = 0 )
{
	if (!_skill.isStacking())
	{
		foreach( i, skill in this.m.Skills )
		{
			if (!skill.isGarbage() && skill.getID() == _skill.getID())
			{
				skill.onRefresh();
				return;
			}
		}
		foreach( i, skill in this.m.SkillsToAdd )
		{
			if (skill.getID() == _skill.getID())
			{
				return; // Function returns here
			}
		}
	}
	_skill.setContainer(this);
	_skill.setOrder(_skill.getOrder() + _order);
	if (this.m.IsUpdating)
	{ 
		this.m.SkillsToAdd.push(_skill);
	}
	else
	{
		this.m.Skills.push(_skill);
		_skill.onAdded();
		_skill.m.IsNew = false;
		this.m.Skills.sort(this.compareSkillsByOrder);
		this.update();
	}
}

8. Then when the skills are added to the character, they get removed during the next collectGarbage because they were added as Garbage.

This entire problem can be solved by changing the if statement in the foreach loop that iterates over SkillsToAdd to

if (!skill.isGarbage() && skill.getID() == _skill.getID())

This is NOT solely a modding related bug. It is an oversight in vanilla code which will cause a bug in vanilla game too if any vanilla skill is added which calls unequip and equip in its onAdded function.

[Author]

Posts