Add HasInnateSkill for Skill checking

[MacroQuestGitHubMigrationBot]

In GitLab by @Sicprofundus on Aug 13, 2021, 02:54

in constant.h we have

constexpr int NUM_SKILLS = 100;

from in-game if we do a /skills
we get an output of all the skills and our skill level

you can see here that slam shows up at 111

this makes it so if we do a /doability slam mq2 says:

"you do not have this skill (slam)"

where other abilities - like begging, work as expected.

it doesn't appear to be simply a matter of adjusting NUM_SKILLS because we get this if we change the value
"Size of PcProfile does not match PcProfile_size"

MQ2Melee appears to have it checked specifically here with the "111"

int SKReady(unsigned long id) {
    if (id < 100 || id == 111 || id == 114 || id == 115 || id == 116)
    {
        return pSkillMgr->IsAvailable(id);
    }
    if (id == 105 || id == 107) return LoH_HT_Ready();
    return false;
}

not really sure where to go from here otherwise i'd try and have done an MR

edit:

to add further information - it isn't something that is actually listed as a skill with a value

yet it says it is a skill

/doability in mq2commands has, what appears to be, some handling for abilities like this if they are innate skills, but doesn't appear to function as indicated by the pics above

our skills.h file has all the way up to 132 (Throw Stone) and has the slam at 111

"Slam", // 111

[MacroQuestGitHubMigrationBot]

In GitLab by @Sicprofundus on Aug 13, 2021, 03:55

since this isn't trainable or something that has skill points associated with it - I'm guessing that was the purpose of "innateskill" here:

Index > NUM_SKILLS && pProfile->InnateSkill[Index - 100] != 0xFF

unfortunately it still checks the !HasSkill function and tells us we don't have it and doesn't use it

[MacroQuestGitHubMigrationBot]

In GitLab by @dannuic on Aug 13, 2021, 03:58

Hmm, this is interesting. I think you might be right about the innate skill bit, but that wouldn't be able to account for throw stone (as the number of innate skills must be 25)

[MacroQuestGitHubMigrationBot]

In GitLab by @dannuic on Aug 13, 2021, 04:18

Okay, looked at the /skills command. It accidentally works up to 125 because the InnateSkill array is directly after the Skill array on PcProfile. Then it reads memory not associated with skills (ArmorProps). My question is this: does the throw stone skill display properly for someone who has it?

[MacroQuestGitHubMigrationBot]

In GitLab by @Sicprofundus on Aug 13, 2021, 04:48

toon that has a throw stone ability:
shows in combat skills,
does not show in skills,
does not have associated skill value

[MacroQuestGitHubMigrationBot]

In GitLab by @dannuic on Aug 13, 2021, 04:55

Yeah, so the issue here is a couple of things: /skills isn't correct (it's just reading from skills.h/szSkills[] which has incorrect information; I'm not sure what might rely on that incorrect information though), the number of skills is definitely 100, and the number of innate skills is definitely 25, so both should be checked. There's HasSkill, but there's also HasInnateSkill (both eqclient functions that we have the addresses for). The signatures are slightly different, but that would be the correct way to check for innates.

I think we need to maintain the skill name mapping unless there's a dbstr lookup for it (there probably is since it shows in the client, but we haven't found/looked for the key mask), and do the simple fix of adding HasInnateSkill checks.

[Sicprofundus]

bumping this as it still exists, but i saw gitlab migration got rid of all the images

I can manually click "slam" but MQ doesn't think i have slam

/skills lists slam: 0 (at 111 position)
/doability slam returns "you do not have this skill (slam)"

manually clicking slam works

you can replicate this with a troll shaman (or w/e)

Skills
-----------------------
1H Blunt: 5
1H Slashing: 0
2H Blunt: 5
2H Slashing: 0
Abjuration: 10
Alteration: 10
Apply Poison: 0
Archery: 0
Backstab: 0
Bind Wound: 0
Bash: 0
Block: 0
Brass Instruments: 0
Channeling: 10
Conjuration: 10
Defense: 5
Disarm: 0
Disarm Traps: 0
Divination: 10
Dodge: 0
Double Attack: 0
Dragon Punch: 0
Dual Wield: 0
Eagle Strike: 0
Evocation: 10
Feign Death: 0
Flying Kick: 0
Forage: 0
Hand To Hand: 3
Hide: 0
Kick: 0
Meditate: 10
Mend: 0
Offense: 5
Parry: 0
Pick Lock: 0
1H Piercing: 5
Riposte: 0
Round Kick: 0
Safe Fall: 0
Sense Heading: 200
Singing: 0
Sneak: 0
Specialize Abjure: 0
Specialize Alteration: 0
Specialize Conjuration: 0
Specialize Divination: 0
Specialize Evocation: 0
Pick Pockets: 0
Stringed Instruments: 0
Swimming: 5
Throwing: 10
Tiger Claw: 0
Tracking: 0
Wind Instruments: 0
Fishing: 10
Make Poison: 0
Tinkering: 0
Research: 0
Alchemy: 0
Baking: 0
Tailoring: 0
Sense Traps: 0
Blacksmithing: 0
Fletching: 0
Brewing: 0
Alcohol Tolerance: 0
Begging: 5
Jewelry Making: 0
Pottery: 0
Percussion Instruments: 0
Intimidation: 0
Berserking: 0
Taunt: 0
Frenzy: 0
Remove Trap: 0
Triple Attack: 0
2H Piercing: 0
78: 0
79: 0
80: 0
81: 0
82: 0
83: 0
84: 0
85: 0
86: 0
87: 0
88: 0
89: 0
90: 0
91: 0
92: 0
93: 0
94: 0
95: 0
96: 0
97: 0
98: 0
99: 0
106: 0
110: 0
Slam: 0
Inspect: 0
Open: 0
125: 0
126: 0
127: 0
128: 0
129: 0
130: 0
131: 0
Throw Stone: 0
113 skills displayed.
> /doability slam
you do not have this skill (slam)

[Sicprofundus]

if i do a

/doability list i get this output:

> /doability list
Abilities & Combat Skills:
<Bind Wound>
<Sense Heading>
<Fishing>
<Begging>
<Remove Trap>
<Slam>
<Inspect>
<Open>
Combat Abilities:

[Sicprofundus]

so interestingly

We fail checking pLocalPC has the skill - in this case, 111

if (pLocalPC && pLocalPC->HasSkill(111)) {
	WriteChatf("I have 111");

}
else {
	WriteChatf("I do not have skill 111");
}

however if we tell it to use skill 111, it does use the skill as expected

pLocalPC->UseSkill(static_cast<uint8_t>(111), pLocalPlayer);

this is why we successfully output slam in the /doability list (because it doesn't check HasSkill)

[jessebevil]

Lines 2695-2696 of MQ2Commands.cpp has a check looks for the index in both pSkill[Index] and InnateSkill[Index - 100], then at 2700 it checks !HasSkill(Index).

I believe the correction here would be to say to do that only if Index < NUM_SKILLS && !HasSkill(Index) at line 2700, since it's already checking if InnateSKill[Index - 100] != 0xFF
This would allow it to function correctly. Since if it returns 255 it's not a valid innate skill for that character.

If it's really necessary to output that you don't have this skill, then it would require breaking the pSkill[Index] and InnateSkill[Index-100] apart like it was done above this area of code for the output of /doability list starting at line 2647 for the innate skills bit.

Additionally, at line 2672 when we use GetIntFromString(szBuffer, 0) we don't account for InnateSkills at all. So /doability 111 doesn't function either.

[Knightly1]

In GitLab by @dannuic on Aug 13, 2021, 04:55
I think we need to maintain the skill name mapping unless there's a dbstr lookup for it (there probably is since it shows in the client, but we haven't found/looked for the key mask)

Names are just regular strings, not key mapped, just keep the list of names.

[jessebevil]

To update this issue since it's brought up again.
The issue for /doability slam exists in void DoAbility(SPAWNINFO* pChar, char* szLine)

The following two checks pass

// scan for matching abilities name
for (int Index = 0; Index < 128; Index++)
{
	if (Index < NUM_SKILLS && pSkillMgr->pSkill[Index]->Activated
		|| Index > NUM_SKILLS && pProfile->InnateSkill[Index - 100] != 0xFF)
	{
		if (!_stricmp(szBuffer, szSkills[Index]))

However, we only check if (HasSkill(Index) to be true to determine if we have the skill. We don't also check

	Index > NUM_SKILLS && pProfile->InnateSkill[Index - 100] != 0xFF

and since HasInnateSkill(unsigned char, int) offset isn't currently available, this is our method of checking innate skills.
We already know it's a valid skill once we're past the two checks, the additional logic that checks HasSkill(nSkill) in this case is just looking for normal skills, not InnateSkill, which is what the additional check does.
This only fixes /doability slam, it does not fix /doability 111

/doability 111 is caught by

	if (IsNumber(szLine))
	{
		cmdDoAbility(pChar, szLine);
		return;
	}

and will have to be addressed seperately.

int SkillNmbr = GetIntFromString(szLine, -1);
    if (SkillNmbr != -1) {
        if (HasSkill(SkillNmbr)) {
            cmdDoAbility(pChar, szLine);
            return;
        }
        else if (SkillNmbr > NUM_SKILLS && pLocalPC->GetCurrentPcProfile()->InnateSkill[SkillNmbr - 100] != 0xFF) {
            pLocalPC->UseSkill(static_cast<uint8_t>(SkillNmbr), pLocalPlayer);
            return;
        }
    }

Will address that. And would also remove the need for

	int abilityNum = GetIntFromString(szBuffer, 0);
	if (abilityNum > 0)
	{
		// Check if user wants us to activate an ability by its "real id" (?)
		if (abilityNum > 6 && abilityNum < NUM_SKILLS)
		{
			int token = pSkillMgr->GetNameToken(abilityNum);
			if (token != 0)
			{
				strcpy_s(szBuffer, pStringTable->getString(token));
			}
		}
		else
		{
			// passthrough to original function
			cmdDoAbility(pChar, szLine);
			return;
		}
	}

for this section of code, which is never currently actually hit, but is also wrong in that it doesn't check innate skills.

[jessebevil]

It is my belief, and from a result of my testing, that

void DoAbility(SPAWNINFO* pChar, char* szLine)
{
	if (!szLine[0] || !cmdDoAbility || !pLocalPC)
		return;

	PcProfile* pProfile = GetPcProfile();
	if (!pProfile)
		return;

	int SkillNmbr = GetIntFromString(szLine, -1);
	if (SkillNmbr != -1) {
		if (HasSkill(SkillNmbr))
		{
			cmdDoAbility(pChar, szLine);
			return;
		}
		else if (SkillNmbr > NUM_SKILLS && pProfile->InnateSkill[SkillNmbr - 100] != 0xFF)
		{
			pLocalPC->UseSkill(static_cast<uint8_t>(SkillNmbr), pLocalPlayer);
			return;
		}
		else
		{
			WriteChatf("You do not seem to have ability (%d) available", SkillNmbr);
		}
	}

	char szBuffer[MAX_STRING] = { 0 };
	GetArg(szBuffer, szLine, 1);

	// display available abilities list
	if (!_stricmp(szBuffer, "list"))
	{
		WriteChatColor("Abilities & Combat Skills:");

		// display skills that have activated state
		for (int Index = 0; Index < NUM_SKILLS; Index++)
		{
			if (HasSkill(Index))
			{
				bool Avail = pSkillMgr->pSkill[Index]->Activated;

				// make sure remove trap is added, they give it to everyone except rogues
				if (Index == 75 && strncmp(pEverQuest->GetClassDesc(pProfile->Class & 0xFF), "Rogue", 6))
					Avail = true;

				if (Avail)
				{
					WriteChatf("<\ag%s\ax>", szSkills[Index]);
				}
			}
		}

		// display innate skills that are available
		for (int Index = 0; Index < 28; Index++)
		{
			if (pProfile->InnateSkill[Index] != 0xFF && strlen(szSkills[Index + 100]) > 3)
			{
				WriteChatf("<\ag%s\ax>", szSkills[Index + 100]);
			}
		}

		// display discipline i have
		WriteChatColor("Combat Abilities:", USERCOLOR_DEFAULT);

		for (int Index = 0; Index < NUM_COMBAT_ABILITIES; Index++)
		{
			if (pCombatSkillsSelectWnd->ShouldDisplayThisSkill(Index))
			{
				if (SPELL* pCA = GetSpellByID(pLocalPC->GetCombatAbility(Index)))
				{
					WriteChatf("<\ag%s\ax>", pCA->Name);
				}
			}
		}
		return;
	}

	// scan for matching abilities name
	for (int Index = 0; Index < 128; Index++)
	{
		if (Index < NUM_SKILLS && pSkillMgr->pSkill[Index]->Activated
			|| Index > NUM_SKILLS && pProfile->InnateSkill[Index - 100] != 0xFF)
		{
			if (!_stricmp(szBuffer, szSkills[Index]))
			{
				bool hasSkill = false;
				if (HasSkill(Index))
				{
					hasSkill = true;
				}
				else if (Index > NUM_SKILLS && pProfile->InnateSkill[Index - 100] != 0xFF)
				{
					hasSkill = true;
				}

				if (!hasSkill)
				{
					WriteChatf("you do not have this skill (%s)", szBuffer);
					return;
				}

				pLocalPC->UseSkill(static_cast<uint8_t>(Index), pLocalPlayer);
				return;
			}
		}
	}

	// scan for matching discipline name
	for (int Index = 0; Index < NUM_COMBAT_ABILITIES; Index++)
	{
		if (pCombatSkillsSelectWnd->ShouldDisplayThisSkill(Index))
		{
			if (SPELL* pCA = GetSpellByID(pProfile->CombatAbilities[Index]))
			{
				if (!_stricmp(pCA->Name, szBuffer))
				{
					pLocalPC->DoCombatAbility(pCA->ID);
					return;
				}
			}
		}
	}

	//display that we didnt find abilities
	WriteChatf("You do not seem to have ability %s available", szBuffer);
}

Fixes both issues.

[Knightly1]

This would break the original /doability command.

[SirCabby]

maybe we make /mqdoability?

[Knightly1]

While I do completely agree with splitting the commands, there's a level of backwards compatibility here that already has to be maintained, so I'm just going to fix this one up. I don't see a use case for needing to use the ability number instead of the name, once it's fixed.