feat: protocol 15.11 (weapon proficiency and new imbuement scroll)#3845
feat: protocol 15.11 (weapon proficiency and new imbuement scroll)#3845
Conversation
Introduce a new WeaponProficiency component (src/creatures/players/components/weapon_proficiency.{hpp,cpp}) that implements per-player weapon proficiencies. Features include loading proficiency definitions from JSON, experience arrays for different weapon classes, persistence (KV) serialization/deserialization, selected-perk management, experience/level calculations and mastery, and registration/application of perks that modify stats, criticals, spell augments, bestiary/boss bonuses and healing/mana on hit/kill. Also provides utility getters, resetters and integration points for combat damage adjustments and player skill interactions.
dudantas
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new WeaponProficiency subsystem: data-driven proficiency definitions, per-weapon progression and persistence, player integration, combat augment application, imbuement/scroll support, protocol/UI messages, item/protobuf updates, Lua bindings, utilities, and extensive data/content additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant WP as WeaponProficiency
participant JSON as Proficiencies_JSON
participant DB as Database
participant Combat
participant Protocol
Player->>WP: construct WeaponProficiency(player)
WP->>JSON: loadFromJson()
JSON-->>WP: definitions
Player->>DB: login -> load player
DB-->>Player: player data
Player->>WP: WP.load() (deserialize saved state)
Player->>Combat: attack/cast
Combat->>WP: applyAutoAttackCritical / applySpellAugment / applyBestiaryDamage
WP-->>Combat: modified CombatDamage
Player->>WP: addExperience(amount, weaponId)
WP->>WP: nextLevelExperience() → level up / unlock perks
WP->>DB: save(weaponId) / saveAll()
Protocol->>Player: request proficiency window
Player->>WP: getSelectedPerks / getExperience
WP-->>Protocol: sendWeaponProficiencyWindow(data)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c428ef9798
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@src/creatures/players/components/weapon_proficiency.cpp`:
- Around line 246-267: deserializePerk currently assumes every key exists in the
map (e.g., map["index"], map["type"], map["value"], etc.), which can crash on
missing keys; update WeaponProficiency::deserializePerk (and similarly
deserialize) to validate key existence before dereferencing: use map.find(...)
or a contains-style check for each expected key and either provide safe default
values for ProficiencyPerk fields (perk.index, perk.type, perk.value,
perk.level, perk.augmentType, perk.bestiaryId, perk.bestiaryName, perk.element,
perk.range, perk.skillId, perk.spellId) or return an empty/optional result when
required keys are absent; ensure you do not call operator-> on a null pointer
and propagate or log an error when skipping malformed entries.
- Around line 629-645: The current addSkillPercentage in WeaponProficiency sets
m_skillPercentage.skill every call which mixes different skills' percentages;
change the storage to track percentages per skill (e.g., replace
m_skillPercentage with a map<skills_t, SkillPercentage_t> or unordered_map and
update addSkillPercentage to accumulate into map[skill]) and update consumers
(applySkillAutoAttackPercentage, any code calling
getSkillLevel(skillPercentage.skill), and applyPerks handling SKILL_PERCENTAGE)
to lookup the correct SkillPercentage_t by skill before using its
autoAttack/spellDamage/spellHealing values; alternatively, if you prefer a
simpler change, have addSkillPercentage set m_skillPercentage.skill only if it
is uninitialized and assert/log/error when a different skill is added so callers
must provide a single-skill aggregation.
- Around line 736-744: getElementCritical currently returns a const reference
but returns a temporary on error (return {}), causing a dangling reference;
change the function to return by value (i.e., WeaponProficiencyCriticalBonus
instead of const WeaponProficiencyCriticalBonus&) so that both the normal path
(return m_elementCritical.at(enumValue)) and the error path return safe copies;
update the function signature in the declaration and implementation
(WeaponProficiency::getElementCritical) and ensure the error return constructs a
proper default WeaponProficiencyCriticalBonus value instead of returning a
temporary reference.
- Around line 908-920: The call to monster->getMonsterType()->isBoss() in
WeaponProficiency::applyBossDamage (and similarly in
WeaponProficiency::applyPowerfulFoeDamage) can dereference a null pointer; guard
against this by first retrieving auto mt = monster->getMonsterType() and
checking if mt is non-null before calling mt->isBoss(), returning early if mt is
null, so the isBoss() call is only invoked on a valid MonsterType object.
- Around line 64-69: The bounds check on perkIndex is off-by-one and allows an
out-of-bounds write to proficiencyLevel.perks; update the conditional in the
loop that currently uses "perkIndex > proficiencyLevel.perks.max_size()" to use
">=" so it rejects perkIndex == max_size(); make the equivalent fix in the
registerLevels function (the check around line 126) and in setSelectedPerk (the
checks around lines 411 and 417), ensuring all places compare with ">=" against
proficiencyLevel.perks.max_size() before accessing proficiencyLevel.perks.
- Line 139: proficiency.maxLevel is being set one too high — change the
assignment in weapon_proficiency.cpp so proficiency.maxLevel = levelIndex
(remove the +1) because levelIndex already equals the number of levels
processed; then audit and fix the indexing in nextLevelExperience and
getMaxExperience (these functions currently use the expression "(maxLevel - 1) +
2") to ensure they convert maxLevel to the correct zero-based index into
experienceArray (remove the extra +2 so you use the proper offset, e.g., convert
maxLevel to index by subtracting 1 or adjust to maxLevel directly as intended)
to avoid inflated indices or out-of-bounds access.
- Around line 790-808: In WeaponProficiency::addSpellBonus the field
m_spellsBonuses[spellId].increase.area is assigned with = instead of accumulated
like the other increase fields; change the handling in addSpellBonus so
increase.area uses += when the spellId already exists (i.e., update
m_spellsBonuses[spellId].increase.area += bonus.increase.area) to match
accumulation semantics for other fields in m_spellsBonuses.
- Around line 557-559: The function WeaponProficiency::getBestiaryExperience
computes a double polynomial that can be negative and is then returned as
uint32_t which causes undefined behavior; fix by validating/clamping: compute
the polynomial into a double (or long double), if monsterStar is out of expected
range (e.g., > maxStar) clamp or return 0, then clamp negative results to 0
before casting to uint32_t and return the non-negative value; ensure the change
is applied inside getBestiaryExperience and consider adding a small unit test
for monsterStar values (including >=7 and extreme uint8_t inputs).
- Around line 191-201: In WeaponProficiency::load() avoid letting std::stoi(key)
throw and avoid repeated scoped lookups: retrieve and cache the scoped KV handle
once (auto wp_kv = m_player.kv()->scoped("weapon-proficiency")), iterate its
keys, and for each key wrap the conversion in try/catch (catch
std::invalid_argument and std::out_of_range) — on parse failure skip that key
and continue; on success use proficiency[parsedId] = deserialize(kv.value()) as
before. Ensure you still check kv.has_value() before deserialize and keep
behavior unchanged for valid entries.
In `@src/creatures/players/components/weapon_proficiency.hpp`:
- Around line 24-27: Remove the misleading defaulted default constructor
declaration for WeaponProficiency; because the class holds a reference member
Player &m_player the implicitly-defined default constructor is deleted, so
delete the line "WeaponProficiency() = default;" and keep only the explicit
constructor declaration "explicit WeaponProficiency(Player &player)" to make
intent clear and avoid confusion.
🧹 Nitpick comments (4)
src/creatures/players/components/weapon_proficiency.cpp (2)
269-275: PassweaponDatabyconstreference to avoid unnecessary copy.
WeaponProficiencyDatacontains astd::vector<ProficiencyPerk>which makes the copy non-trivial. The function only reads the data.♻️ Proposed fix
-ValueWrapper WeaponProficiency::serialize(WeaponProficiencyData weaponData) const { +ValueWrapper WeaponProficiency::serialize(const WeaponProficiencyData &weaponData) const {
309-334: Magic numbers for augment types reduce readability.The
switchonselectedPerk.augmentTypeuses bare integer literals (2, 3, 6, 14, 15, 16, 17). Consider defining named constants or an enum for these augment types to improve maintainability and self-documentation.src/creatures/players/components/weapon_proficiency.hpp (2)
12-18: Consider forward-declaringMonsterinstead of including the full header.
monster.hppis a heavyweight include, andMonsteronly appears asstd::shared_ptr<Monster>in method signatures. Since this header is transitively included byplayer.hpp, forward-declaringMonsterinstead would reduce compilation coupling and build times. Move the#includeto the.cppfile where it's already present.♻️ Proposed change
-#include "creatures/monsters/monster.hpp" -#include "io/io_bosstiary.hpp" +class Monster; +enum class BosstiaryRarity_t : uint8_t;Then ensure
monster.hppandio_bosstiary.hppare included in the.cppfile.
39-44: Deserialize methods could bestatic— they don't usethis.
deserialize,deserializePerk, anddeserializePerks(lines 39-41) don't access any instance state — they only transform input values. Marking themstaticwould make this explicit and allow calling without an instance.
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/protobuf/appearances.proto (1)
19-51:⚠️ Potential issue | 🟠 MajorReplace the direct cast in
src/items/items.cpp:206with an explicit conversion function to map protobuf ITEM_CATEGORY to ItemTypes_t.The protobuf enum now extends to value 31 with
ITEM_CATEGORY_SOULCORES, but the C++ ItemTypes_t enum (protected "Start-End" range) only extends to 27. The direct caststatic_cast<ItemTypes_t>(object.flags().market().category())will produce silent misclassifications:
- Proto value 26 (
TWOHANDWEAPON) maps to C++ value 26 (ITEM_TYPE_SOULCORES) — wrong item type- Proto value 31 (
SOULCORES) maps to C++ value 31 — no valid enum constantCreate an explicit mapping function (e.g.,
protoItemCategoryToCpp()) that handles all 31 proto enum values. Existing code relying on sequential alignment will silently cast items to the wrong category, breaking item classification and persisted data.src/items/items_definitions.hpp (1)
505-642:⚠️ Potential issue | 🟡 Minor
ITEM_PARSE_PROFICIENCYis a dead enum entry without XML parser support.The enum value was added to
items_definitions.hpp, but there is no correspondingparseProficiency()function initem_parse.cpp, andinitParse()never calls such a function. This means proficiency data cannot be loaded from item XML definitions—any proficiency attribute in XML will be silently ignored.While the codebase does handle proficiency data (loaded via protobuf in
items.cpp), the XML parser has no support for it. Either add the parser function call toinitParse()or remove the unused enum entry.src/lua/functions/creatures/player/player_functions.cpp (1)
3305-3325:⚠️ Potential issue | 🟠 Major
luaPlayerApplyImbuementScrollreturns 1 without pushing a value on success; inconsistent with error path.On success, this function returns
1but never pushes a value to the Lua stack (line 3322–3324). The error paths correctly pushfalse(line 3318), but the success path is inconsistent—the Lua caller receives an unpredictable return value instead of a deterministic boolean result.Since
Player::applyScrollImbuementreturnsvoid, the fix is to push a boolean result before returning:Proposed fix
player->applyScrollImbuement(item, scrollItem); - + Lua::pushBoolean(L, true); return 1;src/items/weapons/weapons.cpp (1)
135-146:⚠️ Potential issue | 🟡 MinorAvoid uint8_t wrap when adding proficiency range bonus.
shootRangeisuint8_t; adding a proficiency bonus can overflow and wrap. Consider calculating with a wider type and clamping before the distance check.🔧 Suggested fix
-int32_t Weapon::playerWeaponCheck(const std::shared_ptr<Player> &player, const std::shared_ptr<Creature> &target, uint8_t shootRange) const { - shootRange += player->weaponProficiency().getStat(WeaponProficiencyBonus_t::ATTACK_RANGE); +int32_t Weapon::playerWeaponCheck(const std::shared_ptr<Player> &player, const std::shared_ptr<Creature> &target, uint8_t shootRange) const { + const auto rangeBonus = player->weaponProficiency().getStat(WeaponProficiencyBonus_t::ATTACK_RANGE); + const int32_t effectiveRange = std::max<int32_t>(0, static_cast<int32_t>(shootRange) + rangeBonus); @@ - if (std::max<uint32_t>(Position::getDistanceX(playerPos, targetPos), Position::getDistanceY(playerPos, targetPos)) > shootRange) { + if (std::max<uint32_t>(Position::getDistanceX(playerPos, targetPos), Position::getDistanceY(playerPos, targetPos)) > static_cast<uint32_t>(effectiveRange)) { return 0; }src/creatures/players/player.cpp (1)
7227-7251:⚠️ Potential issue | 🔴 CriticalGuard against null
abilitieswhen proficiency supplies the range.With the new proficiency fallback,
perfectShotRangecan be set even whenitemType.abilitiesis null, but the code still dereferencesitemType.abilities->perfectShotDamage, which can crash.🛠️ Suggested fix (null-check abilities)
- if (perfectShotRange == range) { - result += itemType.abilities->perfectShotDamage; + if (perfectShotRange == range && itemType.abilities) { + result += itemType.abilities->perfectShotDamage; const uint16_t charges = item->getCharges(); if (useCharges && charges != 0) { g_game().transformItem(item, item->getID(), charges - 1); } }src/server/network/protocol/protocolgame.cpp (2)
267-292:⚠️ Potential issue | 🟡 MinorRemove unused local (
playerItem) to avoid -Wunused warnings.
playerItemis fetched but never used. If the project treats warnings as errors, this can break the build.Proposed diff
void addCyclopediaSkills(std::shared_ptr<Player> &player, NetworkMessage &msg, skills_t skill, const SkillsEquipment &skillEquipment) { const auto skillTotal = player->getSkillLevel(skill); - const auto &playerItem = player->getInventoryItem(CONST_SLOT_LEFT); double skillWheel = 0.0; const auto &playerWheel = player->wheel();
4298-4508:⚠️ Potential issue | 🟡 MinorAvoid implicit narrowing for
flatDamageHealingUnknown(double -> uint16).
flatDamageHealingUnknownis declared asdoublebut serialized asuint16_t. Make it auint16_tto avoid accidental narrowing warnings and clarify intent.Proposed diff
const auto flatBonus = player->calculateFlatDamageHealing(); - double flatDamageHealingUnknown = 0.0; + uint16_t flatDamageHealingUnknown = 0; msg.add<uint16_t>(flatBonus); // Flat Damage and Healing Total msg.add<uint16_t>(flatBonus); msg.add<uint16_t>(flatDamageHealingUnknown);
🤖 Fix all issues with AI agents
In `@data/scripts/globalevents/save_interval.lua`:
- Around line 13-30: getTimeLeftMs currently uses an undefined saveTimeStr and
makes multiple unsafe os.time() calls; fix by ensuring saveTimeStr is
initialized (e.g., a module-level default or passed into the save object),
validate that saveTimeStr is a string in "HH:MM" format before parsing in
getTimeLeftMs, safely parse h and m with tonumber checks and return a sensible
fallback (or error) if parsing fails, and compute the time delta using a single
cached now = os.time() / os.date("*t") value (use that same value for
comparisons and subtraction) so the result cannot be skewed by multiple
os.time() calls; update references in getTimeLeftMs and any callers (e.g.,
save.onTime) to rely on the initialized/validated saveTimeStr.
- Around line 34-55: save.onTime currently returns false when
configKeys.TOGGLE_SAVE_INTERVAL is off (permanently removing the scheduled
event), schedules early saves by using delay = math.min(WARNING, timeLeft -
1000) even when timeLeft >> WARNING, and keeps queuing addEvent(serverSave, ...)
on every tick; fix by (1) never returning false to disable the event—simply
return true early so the scheduler remains registered but skip scheduling when
toggle is off by checking
configManager.getBoolean(configKeys.TOGGLE_SAVE_INTERVAL) and returning true if
disabled, (2) compute WARNING = 60*1000 and timeLeft = getTimeLeftMs(), then
only schedule a warning/save if timeLeft <= WARNING (i.e. inside the warning
window); when inside window call serverSave immediately if timeLeft <= 1000 else
schedule addEvent(serverSave, timeLeft - 1000, interval) (do not use math.min
with WARNING), and (3) avoid queuing multiple saves by only calling addEvent
once when entering the warning window (the above condition ensures no repeated
queuing while timeLeft > WARNING).
In `@src/canary_server.cpp`:
- Line 82: The call to WeaponProficiency::loadFromJson() currently ignores its
bool return and relies on consoleHandlerExit() (which doesn't stop execution);
update the caller so that after calling WeaponProficiency::loadFromJson() you
check its return value and throw an exception (e.g., std::runtime_error with
context) on failure—this mirrors initializeDatabase() behavior and matches
loadModules() checks; alternatively, refactor WeaponProficiency::loadFromJson()
itself to throw on error instead of returning false/using consoleHandlerExit()
so callers can handle exceptions consistently.
In `@src/creatures/players/player.cpp`:
- Around line 12389-12411: In Player::getSkillsEquipment(), guard against null
pointers before dereferencing itemType.abilities and imbuementInfo.imbuement:
after retrieving const auto &itemType = Item::items[item->getID()], check if
itemType.abilities is non-null before accessing abilities->skills[skill] and
skip adding equipment bonuses when null; likewise, inside the imbuement loop
verify imbuementInfo.imbuement is non-null before using imbuement->skills[skill]
and continue if null. Ensure these checks are applied around the existing uses
so aggregation remains correct without dereferencing null pointers.
In `@src/enums/weapon_proficiency.hpp`:
- Around line 128-131: The struct WeaponProficiencyBonusStat has an
uninitialized member (uint32_t value) which can hold indeterminate data; fix it
by adding a default member initializer for value (e.g., set value = 0) in the
WeaponProficiencyBonusStat definition so that default-constructed instances are
well-defined while leaving CombatType_t element's existing default (element =
COMBAT_NONE) unchanged.
In `@src/server/network/protocol/protocolgame.cpp`:
- Around line 1332-1337: The 0xB2 and 0xB3 dispatch cases call
parseImbuementAction and parseWeaponProficiency unconditionally and can
mis-parse old clients; add a version gate using the connection's oldProtocol
check so these handlers are skipped for protocol 11.00 clients — e.g. in
protocolgame.cpp, before calling parseImbuementAction or parseWeaponProficiency,
check if (oldProtocol) return; (or continue/skip the case) so only newer
protocol connections reach these functions.
In `@vcproj/canary.vcxproj`:
- Line 58: The vcxproj currently references weapon_proficiency files under a
non-existent wheel/ subdirectory; update the project XML entries (the ClInclude
and ClCompile elements that reference weapon_proficiency.hpp and
weapon_proficiency.cpp) to remove the "wheel/" path segment so they point to
..\src\creatures\players\components\weapon_proficiency.hpp and
..\src\creatures\players\components\weapon_proficiency.cpp respectively.
🟠 Major comments (14)
data-otservbr-global/scripts/actions/object/greater_proficiency_catalyst.lua-12-14 (1)
12-14:⚠️ Potential issue | 🟠 MajorSame missing weapon validation as
proficiency_catalyst.lua.This script has the identical issue: no check that the target is actually a weapon. A player can use the greater catalyst on any item. See the comment on
proficiency_catalyst.luafor the suggested fix.Also, line 12 has the same spaces-vs-tabs indentation inconsistency.
data-otservbr-global/scripts/actions/object/proficiency_catalyst.lua-12-14 (1)
12-14:⚠️ Potential issue | 🟠 MajorNo validation that target is actually a weapon before granting proficiency experience.
Any item (armor, shield, decoration, etc.) can be targeted, and the catalyst will be consumed while granting weapon experience for a non-weapon item ID. This could lead to wasted catalysts or exploitable behavior. Consider checking if the target item has a valid weapon type before proceeding.
Additionally,
addWeaponExperiencereturn value is not checked — if it fails, the catalyst is still consumed.Proposed fix
local weaponId = target:getId() + local itemType = ItemType(weaponId) + if not itemType or itemType:getWeaponType() == WEAPON_NONE then + player:sendCancelMessage("You can only use this on a weapon.") + return true + end + player:addWeaponExperience(25000, weaponId) item:remove(1)src/creatures/players/components/wheel/player_wheel.hpp-182-224 (1)
182-224:⚠️ Potential issue | 🟠 MajorGuard
gradebounds before indexingspellBonus.grade[grade].At Line 204,
spellBonus.grade[grade]is unchecked. Ifgradeis out of range for any entry invocSpells, this is undefined behavior and can crash.Proposed fix (bounds check)
for (const auto &spellBonus : vocSpells) { if (spellBonus.name != spellName) { continue; } + if (grade >= spellBonus.grade.size()) { + continue; + } const auto &bonus = spellBonus.grade[grade]; addValue(Augment_t::ManaCost, -bonus.decrease.manaCost); addValue(Augment_t::BaseDamage, bonus.increase.damage / 100.0); addValue(Augment_t::BaseHealing, bonus.increase.heal / 100.0);data-otservbr-global/npc/albinius.lua-276-286 (1)
276-286:⚠️ Potential issue | 🟠 MajorAvoid overwriting
npcConfig.shop(drops existing entries) + remove duplicate handlers.At Line 276,
npcConfig.shop = { ... }replaces the earlier shop definition (Line 26). If the goal is to add “blank imbuement scroll” and “etcher”, this likely removes the “heavy old tome” sell option unintentionally.Also,
npcType.onSellItemandnpcType.onCheckItemare already defined earlier in the file; redefining them here is redundant and makes the effective behavior depend on file order.Proposed fix (append items instead of replacing the whole shop)
-npcConfig.shop = { - { itemName = "blank imbuement scroll", clientId = 51442, buy = 25000 }, - { itemName = "etcher", clientId = 51443, buy = 30000 } -} - -npcType.onSellItem = function(npc, player, itemId, subtype, amount, ignore, name, totalCost) - player:sendTextMessage(MESSAGE_TRADE, string.format("Sold %ix %s for %i gold.", amount, name, totalCost)) -end - -npcType.onCheckItem = function(npc, player, clientId, subType) end +table.insert(npcConfig.shop, { itemName = "blank imbuement scroll", clientId = 51442, buy = 25000 }) +table.insert(npcConfig.shop, { itemName = "etcher", clientId = 51443, buy = 30000 })src/lua/functions/creatures/player/player_functions.cpp-5231-5244 (1)
5231-5244:⚠️ Potential issue | 🟠 Major
player:setSpeed()incorrectly applies a delta instead of setting absolute speed.The function comment indicates
player:setSpeed(speed)should set the player's speed, but the implementation callsg_game().changePlayerSpeed(player, speed), which treats the parameter as a delta (not absolute). The function signature confirms this:void changePlayerSpeed(..., int32_t varSpeedDelta)and the implementation adds to the existing delta:varSpeed += varSpeedDelta.This creates a semantic mismatch:
creature:setSpeed(speed)correctly callssetCreatureSpeed()which sets absolute base speed, whileplayer:setSpeed(speed)adds a delta. Scripts using the Lua API will be misled into thinking they're setting absolute speed when they're actually modifying a temporary speed bonus.Additionally, there is no validation for negative values or bounds checking before applying the delta.
data/items/items.xml-84744-84760 (1)
84744-84760:⚠️ Potential issue | 🟠 MajorItem 51266 (
norcferatu bloodstrider):augmentsdeclaresvalue="2"but only contains one child augment.Line 84750 sets
augments value="2", but only one augment child is defined (ultimate terra strike/base damage). Every other item withaugments value="2"in this PR has exactly two child entries. This is likely a missing augment definition, which could cause parsing issues or silent data loss depending on how the engine validates the count.Possible fix: either add the missing second augment or change the count to 1
If the second augment was intended (e.g., matching the pattern of nearby items like 51265 which pairs
base damagewithcritical extra damage):<attribute key="augments" value="2"> <attribute key="ultimate terra strike" value="base damage"> <attribute key="value" value="33"/> </attribute> + <attribute key="ultimate terra strike" value="critical extra damage"> + <attribute key="value" value="8"/> + </attribute> </attribute>Otherwise, fix the count:
- <attribute key="augments" value="2"> + <attribute key="augments" value="1">src/utils/tools.cpp-71-73 (1)
71-73:⚠️ Potential issue | 🟠 Major
undoShift: unsigned arithmetic underflow risk.Already flagged in the header —
std::countr_zero(value) - 2can underflow whenvaluehas fewer than 2 trailing zeros. The subtraction is done onint(fromcountr_zeroreturn type), then narrowed touint8_t, so the wrap happens at the narrowing conversion.Consider adding a precondition check:
🛡️ Proposed defensive guard
uint8_t undoShift(uint64_t value) { + auto zeros = std::countr_zero(value); + if (zeros < 2) { + g_logger().error("[undoShift] value {} has fewer than 2 trailing zero bits", value); + return 0; + } - return std::countr_zero(value) - 2; + return static_cast<uint8_t>(zeros - 2); }src/utils/tools.hpp-47-48 (1)
47-48:⚠️ Potential issue | 🟠 MajorAdd defensive guard to prevent unsigned underflow in
undoShift.The implementation computes
std::countr_zero(value) - 2, which underflows whenvaluehas fewer than 2 trailing zeros (e.g.,value == 1→countr_zeroreturns 0 → wraps to 254). Current data in proficiencies.json only contains powers of 2 (8, 16, 32, 64, 128, 256, 1048576), all with ≥3 trailing zeros, so the issue is not exposed in practice. However, the function has no guards and relies entirely on input data constraints—this is fragile.Add a guard or assertion to handle values with fewer than 2 trailing zeros, or document the strict precondition that inputs must be powers of 2 with at least 4 as the minimum value.
src/creatures/players/components/wheel/player_wheel.cpp-2563-2604 (1)
2563-2604:⚠️ Potential issue | 🟠 MajorFix augment registration to avoid misapplying bonuses and missing grade 3.
registerAugmentiterates all registered spells but always uses the currentspellName/grade, so earlier spells can receive the wrong augment data. Also, repeated spells never reach grade 3 because any duplicate gets hard-coded to grade 2. This will under-apply higher-stage augments.🔧 Proposed fix (apply augments per spell and track grade count)
- auto registerAugment = [&](const uint16_t spellId, const std::string &spellName, uint8_t grade, std::unordered_set<uint16_t> registeredSpells) { - for (const auto &spellId : registeredSpells) { - if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_SORCERER) { - getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.sorcerer, wheelAugments); - } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_DRUID) { - getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.druid, wheelAugments); - } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_PALADIN) { - getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.paladin, wheelAugments); - } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_KNIGHT) { - getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.knight, wheelAugments); - } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_MONK) { - getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.monk, wheelAugments); - } - } - }; + auto registerAugment = [&](const uint16_t spellId, const std::string &spellName, uint8_t grade) { + if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_SORCERER) { + getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.sorcerer, wheelAugments); + } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_DRUID) { + getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.druid, wheelAugments); + } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_PALADIN) { + getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.paladin, wheelAugments); + } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_KNIGHT) { + getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.knight, wheelAugments); + } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_MONK) { + getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.monk, wheelAugments); + } + }; - std::unordered_set<uint16_t> registeredSpells; + std::unordered_map<uint16_t, uint8_t> spellGrades; for (const auto &spellName : m_playerBonusData.spells) { - uint8_t grade = 1; const auto &spell = g_spells().getSpellByName(spellName); if (!spell) { continue; } auto spellId = spell->getSpellId(); - if (!registeredSpells.contains(spellId)) { - registeredSpells.emplace(spellId); - - registerAugment(spellId, spellName, grade, registeredSpells); - - continue; - } - - grade = 2; - - registerAugment(spellId, spellName, grade, registeredSpells); + auto &grade = spellGrades[spellId]; + if (grade < 3) { + ++grade; + } + registerAugment(spellId, spellName, grade); }src/creatures/combat/combat.cpp-758-763 (1)
758-763:⚠️ Potential issue | 🟠 Major
applyOn(LIFE/MANA, HIT)called before block check — grants HP/mana even on blocked attacks.Lines 762–763 invoke
applyOn(WeaponProficiencyHealth_t::LIFE, WeaponProficiencyGain_t::HIT)andapplyOn(WeaponProficiencyHealth_t::MANA, WeaponProficiencyGain_t::HIT)beforecombatBlockHit()at line 769. These methods directly apply healing viaCombat::doCombatHealth()andCombat::doCombatMana()without checking whether the attack will be blocked. If the attack is subsequently blocked (shield/armor), the attacker still receives the on-hit life/mana gain.Since
HITsemantically means "on successful hit," these calls should be moved after the block check — either after line 769 or within thecombatChangeHealth()success block starting at line 798.src/creatures/players/player.cpp-2466-2491 (1)
2466-2491:⚠️ Potential issue | 🟠 MajorApply scroll consumption before mutating the item.
item->setImbuement(...)happens before the scroll is removed. IfinternalRemoveItemfails, the item stays imbued without consuming the scroll, and stats/decay can become inconsistent. Reorder so the scroll removal succeeds first (or rollback the imbuement on failure).🛠️ Suggested fix (reorder operations)
- item->setImbuement(freeImbuementSlot, imbuement->getID(), baseImbuement->duration); - - if (g_game().internalRemoveItem(scrollItem, 1) != RETURNVALUE_NOERROR) { + if (g_game().internalRemoveItem(scrollItem, 1) != RETURNVALUE_NOERROR) { g_logger().error("[Player::applyScrollImbuement] - Failed to remove scroll item {} from player {}", scrollItem->getID(), getName()); return; } + + item->setImbuement(freeImbuementSlot, imbuement->getID(), baseImbuement->duration);src/server/network/protocol/protocolgame.cpp-4632-4639 (1)
4632-4639:⚠️ Potential issue | 🟠 MajorFix incorrect foods loop condition (currently can never iterate).
for (uint8_t i; activeFoods < 0; ++i)is effectively dead code becauseactiveFoodsis an unsigned byte and can never be< 0. If/when foods are implemented, this will silently break serialization.Proposed diff
uint8_t activeFoods = 0; // TODO: MODIFY FOODS TO BE LIKE CONCOCTIONS msg.addByte(activeFoods); - for (uint8_t i; activeFoods < 0; ++i) { + for (uint8_t i = 0; i < activeFoods; ++i) { msg.add<uint16_t>(0x00); // Item ID msg.addByte(0x00); msg.addByte(0x00); msg.add<uint32_t>(0x00); // Time Left }src/server/network/protocol/protocolgame.cpp-2292-2324 (1)
2292-2324:⚠️ Potential issue | 🟠 MajorAdd old-protocol guard + validate picked item identity for containers.
Two concerns here:
- This handler should likely ignore
oldProtocolclients (like most other “new UI” parses do).- For container items, you don’t verify that the retrieved item matches
itemId(you do for inventory items). That opens the door to client/server desync and potentially selecting a different item than the one the client claims.Proposed diff
void ProtocolGame::parseImbuementAction(NetworkMessage &msg) { + if (oldProtocol) { + return; + } + auto action = static_cast<ImbuementAction>(msg.getByte()); // 0x01 - pick item, 0x02 - scroll imbuement std::shared_ptr<Item> item = nullptr; if (action == ImbuementAction::PickItem) { msg.skipBytes(2); // Unknown bytes auto slotId = msg.getByte(); msg.skipBytes(1); // Unknown byte msg.skipBytes(1); // Unknown byte auto itemId = msg.get<uint16_t>(); auto slotIndex = msg.getByte(); - if (Item::items[itemId].imbuementSlot <= 0) { - return; - } - if (slotId >= 0x40) { // Padding to not conflict with inventory slot // the client sends with this padding slotId -= 0x40; const auto &container = player->getContainerByID(slotId); item = container ? container->getItemByIndex(slotIndex) : nullptr; } else { item = player->getInventoryItem(static_cast<Slots_t>(slotId)); - if (!item || item->getID() != itemId) { - return; - } } + + if (!item || item->getID() != itemId || item->getImbuementSlot() == 0) { + return; + } } openImbuementWindow(action, item); }src/server/network/protocol/protocolgame.cpp-2326-2355 (1)
2326-2355:⚠️ Potential issue | 🟠 MajorFix unconditional
clearAllStats()clearing all applied proficiency bonuses when editing non-equipped weapons.In the
action == 0x02path,player->weaponProficiency().clearAllStats()is called unconditionally, which clears all applied bonuses across all weapons globally. If the player edits perks for a non-equipped weapon, this wipes the currently-equipped weapon's applied proficiency bonuses, which aren't reapplied. The global state must only be cleared when editing the equipped weapon, following the same pattern as theaction == 0x03path.Additionally, this parse should validate the
actioncode and likely no-op onoldProtocol.Proposed diff
void ProtocolGame::parseWeaponProficiency(NetworkMessage &msg) { + if (oldProtocol) { + return; + } + auto action = msg.getByte(); auto weaponId = msg.get<uint16_t>(); if (action == 0x03) { const auto equippedWeaponId = player->getWeaponId(true); if (equippedWeaponId != 0 && weaponId == equippedWeaponId) { player->weaponProficiency().clearAllStats(); } player->weaponProficiency().clearSelectedPerks(weaponId); auto slots = msg.getByte(); for (uint8_t slot = 0; slot < slots; slot++) { auto level = msg.getByte(); auto perkIndex = msg.getByte(); player->weaponProficiency().setSelectedPerk(level, perkIndex, weaponId); } if (equippedWeaponId != 0 && weaponId == equippedWeaponId) { player->weaponProficiency().applyPerks(weaponId); } } else if (action == 0x02) { - player->weaponProficiency().clearAllStats(); - player->weaponProficiency().clearSelectedPerks(weaponId); + const auto equippedWeaponId = player->getWeaponId(true); + if (equippedWeaponId != 0 && weaponId == equippedWeaponId) { + player->weaponProficiency().clearAllStats(); + player->weaponProficiency().clearSelectedPerks(weaponId); + player->weaponProficiency().applyPerks(weaponId); + } else { + player->weaponProficiency().clearSelectedPerks(weaponId); + } + } else { + return; } sendWeaponProficiencyWindow(weaponId); sendWeaponProficiency(weaponId); }
🟡 Minor comments (15)
data-otservbr-global/scripts/actions/object/proficiency_catalyst.lua-12-12 (1)
12-12:⚠️ Potential issue | 🟡 MinorInconsistent indentation: spaces instead of tabs.
Line 12 uses spaces for indentation while the rest of the file uses tabs.
- local weaponId = target:getId() + local weaponId = target:getId()data/scripts/talkactions/god/weapon_proficiency.lua-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorThe
weaponId=0behavior is intentional, but add confirmation feedback to the god.The
addWeaponExperience()call withweaponId=0is designed to add experience to the player's currently equipped weapon (line 509 of weapon_proficiency.cpp). However, the command will fail silently if the player has no weapon equipped—the error is only logged server-side and never reported back. Consider adding a confirmation message (or error message if no weapon is equipped) so the god knows whether the command succeeded.src/lua/functions/creatures/player/player_functions.cpp-3327-3341 (1)
3327-3341:⚠️ Potential issue | 🟡 MinorMake
openImbuementWindowreturn deterministic (avoid “returns last argument” behavior).With the new optional params (Line 3328–3339), returning
1without pushing means Lua receives different return values depending on the arg count (player vs action vs item). Suggest pushingtrueon success (or returning0if you want a void API).Proposed tweak (explicit success boolean)
const ImbuementAction action = Lua::getNumber<ImbuementAction>(L, 2, ImbuementAction::Open); const auto &item = Lua::getUserdataShared<Item>(L, 3, "Item"); player->openImbuementWindow(action, item); - return 1; + Lua::pushBoolean(L, true); + return 1; }src/lua/functions/creatures/player/player_functions.cpp-5246-5260 (1)
5246-5260:⚠️ Potential issue | 🟡 Minor
addWeaponExperienceshould push a deterministic return value and handle missing player consistently.On both failure (Line 5249–5252) and success (Line 5257–5259), the function returns
1but doesn’t push a value, so Lua receives an arbitrary argument as the return value.Proposed fix (explicit boolean + no-op for 0 exp)
const auto &player = Lua::getUserdataShared<Player>(L, 1, "Player"); if (!player) { Lua::reportErrorFunc(Lua::getErrorDesc(LUA_ERROR_PLAYER_NOT_FOUND)); + Lua::pushBoolean(L, false); return 1; } const auto experience = Lua::getNumber<uint32_t>(L, 2); const auto itemId = Lua::getNumber<uint16_t>(L, 3, 0); + if (experience == 0) { + Lua::pushBoolean(L, true); + return 1; + } player->weaponProficiency().addExperience(experience, itemId); - return 1; + Lua::pushBoolean(L, true); + return 1; }data/items/items.xml-84782-84823 (1)
84782-84823:⚠️ Potential issue | 🟡 MinorItems 51268 and 51269 are missing
articleandprimarytypeattributes, unlike all other new norcferatu items.All other norcferatu items in this batch (51260–51267) declare
articleandprimarytype. These two boots (norcferatu goretrampersandnorcferatu fangstompers) omit both. This may affect client display (no article → no "a"/"an" prefix in messages) and item categorization/filtering.src/items/items.cpp-249-259 (1)
249-259:⚠️ Potential issue | 🟡 MinorGuard against silent proficiency→weapon overwrites.
If multiple items share the same
proficiency_id,proficiency.weaponIdis overwritten without warning. Consider logging or guarding to catch data issues.🔧 Suggested guard
} else { iType.proficiencyId = proficiencyId; auto &proficiency = it->second; - proficiency.weaponId = iType.id; + if (proficiency.weaponId != 0 && proficiency.weaponId != iType.id) { + g_logger().warn("[Items::loadFromProtobuf] - Proficiency ID '{}' already mapped to item id '{}'", proficiencyId, proficiency.weaponId); + } else { + proficiency.weaponId = iType.id; + } }src/enums/weapon_proficiency.hpp-44-46 (1)
44-46:⚠️ Potential issue | 🟡 MinorTypo: "porcentage" should be "percentage".
Lines 44–46 contain the same misspelling in the comments.
Proposed fix
- SKILL_PERCENTAGE_AUTO_ATTACK = 25, // a porcentage of your current skill level as extra damage on auto attacks - OK - SKILL_PERCENTAGE_SPELL_DAMAGE = 26, // a porcentage of your current skill level as extra damage for spells - OK - SKILL_PERCENTAGE_SPELL_HEALING = 27, // a porcentage of your current skill level as extra healing for spells - OK + SKILL_PERCENTAGE_AUTO_ATTACK = 25, // a percentage of your current skill level as extra damage on auto attacks + SKILL_PERCENTAGE_SPELL_DAMAGE = 26, // a percentage of your current skill level as extra damage for spells + SKILL_PERCENTAGE_SPELL_HEALING = 27, // a percentage of your current skill level as extra healing for spellssrc/creatures/players/imbuements/imbuements.cpp-170-173 (1)
170-173:⚠️ Potential issue | 🟡 MinorPotential silent truncation:
as_uint()returnsunsigned intbutscrollIdisuint16_t.
attr.as_uint()returns a 32-bit unsigned value. If the XML value exceeds 65535, the assignment touint16_t scrollIdsilently truncates. Consider usingpugi::cast<uint16_t>(attr.value())for consistency with other parsers in this file (e.g., Line 100, Line 179), or add a range check.Proposed fix
- imbuement.scrollId = attr.as_uint(); + imbuement.scrollId = pugi::cast<uint16_t>(attr.value());src/enums/weapon_proficiency.hpp-163-191 (1)
163-191:⚠️ Potential issue | 🟡 MinorTypo and non-idiomatic types in
WeaponProficiencySpellsstructs.
- Line 168:
aditionalTargetshould beadditionalTarget.- Lines 168, 170, 176–178: Plain
intis used instead ofint32_t, which is the convention throughout this codebase.Proposed fix
struct Increase { bool area = false; double_t damage = 0; double_t heal = 0; - int aditionalTarget = 0; + int32_t additionalTarget = 0; double_t damageReduction = 0; - int duration = 0; + int32_t duration = 0; double_t criticalDamage = 0; double_t criticalChance = 0; }; struct Decrease { - int cooldown = 0; - int manaCost = 0; - int secondaryGroupCooldown = 0; + int32_t cooldown = 0; + int32_t manaCost = 0; + int32_t secondaryGroupCooldown = 0; };src/enums/weapon_proficiency.hpp-1-8 (1)
1-8:⚠️ Potential issue | 🟡 MinorCopyright year says "2019-2024" — should match the project convention.
Other files in this PR use
2019–present. Consider updating for consistency.Proposed fix
-* Copyright (©) 2019-2024 OpenTibiaBR <opentibiabr@outlook.com> +* Copyright (©) 2019–present OpenTibiaBR <opentibiabr@outlook.com>src/creatures/players/player.cpp-2691-2695 (1)
2691-2695:⚠️ Potential issue | 🟡 MinorAvoid sending the imbuement window twice on failure.
The failure path calls
openImbuementWindowtwice; this can cause duplicate UI updates or flicker. Keep only one call.src/server/network/protocol/protocolgame.cpp-2195-2200 (1)
2195-2200:⚠️ Potential issue | 🟡 MinorRemove unused locals in INSPECT_PROFICIENCY branch.
unknownByteandexperiencearen’t used; prefer consuming the byte without binding and avoid the redundantgetExperience()call.Proposed diff
} else if (inspectionType == INSPECT_PROFICIENCY) { - auto itemId = msg.get<uint16_t>(); - auto unknownByte = msg.getByte(); - auto experience = player->weaponProficiency().getExperience(itemId); + const auto itemId = msg.get<uint16_t>(); + msg.getByte(); // Unknown byte sendWeaponProficiencyWindow(itemId); }src/server/network/protocol/protocolgame.cpp-8602-8630 (1)
8602-8630:⚠️ Potential issue | 🟡 MinorRemove unused
categoryImbuement(and consider defensive validation strategy).
categoryImbuementis currently unused (potential warning). Also,g_imbuements().getImbuement()is assumed non-null; that may be fine, but if you ever harden this, prefer filtering/validating before emitting the imbuement list so the message structure stays consistent.Proposed diff
Imbuement* imbuement = g_imbuements().getImbuement(imbuementID); const BaseImbuement* baseImbuement = g_imbuements().getBaseByID(imbuement->getBaseID()); - const CategoryImbuement* categoryImbuement = g_imbuements().getCategoryByID(imbuement->getCategory()); msg.add<uint32_t>(imbuement->getID());src/server/network/protocol/protocolgame.cpp-10500-10519 (1)
10500-10519:⚠️ Potential issue | 🟡 MinorClamp
selectedPerks.size()when serializing into a byte.
msg.addByte(selectedPerks.size())will truncate if the container ever grows >255. Likely small today, but it’s easy to harden.Proposed diff
const auto &selectedPerks = player->weaponProficiency().getSelectedPerks(weaponId); - msg.addByte(selectedPerks.size()); - if (selectedPerks.size() > 0) { - for (const auto &perk : selectedPerks) { - msg.addByte(perk.level); - msg.addByte(perk.index); - } - } + const uint8_t perkCount = static_cast<uint8_t>(std::min<size_t>(selectedPerks.size(), std::numeric_limits<uint8_t>::max())); + msg.addByte(perkCount); + for (size_t i = 0; i < perkCount; ++i) { + const auto &perk = selectedPerks[i]; + msg.addByte(perk.level); + msg.addByte(perk.index); + }src/server/network/protocol/protocolgame.cpp-8632-8659 (1)
8632-8659:⚠️ Potential issue | 🟡 MinorClamp inventory+stash counts to uint16_t max to prevent overflow.
neededItemsisphmap::flat_hash_map<uint16_t, uint16_t>, but you adduint32_tstash counts into it without bounds checking. When inventory + stash exceedsuint16_t::max(), the values silently truncate.The scroll mode call with
nullptritem is intentional and supported bygetImbuements()— the conditional checks inside that function safely guard againstnullptr.Proposed diff
const auto &imbuementItems = imbuement->getItems(); for (const auto &[id, _] : imbuementItems) { if (!neededItems.count(id)) { - neededItems[id] = player->getItemTypeCount(id); - uint32_t stashCount = player->getStashItemCount(id); - if (stashCount > 0) { - neededItems[id] += stashCount; - } + const uint32_t invCount = player->getItemTypeCount(id); + const uint32_t stashCount = player->getStashItemCount(id); + const uint32_t total = invCount + stashCount; + neededItems[id] = static_cast<uint16_t>(std::min<uint32_t>(total, std::numeric_limits<uint16_t>::max())); } }
🧹 Nitpick comments (11)
data/scripts/lib/register_achievements.lua (1)
562-567: Missing gap comments for IDs 563 and 567.The file consistently marks skipped IDs with
-- [N] = Unknown/non-existent(e.g., lines 196, 221, 276, etc.), but IDs 563 and 567 are silently skipped here. Add the conventional comments for consistency and to signal these gaps are intentional.Proposed fix
[562] = { name = "Alpha Rider", grade = 1, points = 2, description = "You never can tell, who's boss here." }, + -- [563] = Unknown/non-existent [564] = { name = "The First of Many", grade = 1, points = 3, description = "The first step to greatness has been done." }, [565] = { name = "A Well-Honed Arsenal", grade = 2, points = 5, description = "Your expertise in weaponry reaches new heights." }, [566] = { name = "Arsenal of War", grade = 3, points = 7, description = "Feared by your enemies, admired by many - your reputation in weapon mastery precedes you." }, + -- [567] = Unknown/non-existent [568] = { name = "Bat Person", grade = 1, points = 3, description = "No matter your command, this beast is all ears!" },src/creatures/monsters/monster.cpp (1)
2372-2380: Inconsistent zero-check between bosstiary and bestiary experience.Line 2373 uses an explicit
> 0comparison, while line 2378 uses an implicit truthiness check. Use the same style for consistency and clarity.Proposed fix
const auto weaponExperienceFromBestiary = targetPlayer->weaponProficiency().getBestiaryExperience(m_monsterType->info.bestiaryStars); - if (weaponExperienceFromBestiary) { + if (weaponExperienceFromBestiary > 0) { targetPlayer->weaponProficiency().addExperience(weaponExperienceFromBestiary); }data-otservbr-global/scripts/actions/object/greater_proficiency_catalyst.lua (1)
1-22: Consider extracting a shared helper to reduce duplication withproficiency_catalyst.lua.Both catalyst scripts are near-identical — differing only in experience amount (25000 vs 100000) and item ID. A shared factory or helper function would reduce maintenance burden.
Example approach
-- In a shared lib or at the top of each file: local function createProficiencyCatalyst(itemId, experience) local catalyst = Action() function catalyst.onUse(player, item, fromPosition, target, toPosition, isHotkey) if not item or not item:isItem() then return false end if not target or not target:isItem() then return false end local weaponId = target:getId() -- TODO: validate weaponId is a weapon player:addWeaponExperience(experience, weaponId) item:remove(1) player:getPosition():sendMagicEffect(CONST_ME_MAGIC_GREEN) return true end catalyst:id(itemId) catalyst:register() end createProficiencyCatalyst(51588, 25000) createProficiencyCatalyst(51589, 100000)data-otservbr-global/scripts/actions/object/etcher.lua (1)
14-18: No feedback to the player whenclearAllImbuementsfails.If the target has no imbuements or isn't imbuable, the action silently does nothing. Consider adding an else branch with a cancel message so the player understands why the etcher wasn't consumed.
Suggested improvement
if clearImbuements then player:sendTextMessage(MESSAGE_EVENT_ADVANCE, "You have cleared all imbuements from the item.") item:remove(1) player:getPosition():sendMagicEffect(CONST_ME_MAGIC_GREEN) + else + player:sendCancelMessage("This item has no imbuements to clear.") endsrc/utils/tools.hpp (1)
201-203:static inlinein header produces per-TU copies.While functionally correct, using
static inlinein a shared header means each translation unit gets its own copy. For a trivial function this is fine, but idiomatic C++17 would use justinline(implicitly extern linkage, ODR-safe) to avoid potential bloat.♻️ Suggested change
-static inline double quadraticPoly(double a, double b, double c, double x) { +inline double quadraticPoly(double a, double b, double c, double x) { return a * x * x + b * x + c; }config.lua.dist (1)
93-96: Consider adding a brief NOTE explaining the values.Other config sections include
NOTE:comments explaining units, valid ranges, and effects. A brief explanation of what these values represent (e.g., "0.05 = 5% base critical hit chance", "0.1 = 10% base critical extra damage") would improve consistency and help server operators tune them correctly.📝 Suggested documentation
-- Player base critical +-- NOTE: playerBaseCriticalChance = base critical hit chance (0.05 = 5%) +-- NOTE: playerBaseCriticalDamage = base critical extra damage multiplier (0.1 = 10%) playerBaseCriticalChance = 0.05 playerBaseCriticalDamage = 0.1src/io/iologindata.cpp (1)
290-291: Consistent with existing KV-save pattern, but consider error handling for robustness.The
weaponProficiency().saveAll()call returnsvoidand follows the same fire-and-forget pattern aswheel().saveRevealedGems(),saveActiveGems(),saveKVModGrades(), andsaveKVScrolls(). However, the asymmetry is worth noting: more critical database-direct operations likeforgeHistory().save(),savePlayerBosstiary(), andsavePlayerStorage()all returnbooland throw on failure, while KV-based saves silently ignore potential failures.Since the underlying KV store operations can fail, consider whether
saveAll()should explicitly propagate errors (returnbooland throw on failure) to match the pattern of other critical saves in this function, especially given that data loss would be silent.src/creatures/players/imbuements/imbuements.cpp (1)
39-47: Linear scan to find imbuement by scroll ID.This iterates the entire
imbuementMapon every call. With a small number of imbuements this is fine, but if this is called in hot paths (e.g., per-packet processing), consider maintaining a secondaryscrollId → Imbuement*lookup map populated duringloadFromXml.src/enums/weapon_proficiency.hpp (1)
107-120: Consider named constants for magic numbers 6 and 10.
ProficiencyLevelhas a fixed array of 6 perks andProficiencyhas 10 levels. Extracting these as named constants improves readability and makes future changes safer.Proposed refactor
+static constexpr uint8_t MAX_PERKS_PER_LEVEL = 6; +static constexpr uint8_t MAX_PROFICIENCY_LEVELS = 10; + struct ProficiencyLevel { - std::array<ProficiencyPerk, 6> perks = {}; + std::array<ProficiencyPerk, MAX_PERKS_PER_LEVEL> perks = {}; }; struct Proficiency { Proficiency() = default; Proficiency(uint16_t id, uint16_t weaponId) : id(id), weaponId(weaponId) { } uint16_t id = 0; - std::array<ProficiencyLevel, 10> level = {}; + std::array<ProficiencyLevel, MAX_PROFICIENCY_LEVELS> level = {}; uint16_t weaponId = 0; uint8_t maxLevel = 0; };src/server/network/protocol/protocolgame.cpp (2)
294-312: Critical aggregation helper is a good consolidation; consider a small naming polish.
concotionCriticallooks like a typo (probablyconcoctionCritical), but the flow/serialization order is clear.
8661-8720: Scroll availability flag ignores stash, while neededItems includes stash—confirm intended UX.
emptyImbuementScrollsis computed only from inventory, but later you compute needed-items counts by inventory+stash. If stash scrolls are supposed to count, this “has scroll” flag may be misleading.Also, once you clamp counts in
addAvailableImbuementsInfo, the finalneededItemsserialization becomes safer.
Introduce weapon proficiency support and related helpers, plus multiple bugfixes and refactors. - Add proficiency helper script (createProficiencyCatalyst) and replace duplicated catalyst actions. - Add weapon proficiency config keys and enums; load settings via ConfigManager. - Implement/extend WeaponProficiency: JSON loading, KV (de)serialization, experience arrays as vectors, perk/level limits, safer parsing, various logic fixes (boss/forge checks, bestiary clamping, mastery calculations), skill percentage mapping, and API tweaks (saveAll returns bool). - Add new proficiency JSON loader error handling to throw on failure. - Fix save_interval global event: robust time parsing, scheduling, broadcasting and return values. - Imbuements: map scroll IDs for fast lookup, fix parsing, reload handling, and ordering when applying scrolls; small gameplay fix for imbuement failure flow. - Items XML: correct augments count and add primarytype/article attributes for boots. - Fix typos (aditionalTarget -> additionalTarget) across wheel and spell augment code and related header changes. - UI/UX and safety fixes: feedback message for weapon_proficiency god talk action and etcher cancel message. - Misc: small includes and header adjustments, logging improvements, and various defensive checks to avoid crashes. These changes improve weapon proficiency functionality, remove duplicated code, and harden several systems for stability and correctness. Co-Authored-By: Felipe <87909998+FelipePaluco@users.noreply.github.com> Co-Authored-By: Pedro Cruz <pedro.ha.cruz2022@gmail.com>
Add validation to weapon proficiency handling to skip entries with invalid or unmapped item IDs and log warnings (WeaponProficiency::load, addExperience, and the Lua API). Remove legacy behavior that mapped the first item to a Proficiency (Proficiency::weaponId removed, constructor adjusted, and Items::loadFromProtobuf no longer assigns weaponId). These changes prevent using items without a defined proficiency and avoid silent, implicit mappings.
Update items.xml to remove 'paralysis removal' imbuement attributes from several items (including removing an imbuementslot block where it was the only entry). Also correct the 'wound cleansing' augment attribute from value="base" to value="base damage" (value stays 17). These changes adjust imbuement/augment definitions and fix an attribute type inconsistency.
Add an optional monsterName field and constructor to Loot, propagate the monster name when creating parent and child loot in register_monster_type.lua, and update the Lua binding to accept Loot(monsterName). Enhance item-lookup warnings in luaLootSetIdFromName to include the monster context so missing or duplicate item names can be diagnosed per-monster. Changes touch data/scripts/lib/register_monster_type.lua, src/creatures/monsters/monsters.hpp, and src/lua/functions/creatures/monster/loot_functions.cpp.
Replace name-based "transcendence potion" loot entries with id = 49271 across multiple monster and boss loot files to standardize item references and avoid issues with name lookups. Added inline comments for clarity. Affected files include various boss/quest monsters (e.g. the_brainstealer, the_monster, feaster_of_souls, grave_danger bosses, timira_the_many-headed, magma_bubble, soul_war entries, the_dream_courts bosses, drume, ghulosh).
Add include for creatures/players/components/weapon_proficiency.hpp to src/items/items.cpp. This brings in the weapon proficiency declarations needed by the items implementation (prevents missing-symbol/compile issues and enables use of proficiency types/functions).
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lua/functions/creatures/player/player_functions.cpp (1)
3305-3325:⚠️ Potential issue | 🔴 CriticalMissing return value push on the success path — Lua caller gets an undefined value.
After
player->applyScrollImbuement(item, scrollItem)on line 3322, the function returns 1 but never pushes a result onto the Lua stack. The error paths correctly pushfalse, but the success path leaves the stack without a return value. This means the Lua caller will receive whatever happens to be on the top of the stack (likely thescrollItemuserdata), which is incorrect.🐛 Proposed fix: push a boolean result on the success path
player->applyScrollImbuement(item, scrollItem); - + Lua::pushBoolean(L, true); return 1; }Alternatively, if
applyScrollImbuementreturns a success/failure indicator, push that instead.src/lua/functions/creatures/monster/loot_functions.cpp (1)
40-62:⚠️ Potential issue | 🔴 CriticalFix the argc check to allow
Loot()calls with the default constructor.The condition
argc >= 1is off-by-one for this Lua binding framework. When a class constructor is registered viaregisterSharedClass, the class table occupies stack position 1, so user arguments start at position 2. This means:
Loot()with no args:lua_gettop(L) = 1, enters the if block, then fails onisString(L, 2)checking nil → hard errorLoot("name"):lua_gettop(L) = 2, works correctlyChange to
argc >= 2to properly distinguish the two cases and allow the default constructor path to execute.Proposed fix
const int argc = lua_gettop(L); std::shared_ptr<Loot> loot; -std::string monsterName; -if (argc >= 1) { +if (argc >= 2) { if (!Lua::isString(L, 2)) { luaL_error(L, "Loot([monsterName]) expects argument `#1` to be a string"); return 0; } - monsterName = Lua::getString(L, 2); + auto monsterName = Lua::getString(L, 2); loot = std::make_shared<Loot>(monsterName); } else { loot = std::make_shared<Loot>(); }(The
monsterNamescope reduction addresses a cppcheck hint.)src/creatures/players/imbuements/imbuements.cpp (1)
380-386:⚠️ Potential issue | 🟠 MajorPre-existing: operator precedence bug in storage check (not introduced by this PR).
Line 383:
player->getStorageValue(imbuement->getStorage() == -1)— the== -1binds togetStorage()rather than to the result ofgetStorageValue(...). This passestrue/false(i.e., storage key 0 or 1) togetStorageValueinstead of the actual storage key. The likely intent was:player->getStorageValue(imbuement->getStorage()) == -1This is pre-existing and out of scope for this PR, but worth fixing.
🤖 Fix all issues with AI agents
In `@data/items/items.xml`:
- Around line 84927-84929: The item element with id "51443" (name "etcher") is
missing the required primarytype attribute; add a primarytype attribute to the
<item id="51443" ...> element (e.g., primarytype="other items" or the correct
category for etchers) so it matches the other new items and will be included in
filters/shops; update the <item id="51443" ...> tag to include primarytype="..."
alongside article="a" and ensure the attribute value uses the same taxonomy
strings used elsewhere.
- Around line 77342-77345: The attribute for the "wound cleansing" spell
currently uses key="base damage" but should use key="base healing"; update the
nested attribute under attribute key="wound cleansing" to use "base healing"
(and keep the same numeric value attribute key="value" value="17") so it matches
the upgraded "intense wound cleansing" and other healing spells like "spirit
mend".
In `@data/scripts/globalevents/save_interval.lua`:
- Around line 50-73: The save.onTime function schedules duplicate serverSave
events when called repeatedly inside the WARNING window; add a module-level
guard (e.g., saveScheduled boolean or scheduledSaveEventId) checked at the top
of save.onTime before calling addEvent(serverSave, ...), set the guard when you
schedule the event, and ensure serverSave clears the guard (or clear it when the
scheduled event runs/after the save completes) so only one scheduled save exists
per cycle; reference save.onTime, WARNING, serverSave, addEvent and the
timeLeft/saveTimeStr logic when implementing the guard.
In `@data/scripts/lib/proficiency_helper.lua`:
- Around line 1-28: In createProficiencyCatalyst's catalyst.onUse, handle the
boolean return from player:addWeaponExperience( experience, weaponId ): call
addWeaponExperience and store its result, and only proceed to consume the
catalyst (item:remove(1)) and trigger
player:getPosition():sendMagicEffect(CONST_ME_MAGIC_GREEN) when that result is
true; if it returns false, send the player a clear cancel message (e.g. via
player:sendCancelMessage) and do not remove the item. Ensure you reference
catalyst.onUse, addWeaponExperience, item:remove, and sendMagicEffect when
making the change.
In `@src/creatures/players/components/weapon_proficiency.cpp`:
- Around line 906-928: Remove the unused local variable by editing
WeaponProficiency::getSkillValueFromWeapon: delete the declaration "uint16_t
skill = 0;" (it is never read) and keep the existing early returns based on
Item::items[weapon->getID()].type that call m_player.getSkillLevel(...) or
m_player.getMagicLevel(); ensure no other references to "skill" remain in the
function.
- Around line 656-664: The loop can read past experienceArray because it
iterates to proficiencyInfo.maxLevel without checking experienceArray.size();
change the loop to clamp the upper bound to the smaller of
experienceArray.size() and proficiencyInfo.maxLevel (e.g. compute a size_t limit
= std::min(experienceArray.size(),
static_cast<size_t>(proficiencyInfo.maxLevel)) and iterate for (size_t i = 0; i
< limit; ++i)); keep references to playerProficiency.experience and
playerProficiency.perks logic the same but switch i to a size_t to match the
clamped limit.
- Around line 692-713: The getter getSkillPercentage() returns an unused member
m_skillPercentage while addSkillPercentage() updates the map m_skillPercentages,
so the caller gets stale defaults; fix by changing the API to return the correct
entry from the map: update WeaponProficiency::getSkillPercentage to accept a
skills_t parameter (e.g., getSkillPercentage(skills_t skill) const) and return
the corresponding SkillPercentage from m_skillPercentages (or a const
reference/default if not present), and remove m_skillPercentage if it becomes
unused; ensure callers (where protocolgame.cpp requests skill percentages) are
updated to pass the skill key and use the new getter.
In `@src/creatures/players/components/weapon_proficiency.hpp`:
- Around line 136-137: The member m_skillPercentage is dead state while
m_skillPercentages holds the actual data; update the class to either remove
m_skillPercentage or make getSkillPercentage() return the correct map entry.
Specifically, either delete the field m_skillPercentage and change
getSkillPercentage() to return a reference/value from m_skillPercentages (e.g.,
find/insert the skills_t key and return that SkillPercentage), or keep
m_skillPercentage but change all places that write to m_skillPercentages (in
functions that currently update the map) to also update m_skillPercentage so the
getter remains valid; target the symbols m_skillPercentage, m_skillPercentages,
and getSkillPercentage() when making the change.
In `@src/creatures/players/player.cpp`:
- Around line 2486-2495: applyScrollImbuement sets the imbuement but never
starts its decay, so replicate the regular apply flow: after
item->setImbuement(freeImbuementSlot, imbuement->getID(),
baseImbuement->duration) call the same decay-start helper used by the normal
imbuement path (e.g., startImbuementDecay(item, freeImbuementSlot) or the
project’s equivalent function that registers/starts the imbuement timer) so the
scroll-applied imbuement will expire; keep the addItemImbuementStats(imbuement)
call for items whose parent is the player.
- Around line 7233-7252: The code currently falls back to perfectShotBonus.range
so items lacking a perfect-shot ability can match the range and still consume
charges; fix by only applying perfect-shot effects and consuming charges when
the item actually defines the perfect-shot ability (i.e., itemType.abilities
exists and itemType.abilities->perfectShotRange is non-zero), or alternatively
require itemType.abilities->perfectShotDamage to be non-zero before adding
damage and transforming the item; update the conditional around the perfect-shot
block (the check that handles perfectShotRange == range, the itemType.abilities
pointer, perfectShotDamage, useCharges, item->getCharges(), and
g_game().transformItem(...)) to gate charge consumption to items that explicitly
provide a perfect-shot ability/damage.
In `@src/server/network/protocol/protocolgame.cpp`:
- Around line 10496-10506: ProtocolGame::sendWeaponProficiency currently emits
opcode 0x5C unconditionally which breaks oldProtocol clients and can crash if
player is null; add an early guard at the top of
ProtocolGame::sendWeaponProficiency that returns immediately when oldProtocol is
true (and also when player == nullptr) so the message is never constructed or
written via writeToOutputBuffer; keep the rest of the logic unchanged so
weaponProficiency().getExperience and isUpgradeAvailable are only accessed when
player is valid and the protocol supports this opcode.
- Around line 1836-1839: The code reads imbuementId with the wrong type causing
implicit truncation: change the read from msg.get<uint32_t>() to
msg.get<uint16_t>() so the type matches the playerApplyImbuement signature
(playerApplyImbuement(player->getID(), imbuementId, slot)) and avoids truncating
IDs; update the variable declaration for imbuementId accordingly and ensure any
use of imbuementId in this scope is treated as uint16_t.
- Around line 4490-4519: The code currently computes cipbiaSkill =
static_cast<uint8_t>(getCipbiaSkill(skillPercentage.skill)) unconditionally;
defer this call and the cipbiaSkill variable into each branch that actually
needs it to avoid unnecessary work. Specifically, remove the early cipbiaSkill
initialization and inside the hasAutoAttackSkill, hasSpellDamage, and
hasSpellHealing blocks call getCipbiaSkill(skillPercentage.skill) (casting to
uint8_t as needed) right before using it; keep existing logic for checking
skillPercentage.skill != SKILL_NONE and using playerSkill and skillPercentage
values unchanged.
In `@src/utils/tools.cpp`:
- Around line 71-77: The undoShift function should guard against value == 0
because std::countr_zero(0) yields the bit width and produces an incorrect
result; update undoShift to first check if value == 0 and return 0 in that case,
otherwise compute auto trailingZeros = std::countr_zero(value) and keep the
existing logic (return 0 if trailingZeros < 2, else return trailingZeros - 2) so
that std::countr_zero is never invoked or relied on for the zero input;
references: function undoShift and std::countr_zero.
🧹 Nitpick comments (11)
src/items/items_definitions.hpp (1)
277-299: Enum values look intentional but the gaps at 10–11 are undocumented.Values 10 and 11 are skipped between
IncreasedDamageReduction = 9andEnhancedEffect = 12. The other "not implemented" entries have comments — consider adding a brief comment for the gap (e.g.,// 10, 11 reserved) so future maintainers don't accidentally assign those values.src/creatures/players/imbuements/imbuements.cpp (2)
329-332: Duplicate scroll IDs are silently overwritten.If two imbuements share the same
scrollId, the second one silently overwrites the first inscrollIdMap. Consider logging a warning to aid debugging of misconfigured data.Proposed fix
if (imbuement.scrollId != 0) { - scrollIdMap[imbuement.scrollId] = &imbuement; + auto [it, inserted] = scrollIdMap.emplace(imbuement.scrollId, &imbuement); + if (!inserted) { + g_logger().warn("Duplicate scroll ID {} for imbuement '{}', already mapped to '{}'", imbuement.scrollId, imbuement.name, it->second->getName()); + it->second = &imbuement; + } }
370-378: Null pointer check on line 372 is dead code.
imbuementis assigned as&valuefrom the map iterator — it can never benullptr. The checkif (!imbuement)will never trigger.data/items/items.xml (1)
76122-76129: Five augments with incorrect count declaration (value="1"for 2 augments).Items sanguine coil, grand sanguine coil, sanguine rod, grand sanguine rod, and stoic iks faulds declare
augments value="1"but contain 2 augment entries each. While the count declaration itself is pre-existing, the augment children are being actively modified in this PR (changing from "base" to "base damage"). Consider fixing the declared count while editing these elements:Proposed fixes
- sanguine coil (line 76122): value="1" → value="2" - grand sanguine coil (line 76163): value="1" → value="2" - sanguine rod (line 76233): value="1" → value="2" - grand sanguine rod (line 76274): value="1" → value="2" - stoic iks faulds (line 76831): value="1" → value="2"Note: A broader scan revealed 20 items total with this mismatch in the file, though only these five are being touched in this PR.
src/creatures/players/components/wheel/player_wheel.cpp (1)
2568-2599: Avoid copying and shadowing inregisterAugment.Passing the set by value and shadowing
spellIdis avoidable noise and extra work.♻️ Suggested refactor
- auto registerAugment = [&](const uint16_t spellId, const std::string &spellName, uint8_t grade, std::unordered_set<uint16_t> registeredSpells) { - for (const auto &spellId : registeredSpells) { + auto registerAugment = [&](const std::string &spellName, uint8_t grade, const std::unordered_set<uint16_t> ®isteredSpells) { + for (const auto ®isteredSpellId : registeredSpells) { if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_SORCERER) { - getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.sorcerer, wheelAugments); + getAugmentsByVocation(registeredSpellId, spellName, grade, ioBonusData.spells.sorcerer, wheelAugments); } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_DRUID) { - getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.druid, wheelAugments); + getAugmentsByVocation(registeredSpellId, spellName, grade, ioBonusData.spells.druid, wheelAugments); } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_PALADIN) { - getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.paladin, wheelAugments); + getAugmentsByVocation(registeredSpellId, spellName, grade, ioBonusData.spells.paladin, wheelAugments); } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_KNIGHT) { - getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.knight, wheelAugments); + getAugmentsByVocation(registeredSpellId, spellName, grade, ioBonusData.spells.knight, wheelAugments); } else if (m_player.getVocation()->getBaseId() == Vocation_t::VOCATION_MONK) { - getAugmentsByVocation(spellId, spellName, grade, ioBonusData.spells.monk, wheelAugments); + getAugmentsByVocation(registeredSpellId, spellName, grade, ioBonusData.spells.monk, wheelAugments); } } };- registerAugment(spellId, spellName, grade, registeredSpells); + registerAugment(spellName, grade, registeredSpells);src/items/items.cpp (1)
260-269: Avoid iterator shadowing for readability.The inner
auto itshadows the outer loop indexit(Line 263). Renaming reduces confusion during debugging.♻️ Suggested rename
- auto it = proficiencies.find(proficiencyId); - if (it == proficiencies.end()) { + auto profIt = proficiencies.find(proficiencyId); + if (profIt == proficiencies.end()) { g_logger().warn("[Items::loadFromProtobuf] - Unknown Proficiency ID '{}'", proficiencyId); } else { iType.proficiencyId = proficiencyId; }src/server/network/protocol/protocolgame.cpp (2)
4644-4651: TODO left in payload path.
If this is not meant to ship, consider tracking it as a task or finishing it before release.Would you like me to open an issue or draft the implementation plan for food effects?
7190-7196: Consider guarding trace logging in this hot path.
pos.toString()andsource_locationstringification happen on every call; if trace is disabled, a guard can avoid the cost.src/creatures/players/components/weapon_proficiency.cpp (3)
873-876:addPerfectShotBonusoverwrites rather than accumulates.Unlike most other
add*methods in this class (which use+=),addPerfectShotBonusdirectly assignsrangeanddamage. If multiple perks grant perfect shot bonuses, only the last one takes effect. If this is intentional (e.g., only one perfect shot perk can be active), consider adding a comment to clarify.
1028-1064:applySkillSpellPercentagemay flip sign for secondary damage even whensecondary.type == COMBAT_NONE.Lines 1061–1062 unconditionally negate both
primary.valueandsecondary.valuewhen!healing. Ifsecondary.typeisCOMBAT_NONEandsecondary.valuewas 0, this is benign. However, ifsecondary.type == COMBAT_NONEbut the value was modified by a prior perk accumulation path (line 1056 only guards on!= COMBAT_NONE, so this shouldn't happen), it could produce unexpected results. The logic appears safe for now but is fragile — consider guarding the negation with the same!= COMBAT_NONEcheck for consistency.
1122-1140: Considerstd::find_ifinstead of a raw loop.Flagged by cppcheck. The loop searches for the first matching perk and returns — a direct candidate for
std::find_if.♻️ Suggested refactor
- const auto &perks = getSelectedPerks(weaponId); - for (const auto &perk : perks) { - if (perk.type == criticalType && perk.element != COMBAT_NONE) { - const auto cipElement = getCipbiaElement(perk.element); - return std::make_pair(cipElement, perk.value); - } - } + const auto &perks = getSelectedPerks(weaponId); + auto it = std::find_if(perks.begin(), perks.end(), [&](const ProficiencyPerk &perk) { + return perk.type == criticalType && perk.element != COMBAT_NONE; + }); + if (it != perks.end()) { + return std::make_pair(getCipbiaElement(it->element), it->value); + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/items/items.cpp`:
- Around line 261-270: The inner variable name "it" in the block that looks up
proficiencies shadows the outer loop counter "it" (uint32_t) and should be
renamed to avoid confusion; change the iterator returned by
WeaponProficiency::getProficiencies() (currently declared as "auto it =
proficiencies.find(proficiencyId);") to a distinct name such as "profIt" or
"proficiencyIt", update the subsequent checks (profIt == proficiencies.end())
and access accordingly, leaving proficiencyId and iType.proficiencyId logic
unchanged.
🧹 Nitpick comments (2)
src/items/items.cpp (2)
110-114: TheBaseDamage/BaseHealingbranch is identical to the default return.Lines 110-111 produce the exact same format string as the fallthrough on line 114. If these are intended to diverge later, a comment noting that would help; otherwise this branch can be removed to reduce dead logic.
♻️ Suggested simplification (if no future divergence is planned)
} else if (augmentInfo->type == Augment_t::Cooldown) { return fmt::format("{} -> {}{}s {}", augmentSpellNameCapitalized, signal, augmentInfo->value / 1000, augmentName); - } else if (augmentInfo->type == Augment_t::BaseDamage || augmentInfo->type == Augment_t::BaseHealing) { - return fmt::format("{} -> {:+}% {}", augmentSpellNameCapitalized, augmentInfo->value, augmentName); } return fmt::format("{} -> {:+}% {}", augmentSpellNameCapitalized, augmentInfo->value, augmentName);
141-150: Free function has external linkage — considerstaticor anonymous namespace.
protoItemCategoryToCppis a file-local helper but has external linkage, risking ODR violations if another translation unit defines a symbol with the same name.♻️ Suggested fix
-ItemTypes_t protoItemCategoryToCpp(uint32_t protoCategory) { +static ItemTypes_t protoItemCategoryToCpp(uint32_t protoCategory) {
Multiple improvements and fixes across the codebase: - data/items: Fix wound cleansing augment to use base healing and add primarytype="tools" for etcher (id 51443). - save_interval: Prevent duplicate save scheduling via isSaveScheduled flag. - proficiency_helper: Only consume catalyst and show effect when addWeaponExperience succeeds. - canary_server: Use modulesLoadHelper with a string_view identifier and update signature for better logging and error messages. - weapon_proficiency: Large refactor and safety hardening — add [[nodiscard]] annotations, handle JSON key parsing exceptions more accurately, replace .at() and repeated contains() with iterator-based lookups, prevent out-of-range indexing for mastery experience, streamline perk application switch, safe access to spell bonuses and bestiary maps, return defaults where appropriate, minor API changes (getSkillPercentage now takes a skill param) and various micro-optimizations/unused-result annotations. - wheel/player_wheel: Fix variable shadowing in augment registration loop. - imbuements/player: Silence unused-result warnings for emplace_back and start imbuement decay when applying scroll imbuement. - lua/loot_functions: Introduce templated luaLootSetter and luaLootStringSetter to consolidate many similar setter functions and reduce duplication. - protocolgame: Change imbuement id type to uint16_t, guard weapon proficiency messages for nullptr/oldProtocol, determine skill for skillPercentage lookup and use it when building cyclopedia data. - tools: Fix undoShift to handle value == 0 safely. Overall these changes improve safety, reduce duplication, fix bugs, and modernize interfaces.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@data/items/items.xml`:
- Around line 84610-84637: Item id 51263 (norcferatu bloodhide) has an anomalous
<attribute key="weight" value="39000"/>; update the weight attribute for that
item to the intended value (change value from "39000" to "3900") so the
<attribute key="weight"/> under item id="51263" aligns with other sorcerer
armors (e.g., id 51264). Keep the rest of the item block (imbuementslot,
augments, script) unchanged.
In `@src/creatures/players/components/weapon_proficiency.cpp`:
- Around line 541-566: In nextLevelExperience, avoid unsigned underflow when
proficiencyInfo.maxLevel == 0 by adding an early guard: if
proficiencyInfo.maxLevel == 0 return 0 (or set maxExpLevels to 0) before
computing proficiencyInfo.maxLevel - 1; update the computation of maxExpLevels
in WeaponProficiency::nextLevelExperience to use a safe branch rather than
subtracting from an unsigned type. Apply the same pattern to
WeaponProficiency::getMaxExperience so it also checks proficiencyInfo.maxLevel
== 0 and returns 0 (or treats maxExpLevels as 0) before any subtraction to
prevent wraparound.
In `@src/creatures/players/components/wheel/player_wheel.cpp`:
- Around line 2568-2602: The registerAugment lambda currently takes an unused
spellId and a pass-by-value registeredSpells, and you allocate a single-element
unordered_set for every spell — simplify by removing the dead spellId parameter,
change registeredSpells to a const reference if you keep it, or better yet
replace the set+loop with a single uint16_t currentSpellId parameter and call
getAugmentsByVocation directly for that id; also hoist the vocation lookup
(m_player.getVocation()->getBaseId()) outside the per-spell loop and dispatch
once (e.g., switch or if/else) so registerAugment/getAugmentsByVocation are
invoked with the proper ioBonusData.spells.* and wheelAugments without per-spell
allocation.
In `@src/creatures/players/player.cpp`:
- Around line 726-730: The code adds weapon proficiency bonuses to defenseValue
in the weapon branch but then overwrites defenseValue in the shield branch,
causing shield users to lose those bonuses; move the additions from
weaponProficiency().getStat(WeaponProficiencyBonus_t::DEFENSE_BONUS) and
WeaponProficiencyBonus_t::WEAPON_SHIELD_MODIFIER so they are applied after you
compute the final base defense (after choosing between weapon and shield
branches) in the function(s) that set defenseValue (refer to defenseValue,
weapon->getDefense()/getExtraDefense(), shield branch code, and
weaponProficiency()); ensure you also preserve defenseSkill logic
(getWeaponSkill) and apply the same post-selection proficiency additions in the
other affected function(s) around lines 765-769.
- Around line 2620-2621: The user-facing error message passed to
sendImbuementResult contains a typo: change "An error ocurred, please reopen
imbuement window." to "An error occurred, please reopen imbuement window."
Update every occurrence of that string (e.g., the call in player.cpp around the
sendImbuementResult invocation and the similar occurrence at the other location
mentioned) so the message is spelled correctly for players.
In `@src/lua/functions/creatures/monster/loot_functions.cpp`:
- Around line 42-57: The constructor binding reads argc = lua_gettop(L) but
treats any argc >= 1 as having a monsterName, which is wrong because the
metatable/class occupies stack index 1; update the branch in the constructor
(the block using argc, Lua::isString(L, 2), Lua::getString(L, 2), and
std::make_shared<Loot>(monsterName)) to check argc >= 2 instead of argc >= 1 so
that calling Loot() with no explicit args uses std::make_shared<Loot>() and only
validates index 2 when an actual argument is present.
🧹 Nitpick comments (10)
src/lua/functions/creatures/monster/loot_functions.cpp (1)
149-150: Stale changelog-style comment — consider removing.This comment reads like a refactoring note rather than documentation. The functions are already defined above using the template; the comment doesn't add clarity and may confuse future readers.
Proposed fix
-// luaLootSetMinCount and luaLootSetMaxCount are now using the template above -src/creatures/players/imbuements/imbuements.cpp (4)
66-66: Static analysis:unusedBase,unusedCategory,unusedItemreferences should beconst.cppcheck flags these on lines 66, 83, and 193. Since you're only using
[[maybe_unused]]to discard the return, make themconst:Proposed fix
- [[maybe_unused]] auto &unusedBase = basesImbuement.emplace_back( + [[maybe_unused]] const auto &unusedBase = basesImbuement.emplace_back(Apply the same pattern at lines 83 and 193.
168-171: Missing warning when scroll entry lacks avalueattribute.For the
"item"type (line 173-176), a missingvalueattribute logs a warning. The"scroll"branch silently ignores a missing value. Consider logging a warning for consistency.Proposed fix
if (strcasecmp(type.c_str(), "scroll") == 0) { if ((attr = childNode.attribute("value"))) { imbuement.scrollId = pugi::cast<uint16_t>(attr.value()); + } else { + g_logger().warn("Missing scroll ID for imbuement name '{}'", imbuement.name); }
329-332: DuplicatescrollIdvalues silently overwrite — consider warning.If two imbuements share the same
scrollId, the second entry silently overwrites the first inscrollIdMap. This could mask XML configuration errors.Proposed fix
if (imbuement.scrollId != 0) { + if (scrollIdMap.contains(imbuement.scrollId)) { + g_logger().warn("Duplicate scroll ID {} for imbuement '{}', overwriting previous mapping", imbuement.scrollId, imbuement.name); + } scrollIdMap[imbuement.scrollId] = &imbuement; }
370-378: Line 372: null check onimbuementis dead code.
imbuementis&valuewherevalueis a reference to astd::mapelement — it can never benullptr. The check is harmless but misleading.data/scripts/globalevents/save_interval.lua (1)
1-2:saveTimeStris hardcoded to"00:00"— load from config instead.Other globalevents (raids.lua, global_server_save.lua) load the save time from
configManager.getString(configKeys.GLOBAL_SERVER_SAVE_TIME), which defaults to "06:00". This file hardcodes it to midnight, inconsistent with the config-driven pattern used elsewhere and the defined server configuration.Suggested change
-local saveTimeStr = "00:00" +local saveTimeStr = configManager.getString(configKeys.GLOBAL_SERVER_SAVE_TIME) or "00:00"src/creatures/players/components/weapon_proficiency.cpp (4)
568-584: Double lookup onproficienciesmap ingetMaxExperience.Lines 570 and 574 perform
findthenaton the same key. Use the iterator fromfinddirectly to avoid the redundant hash lookup.♻️ Proposed fix
uint32_t WeaponProficiency::getMaxExperience(uint16_t weaponId) const { const auto &experienceArray = getExperienceArray(weaponId); - if (proficiencies.find(Item::items[weaponId].proficiencyId) == proficiencies.end()) { + auto prof_it = proficiencies.find(Item::items[weaponId].proficiencyId); + if (prof_it == proficiencies.end()) { g_logger().error("{} - Proficiency not found for weapon ID: {}", __FUNCTION__, weaponId); return 0; } - const auto &proficiencyInfo = proficiencies.at(Item::items[weaponId].proficiencyId); + const auto &proficiencyInfo = prof_it->second;
276-283: Remove unnecessary[[maybe_unused]]wrapper variable.
unusedPerkis never used — theemplace_backreturn value can simply be discarded. Static analysis also flags this as needingconst.♻️ Proposed fix
for (const auto &item : array) { - [[maybe_unused]] auto &unusedPerk = perks.emplace_back(deserializePerk(item)); + perks.emplace_back(deserializePerk(item)); }
242-248:saveAll()creates a new scoped KV handle per iteration.Each loop iteration calls
m_player.kv()->scoped("weapon-proficiency"), performing a repeated lookup. Cache it before the loop, similar to howload()does it on line 208.♻️ Proposed fix
bool WeaponProficiency::saveAll() const { + auto wp_kv = m_player.kv()->scoped("weapon-proficiency"); for (const auto &[weaponId, weaponData] : proficiency) { - m_player.kv()->scoped("weapon-proficiency")->set(std::to_string(weaponId), serialize(weaponData)); + wp_kv->set(std::to_string(weaponId), serialize(weaponData)); } return true; }
1151-1169: Considerstd::find_iffor the loop.Static analysis suggests replacing the raw loop with
std::find_if. This is a minor readability improvement.♻️ Proposed fix
- const auto &perks = getSelectedPerks(weaponId); - for (const auto &perk : perks) { - if (perk.type == criticalType && perk.element != COMBAT_NONE) { - const auto cipElement = getCipbiaElement(perk.element); - return std::make_pair(cipElement, perk.value); - } - } + const auto &perks = getSelectedPerks(weaponId); + auto it = std::find_if(perks.begin(), perks.end(), [&](const auto &perk) { + return perk.type == criticalType && perk.element != COMBAT_NONE; + }); + if (it != perks.end()) { + return std::make_pair(getCipbiaElement(it->element), it->value); + }
Read global server save time from configManager in save_interval.lua instead of a hardcoded value. In weapon_proficiency.cpp: cache the KV scoped handle, remove unused variable annotations, simplify perks search with std::find_if, and optimize proficiency lookup by using iterator access to avoid double lookup. In imbuements.cpp: make emplaced references const, add warnings for missing or duplicate scroll IDs, and remove an unnecessary null check when iterating imbuements. Minor cleanup in lua loot functions by removing an outdated comment. These changes improve performance, safety, and logging clarity.
Corrects an item weight typo (39000 -> 3900) in items.xml and hardens proficiency_helper.lua by checking addWeaponExperience result before removing the catalyst. Now the item is only consumed on success, a green magic effect is shown, and a cancel message is sent to the player if applying experience fails.
Refactor player_wheel.cpp to simplify the registerAugment lambda: cache the player's vocation base ID, remove the unused unordered_set parameter, and call getAugmentsByVocation directly with the currentSpellId. Also remove the creation/emplacement of registeredSpells at the call site. This eliminates an unused parameter and avoids repeated getVocation() calls while keeping behavior unchanged.
Delete the unused #include <nlohmann/json.hpp> (and an extra blank line) from src/creatures/players/components/weapon_proficiency.cpp to clean up headers and avoid an unnecessary dependency.
Add early guards in nextLevelExperience() and getMaxExperience() to return 0 when proficiencyInfo.maxLevel == 0. This prevents underflow/invalid indexing from computing (proficiencyInfo.maxLevel - 1) and avoids out-of-range access to experienceArray.
Fix merging of spell 'area' bonuses in WeaponProficiency::addSpellBonus. The code previously used a logical OR which prevented numeric area bonuses from stacking; this changes the merge to use += so multiple bonuses properly accumulate. This corrects lost/incorrect area bonus behavior when combining effects.
|
@CodeRabbit help |
|
Tip For best results, initiate chat on the files or code changes.
CodeRabbit doesn't provide formal approvals ✨ As an AI code reviewer, I provide:
However, I don't:
What you should do:
For this specific PR: This is a large feature addition with protocol changes and new weapon proficiency systems. If you haven't seen any critical issues flagged in the review, and you're confident in the changes, you can proceed with merging at your discretion! 🚀 |
Reintroduced loading of weapon proficiency data from JSON.
Removed duplicate loading of 'proficiencies.json' in canary_server.cpp.
Add explicit bounds and enum checks for element-critical handling to prevent out_of_range exceptions and improve clarity. Changes include: - getElementCritical: use const auto for enum conversion and return via operator[] when in-range. - addElementCritical: early-return on COMBAT_NONE, validate index against m_elementCritical size, log out-of-range and avoid try/catch; update entries with operator[]. - applyElementCritical: early-return if damage.primary.type is COMBAT_NONE or >= COMBAT_COUNT before fetching bonuses. These changes replace exception-based control flow with explicit checks and safer indexing.
Extract helper functions to consolidate perk logic and reduce duplication: applyCriticalBonus and applySkillPercentageBonus. Simplify loading and lookup paths (use if-with-init, std::min, vector::back, push_back) and remove unused locals. ClearSelectedPerks now erases proficiency and notifies the player. Update bestiary check to use WEAPON_PROFICIENCY_BESTIARY and add a comment in the enum to avoid shadowing CyclopediaTitle_t::BESTIARY. Overall changes improve readability, maintainability, and correctness of weapon proficiency handling.
The tests were calling getTimeNow() both during account creation and again during assertion. If a DST transition happens between those calls, the 3600s difference exceeds the 60s tolerance. Fix: createAccount now returns the lastDay it computed, and assertAccountLoad receives it as a parameter instead of recalculating. Also changed FAIL() to ADD_FAILURE() so the helper function can return a value on error paths.
Apply modern C++ style and small refactors across weapon proficiency, items, imbuements and player code: add [[nodiscard]] to many Item/Imbuement APIs, use using enum and unqualified enum values, replace .find(...)=end checks with contains, switch to std::ranges::find_if, and use (void) casts to silence unused-return warnings. Also adjust container operations (try_emplace/emplace_back), fix a serialization cast for perk.value, rename a loop variable for clarity, and simplify getSkillsEquipment signature and imbuement checks. These changes improve clarity, eliminate warnings, and adopt safer/cleaner idioms.
Declare a local SkillPercentage_t variable in WeaponProficiency::applySkillPercentageBonus to ensure the switch target is defined and avoid compilation issues. Remove the top-level const from Player::getSkillsEquipment() return type so the array is returned by value (allowing moves and consistent API semantics).
There was a problem hiding this comment.
Pull request overview
Adds protocol 15.11 support by introducing a new player Weapon Proficiency system (XP, perks, bonuses, serialization) and extending the imbuement flow to support imbuement scroll actions, alongside a broad set of content/protocol data updates needed by the 15.11 client.
Changes:
- Introduce
WeaponProficiencyplayer component (perk selection, XP progression, combat bonus application, KV persistence) and wire it into combat, equipment, login/save, and network protocol. - Rework imbuement networking/UI flows to support scroll actions (open/pick/scroll), add scroll IDs to imbuements, and add Lua APIs for scroll usage and clearing imbuements.
- Update protocol/data definitions and game content for 15.11 (client version bump, appearances proto flags/categories, new items/effects, achievements, loot table adjustments, outfits/mounts/NPC shop updates).
Reviewed changes
Copilot reviewed 87 out of 89 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vcproj/canary.vcxproj | Adds Weapon Proficiency sources to the VS project. |
| src/utils/utils_definitions.hpp | Adds Cipbia skill enum, new effect IDs, new item ID(s). |
| src/utils/tools.hpp | Adds new helpers + [[nodiscard]] annotations and new conversion helpers. |
| src/utils/tools.cpp | Implements undoShift + Cipbia conversion helpers. |
| src/utils/hash.hpp | Adds hash/equality for std::pair<uint16_t,uint8_t> map keys. |
| src/server/server_definitions.hpp | Adds INSPECT_PROFICIENCY. |
| src/server/network/protocol/protocolgame.hpp | Adds new parsers/senders and imbuement window API changes. |
| src/server/network/protocol/protocolgame.cpp | Implements weapon proficiency/imbuement protocol flows; augments cyclopedia stats payload. |
| src/protobuf/appearances.proto | Updates market categories + adds proficiency appearance flag. |
| src/pch.hpp | Tweaks STL include section and adds JSON include. |
| src/lua/functions/creatures/player/player_functions.hpp | Adds Lua methods for scrolls/imbuements/speed/proficiency XP. |
| src/lua/functions/creatures/player/player_functions.cpp | Implements new Lua methods for imbuement scrolls, clearing imbuements, speed, proficiency XP. |
| src/lua/functions/creatures/monster/loot_functions.cpp | Improves Loot constructor + refactors setters and error messages. |
| src/lua/creature/movement.cpp | Applies/clears weapon proficiency perks when equipping/unequipping. |
| src/items/weapons/weapons.hpp | Changes shootRange type to int32_t for proficiency range bonus. |
| src/items/weapons/weapons.cpp | Applies proficiency bonuses to range, attack damage, and hit chance. |
| src/items/items_definitions.hpp | Expands/renumbers augment enum to align with new system. |
| src/items/items.hpp | Adds [[nodiscard]] and introduces proficiencyId in ItemType. |
| src/items/items.cpp | Maps protobuf categories and loads proficiency IDs from appearances into items. |
| src/items/item.hpp | Adds Item::getFreeImbuementSlot(). |
| src/items/item.cpp | Implements free-slot search + strengthens imbuement validation/error messages. |
| src/items/functions/item/item_parse.cpp | Improves unknown imbuement type warning message. |
| src/io/iologindata.cpp | Saves weapon proficiency KV data for online players. |
| src/io/io_wheel.cpp | Fixes spelling field name (additionalTarget). |
| src/io/functions/iologindata_load_player.cpp | Loads weapon proficiency on login init. |
| src/game/scheduling/events_scheduler.cpp | Removes redundant JSON include (now in PCH). |
| src/game/game.hpp | Adjusts imbuement apply signature. |
| src/game/game.cpp | Updates imbuement apply flow + some combat/leech logic changes. |
| src/enums/weapon_proficiency.hpp | Adds weapon proficiency enums/structs. |
| src/enums/imbuement.hpp | Adds ImbuementAction enum. |
| src/creatures/players/player.hpp | Integrates weapon proficiency, base critical config, and augment aggregation. |
| src/creatures/players/player.cpp | Implements imbuement scroll creation/apply/clear + weapon proficiency accessors and skill breakdown. |
| src/creatures/players/imbuements/imbuements.hpp | Adds scroll mapping APIs and getImbuements options. |
| src/creatures/players/imbuements/imbuements.cpp | Loads scroll IDs from XML and builds scroll lookup map. |
| src/creatures/players/components/wheel/wheel_spells.hpp | Renames aditionalTarget → additionalTarget. |
| src/creatures/players/components/wheel/wheel_definitions.hpp | Adds wheel stat CRITICAL_CHANCE. |
| src/creatures/players/components/wheel/player_wheel.hpp | Adds wheel augment aggregation helpers. |
| src/creatures/players/components/wheel/player_wheel.cpp | Implements getActiveAugments() for wheel bonuses. |
| src/creatures/players/components/weapon_proficiency.hpp | Declares WeaponProficiency component API. |
| src/creatures/players/components/weapon_proficiency.cpp | Implements full weapon proficiency system: JSON load, KV load/save, perk application, combat bonuses. |
| src/creatures/monsters/monsters.hpp | Extends Loot with monster context name for better logging. |
| src/creatures/monsters/monster.cpp | Awards weapon proficiency on kill + applies on-kill life/mana gains. |
| src/creatures/creatures_definitions.hpp | Adds new combat origin and updates bestiary race enum ordering. |
| src/creatures/combat/spells.cpp | Updates augment types used for spell augment computation. |
| src/creatures/combat/combat.cpp | Integrates weapon proficiency augments/crit/bestiary/boss bonuses into combat. |
| src/creatures/CMakeLists.txt | Adds weapon proficiency component to build. |
| src/core.hpp | Bumps client version to 1511. |
| src/config/configmanager.cpp | Adds new config keys for base critical + weapon proficiency limits. |
| src/config/config_enums.hpp | Declares new config keys. |
| src/canary_server.hpp | Changes helper signature to std::string_view. |
| src/canary_server.cpp | Loads proficiencies.json at module load. |
| data/scripts/talkactions/god/weapon_proficiency.lua | Adds a god command to grant weapon proficiency XP. |
| data/scripts/lib/register_monster_type.lua | Passes monster name into Loot for better diagnostics. |
| data/scripts/lib/register_achievements.lua | Adds new achievements for 15.11 content. |
| data/scripts/lib/proficiency_helper.lua | Adds helper to create proficiency catalyst actions. |
| data/scripts/globalevents/save_interval.lua | Reworks save warning scheduling based on configured daily save time. |
| data/modules/scripts/gamestore/catalog/consumables_exercise_weapons.lua | Adds exercise wraps store offers. |
| data/XML/outfits.xml | Adds new outfits for 15.11. |
| data/XML/mounts.xml | Adds new mount (Gloom Maw). |
| data/XML/imbuements.xml | Adds scroll IDs for imbuements + tweaks some imbuement configs. |
| data-otservbr-global/scripts/actions/object/proficiency_catalyst.lua | Registers proficiency catalyst action. |
| data-otservbr-global/scripts/actions/object/imbuement_shrine.lua | Updates shrine to open imbuement UI without target param. |
| data-otservbr-global/scripts/actions/object/imbuement_scrolls.lua | Adds action script for applying imbuement scrolls. |
| data-otservbr-global/scripts/actions/object/greater_proficiency_catalyst.lua | Registers greater catalyst action. |
| data-otservbr-global/scripts/actions/object/etcher.lua | Adds etcher tool action to clear all imbuements. |
| data-otservbr-global/npc/blue_valley/enpa-deia_pema.lua | Adds NPC shop item. |
| data-otservbr-global/npc/albinius.lua | Adds shop entries for blank scroll + etcher. |
| data-otservbr-global/monster/vermins/emerald_damselfly.lua | Adds loot item drop(s). |
| data-otservbr-global/monster/quests/the_secret_library/bosses/ghulosh.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/the_order_of_lion/bosses/drume.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/the_dream_courts/bosses/the_nightmare_beast.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/the_dream_courts/bosses/maxxenius.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/soul_war/goshnars_malice.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/soul_war/goshnars_hatred.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/primal_ordeal_quest/magma_bubble.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/marapur/timira_the_many-headed.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/grave_danger/bosses/scarlett_etzel.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/grave_danger/bosses/earl_osam.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/grave_danger/bosses/count_vlarkorth.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/feaster_of_souls/the_pale_worm.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/quests/feaster_of_souls/the_fear_feaster.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/dawnport/dawnfly.lua | Adds loot item drop(s). |
| data-otservbr-global/monster/bosses/the_monster.lua | Standardizes transcendence potion drop by ID. |
| data-otservbr-global/monster/bosses/the_brainstealer.lua | Standardizes transcendence potion drop by ID. |
| data-canary/scripts/actions/objects/imbuement_shrine.lua | Updates shrine to open imbuement UI without target param. |
| config.lua.dist | Adds config knobs for base critical and proficiency limits. |
Comments suppressed due to low confidence (2)
src/creatures/players/components/weapon_proficiency.cpp:712
WeaponProficiency::getExperiencedefaultsweaponIdto 0 but doesn’t resolve it to the currently equipped weapon (unlikeisUpgradeAvailable/addExperience). CallinggetExperience()without arguments will always return 0 even when the player has proficiency data. Consider normalizingweaponIdwhen it is 0 (e.g., usem_player.getWeaponId(true)).
data/XML/outfits.xml:257- In the male outfit section, the new Illuminator entry uses
type="0"(female) even though it’s under the male list (where other entries usetype="1"). This likely prevents the male Illuminator outfit from being registered correctly. Change thetypeto match the male section.
<outfit type="1" looktype="1824" name="Monk" premium="no" unlocked="yes" enabled="yes" />
<outfit type="1" looktype="1837" name="Martial Artist" premium="no" unlocked="no" from="store" enabled="yes" />
<outfit type="0" looktype="1860" name="Illuminator" premium="yes" unlocked="no" from="quest" enabled="yes" />
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void WeaponProficiency::setSelectedPerk(uint8_t level, uint8_t perkIndex, uint16_t weaponId /* = 0 */) { | ||
| using enum WeaponProficiencyBonus_t; | ||
|
|
||
| if (weaponId == 0) { | ||
| weaponId = m_player.getWeaponId(true); | ||
| } | ||
|
|
||
| if (weaponId == 0) { | ||
| g_logger().error("{} - Invalid weapon ID: {}", __FUNCTION__, weaponId); | ||
| return; | ||
| } | ||
|
|
||
| if (!isValidWeaponId(weaponId)) { | ||
| g_logger().error("{} - Weapon ID out of range: {}", __FUNCTION__, weaponId); | ||
| return; | ||
| } | ||
|
|
||
| auto proficiencyId = Item::items[weaponId].proficiencyId; | ||
| if (!proficiencies.contains(proficiencyId)) { | ||
| g_logger().error("{} - Proficiency not found for weapon ID: {}", __FUNCTION__, weaponId); | ||
| return; | ||
| } | ||
|
|
||
| const auto &info = proficiencies.at(proficiencyId); | ||
| if (level >= info.level.size()) { | ||
| g_logger().error("{} - Proficiency level exceeds maximum size for weapon ID: {}", __FUNCTION__, weaponId); | ||
| return; | ||
| } | ||
| const auto &selectedLevel = info.level.at(level); | ||
|
|
||
| if (perkIndex >= selectedLevel.perks.size()) { | ||
| g_logger().error("{} - Proficiency level {} exceeds maximum perks size for weapon ID: {}", __FUNCTION__, level, weaponId); | ||
| return; | ||
| } | ||
| const auto &selectedPerk = selectedLevel.perks.at(perkIndex); | ||
|
|
||
| if (auto it = proficiency.find(weaponId); it != proficiency.end()) { | ||
| it->second.perks.push_back(selectedPerk); | ||
| } | ||
| } |
There was a problem hiding this comment.
WeaponProficiency::setSelectedPerk only appends the selected perk if proficiency.find(weaponId) already exists. However, parseWeaponProficiency clears selections via clearSelectedPerks before re-sending selected perks, which removes the entry—so perk selection updates can be silently dropped. Ensure an entry exists (create one preserving experience) before pushing perks, or change clearSelectedPerks to not erase the entry.
| @@ -12217,6 +12417,11 @@ bool Player::isFirstOnStack() const { | |||
| return false; | |||
| } | |||
|
|
|||
| if (hasCondition(CONDITION_SPELLCOOLDOWN)) { | |||
| g_logger().warn("[isFirstOnStack] cooldown error for player: {}", getName()); | |||
| return false; | |||
| } | |||
There was a problem hiding this comment.
Player::isFirstOnStack now returns true when getTile() is null. That changes the meaning of the check and can incorrectly treat players as first-on-stack in edge cases (e.g., during logout/teleport transitions), potentially bypassing logic that depends on stack ordering. It also logs a warning every call when CONDITION_SPELLCOOLDOWN is present, which can spam logs. Consider restoring the previous false behavior for null tiles and avoid per-call warn logging here (or gate it behind a debug/trace level).
| uint8_t undoShift(uint64_t value) { | ||
| if (value == 0) { | ||
| return 0; | ||
| } | ||
|
|
||
| auto trailingZeros = std::countr_zero(value); | ||
| if (trailingZeros < 2) { | ||
| return 0; | ||
| } | ||
| return trailingZeros - 2; | ||
| } |
There was a problem hiding this comment.
undoShift uses std::countr_zero, which requires including the <bit> header. Currently this translation unit doesn’t include <bit> (and it’s not guaranteed to be provided transitively), so this can fail to compile on some toolchains. Add the appropriate include in this file (or the PCH) to ensure portability.
| uint32_t maxExperience = getMaxExperience(weaponId); | ||
|
|
||
| if (!proficiency.contains(weaponId)) { | ||
| (void)proficiency.try_emplace(weaponId, std::min(experience, maxExperience)); | ||
| m_player.sendWeaponProficiency(weaponId); | ||
|
|
||
| return; |
There was a problem hiding this comment.
WeaponProficiency::addExperience uses proficiency.try_emplace(weaponId, std::min(experience, maxExperience)), but WeaponProficiencyData has no constructor that takes a uint32_t, so this won’t compile. Initialize a WeaponProficiencyData value (e.g., default-construct then assign .experience) before inserting, or use an aggregate/value-initialization that sets the experience field explicitly.
| void WeaponProficiency::clearSelectedPerks(uint16_t weaponId) { | ||
| [[maybe_unused]] const auto &unusedProficiency = proficiency.erase(weaponId); | ||
| m_player.sendWeaponProficiency(weaponId); | ||
| } |
There was a problem hiding this comment.
WeaponProficiency::clearSelectedPerks currently erases the entire proficiency[weaponId] entry. That drops not only selected perks but also the stored experience/mastered state for the weapon, which seems unintended when the client is only clearing perk selections. Consider clearing only the perks vector (and keeping experience/mastered) instead of erasing the whole entry.
Delete the random failure branch in Player::onApplyImbuement that used uniform_random to abort the imbuement, send a failure message and reopen the imbuement window. With this change, once prerequisites are met the code proceeds to apply imbuement stats instead of potentially failing at this random check.
|
Thanks and credits for: @phacUFPE and @FelipePaluco for developing.
Client: dudantas/tibia-client#17
This pull request introduces a new
WeaponProficiencycomponent for players, which provides a comprehensive system for managing weapon proficiency, perks, experience, and related combat bonuses. The implementation includes serialization/deserialization methods, experience and perk management, stat tracking, and various combat-related bonus applications.Key additions in the new
weapon_proficiency.hpp:Weapon Proficiency System:
WeaponProficiencyclass, encapsulating all logic related to weapon proficiency, including experience tracking, perk management, stat bonuses, and combat effect applications.Combat and Stat Enhancements:
Experience and Perk Management:
Summary by CodeRabbit
New Features
Items & Actions
Content
Quality of Life
Loot