About Player::addSkillAdvance(skills_t skill, uint64_t count)

[Jotran]

Why are we passing an unsigned 64 bit integer to this method?
Wouldn't it be easier to pass a smaller datatype to it when tries are added.. like a signed datatype?

It seems kind of silly if you think about it, this gets the tries for you

uint64_t currReqTries = vocation->getReqSkillTries(skill, skills[skill].level); 
uint64_t nextReqTries = vocation->getReqSkillTries(skill, skills[skill].level + 1);

But yet if you look at this line in add_skill.lua your sending the amount of tries for 1 level, which is counter productive when you can just do that inside the method.

target:addSkillTries(skillId, target:getVocation():getRequiredSkillTries(skillId, target:getSkillLevel(skillId) + 1) - target:getSkillTries(skillId))

Internally tries are added 1 point at a time, not entire levels & there is only 4 calls to this method 1 in luascript.cpp, 1 in player.cpp and 2 in weapons.cpp

luascript.cpp

int32_t LuaScriptInterface::luaPlayerAddSkillTries(lua_State* L)
{
    // player:addSkillTries(skillType, tries)
    Player* player = getUserdata<Player>(L, 1);
    if (player) {
        skills_t skillType = getNumber<skills_t>(L, 2);
        uint64_t tries = getNumber<uint64_t>(L, 3);
        player->addSkillAdvance(skillType, tries);
        pushBoolean(L, true);
    } else {
        lua_pushnil(L);
    }
    return 1;
}

player.cpp

void Player::onBlockHit()
{
    if (shieldBlockCount > 0) {
        --shieldBlockCount;

        if (hasShield()) {
            addSkillAdvance(SKILL_SHIELD, 1);
        }
    }
}

weapons.cpp

// 1st call
bool Weapon::useFist(Player* player, Creature* target)
{
    if (!Position::areInRange<1, 1>(player->getPosition(), target->getPosition())) {
        return false;
    }

    float attackFactor = player->getAttackFactor();
    int32_t attackSkill = player->getSkillLevel(SKILL_FIST);
    int32_t attackValue = 7;

    int32_t maxDamage = Weapons::getMaxWeaponDamage(player->getLevel(), attackSkill, attackValue, attackFactor);

    CombatParams params;
    params.combatType = COMBAT_PHYSICALDAMAGE;
    params.blockedByArmor = true;
    params.blockedByShield = true;

    CombatDamage damage;
    damage.origin = ORIGIN_MELEE;
    damage.primary.type = params.combatType;
    damage.primary.value = -normal_random(0, maxDamage);

    Combat::doCombatHealth(player, target, damage, params);
    if (!player->hasFlag(PlayerFlag_NotGainSkill) && player->getAddAttackSkill()) {
        player->addSkillAdvance(SKILL_FIST, 1);
    }

    return true;
}
// 2nd call
void Weapon::onUsedWeapon(Player* player, Item* item, Tile* destTile) const
{
    if (!player->hasFlag(PlayerFlag_NotGainSkill)) {
        skills_t skillType;
        uint32_t skillPoint;
        if (getSkillType(player, item, skillType, skillPoint)) {
            player->addSkillAdvance(skillType, skillPoint);
        }
    }

    uint32_t manaCost = getManaCost(player);
    if (manaCost != 0) {
        player->addManaSpent(manaCost);
        player->changeMana(-static_cast<int32_t>(manaCost));
    }

    if (!player->hasFlag(PlayerFlag_HasInfiniteSoul) && soul > 0) {
        player->changeSoul(-static_cast<int32_t>(soul));
    }

    if (breakChance != 0 && uniform_random(1, 100) <= breakChance) {
        decrementItemCount(item);
        return;
    }

    switch (action) {
        case WEAPONACTION_REMOVECOUNT:
            decrementItemCount(item);
            break;

        case WEAPONACTION_REMOVECHARGE: {
            uint16_t charges = item->getCharges();
            if (charges > 1) {
                g_game.transformItem(item, item->getID(), charges - 1);
            } else {
                g_game.internalRemoveItem(item);
            }
            break;
        }

        case WEAPONACTION_MOVE:
            g_game.internalMoveItem(item->getParent(), destTile, INDEX_WHEREEVER, item, 1, nullptr, FLAG_NOLIMIT);
            break;

        default:
            break;
    }
}

If your argument is we have to send a unsigned 64 bit integer to skill tries, that can be and is handled inside of this method.

Even death has a better formula for summing up the amount of tries a player has for their current skill level

for (uint16_t c = 11; c <= skills[i].level; ++c) { //sum up all required tries for all skill levels
    sumSkillTries += vocation->getReqSkillTries(i, c);
}

I was thinking about maybe changing it to an int8 or int16 so it wouldn't use up a ton of memory but also could handle negative numbers.

Also this would allow you add skill tries but also add or remove levels using the same function

Same thing with addManaSpent, another method of Player seems like it was written in a strange manner.