proposal: extra attack ability#3983
Conversation
Feature created for https://rookgaardtales.com Useful if you plan to run baiak projects in canary or custom content related. This new function allows you to enable rows of auto attacks in your weapons via xml without need of coding extra scripts. It handles animations and range for each try and it also ammo consumption for paladins. I did made a few test and anti crash methods to have this feature available on crystal server. This is only a proposal of a feature I wanted to share with the community, it's up to canary team to merge it or not. Inspired by x4Cut ability from Final Fantasy 7 Everything has been debuged and tested at https://tibiatales.com using canary/tfs crystal server distribution. I hope you like it! - Con cariño para la comunidad.
📝 WalkthroughWalkthroughThis PR implements weapon extra attacks: weapons can now trigger follow-up hits based on configurable chance, count, and delay parameters. The implementation adds data fields to ItemType and CombatDamage, parses XML attributes, provides Item accessor methods, and schedules delayed follow-up combat hits with runtime validation for range, sight, and z-level, including optional ammo consumption. ChangesWeapon Extra Attacks Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for weapon extra attacks, including parsing of extra attack attributes (count, delay, and chance) and scheduling these attacks during combat. However, several critical issues were identified in the implementation: the implementation for ItemParse::parseExtraAttack is completely missing from item_parse.cpp which will cause compilation failures, reassigning the weapon tool to the bow instead of keeping it as ammo breaks damage calculations, and the scheduled event lambda lacks necessary checks to ensure the caster is still alive and has not been removed from the game.
| ItemParse::parseDefense(stringValue, valueAttribute, itemType); | ||
| ItemParse::parseExtraDefense(stringValue, valueAttribute, itemType); | ||
| ItemParse::parseAttack(stringValue, valueAttribute, itemType); | ||
| ItemParse::parseExtraAttack(stringValue, valueAttribute, itemType); |
There was a problem hiding this comment.
The function ItemParse::parseExtraAttack is declared in item_parse.hpp and called here, but its implementation is completely missing from item_parse.cpp. This will result in a compilation error (undefined reference).
Please add the implementation of parseExtraAttack to item_parse.cpp:
void ItemParse::parseExtraAttack(const std::string &stringValue, pugi::xml_attribute valueAttribute, ItemType &itemType) {
if (stringValue == "extraattack") {
itemType.extraAttack = pugi::cast<int32_t>(valueAttribute.value());
} else if (stringValue == "extraattackdelay") {
itemType.extraAttackDelay = pugi::cast<int32_t>(valueAttribute.value());
} else if (stringValue == "extraattackchance") {
itemType.extraAttackChance = pugi::cast<int32_t>(valueAttribute.value());
}
}Additionally, make sure to map these attributes in ItemParseAttributesMap inside item_parse.hpp to keep the attribute map synchronized.
| if (tool->getWeaponType() == WEAPON_AMMO) { | ||
| currentAmmo = tool; | ||
| if (currentAmmo->getItemCount() < 1) { | ||
| return; | ||
| } | ||
|
|
||
| const auto &bow = player->getWeapon(true); | ||
| if (bow) { | ||
| const auto &bowWeapon = g_weapons().getWeapon(bow); | ||
| if (bowWeapon) { | ||
| tool = bow; | ||
| weaponObj = bowWeapon; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Reassigning tool and weaponObj to the bow inside the lambda breaks the damage calculation for distance weapons.
In Canary, getWeaponDamage and getElementDamage for distance weapons expect the item parameter to be the ammo (which is the arrow/bolt), not the bow itself. By reassigning tool to the bow, the combat system will treat the bow as the ammo, leading to incorrect damage calculations (or failing to calculate damage entirely if the bow is not registered as a weapon in g_weapons()).
Since the range and other bow-specific properties are already resolved outside the lambda, you should keep tool as the ammo and weaponObj as the ammo's weapon object.
if (tool->getWeaponType() == WEAPON_AMMO) {
currentAmmo = tool;
if (currentAmmo->getItemCount() < 1) {
return;
}
}| if (!caster || !target || !target->isAlive()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The scheduled event lambda only checks if the target is alive, but does not check if the caster is still alive or has been removed from the game.
If the caster dies or logs out during the extraDelay period, the scheduled extra attacks will still fire from their corpse or a null position, which is a logical bug and can potentially lead to crashes.
Add checks to ensure the caster is still alive and has not been removed.
if (!caster || caster->isRemoved() || !caster->isAlive() || !target || target->isRemoved() || !target->isAlive()) {
return;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/items/functions/item/item_parse.hpp (1)
19-165:⚠️ Potential issue | 🟠 MajorAdd
extraattack*keys toItemParseAttributesMapso item XML parsing doesn’t skipparseExtraAttack.
Items::parseItemNodelowercases the XML attribute name and only callsItemParse::initParse(...)when the key exists inItemParseAttributesMap; unknown keys are logged and ignored.ItemParseAttributesMapcurrently contains no"extraattack","extraattackdelay", or"extraattackchance"entries, soparseExtraAttack(...)will never run for those attributes even though it exists inItemParse::initParse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/items/functions/item/item_parse.hpp` around lines 19 - 165, The ItemParseAttributesMap is missing entries for extraattack-related XML attributes so Items::parseItemNode never invokes ItemParse::initParse to call parseExtraAttack; add mapping entries for "extraattack", "extraattackdelay", and "extraattackchance" to ItemParseAttributesMap pointing to the appropriate ItemParseAttributes_t value used by parseExtraAttack (use the same enum/identifier that other attack attributes use) so ItemParse::initParse will route those attributes into parseExtraAttack.
🧹 Nitpick comments (1)
src/creatures/combat/combat.cpp (1)
1578-1716: ⚡ Quick winPlease add a focused regression test for extra-attack scheduling.
Cover at least: recursion guard (
isExtraAttack), range/sight revalidation at execution time, and ammo consumption behavior.As per coding guidelines, "**/*.{cpp,hpp,h,cc,cxx}: For C++ changes, prefer focused tests or the smallest practical build/check that validates the touched code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/creatures/combat/combat.cpp` around lines 1578 - 1716, Add a focused regression test that exercises the extra-attack scheduling path triggered in the block using g_dispatcher().scheduleEvent (the extra-attack loop that constructs extraDamage and calls Combat::doCombatHealth); the test should (1) verify the recursion/guard by ensuring an extra attack with isExtraAttack=true does not itself schedule further extra attacks, (2) simulate movement or line-of-sight changes so the scheduled job revalidates range/sight at execution time and does not apply damage when out of range or not visible, and (3) validate ammo consumption behavior when REMOVE_WEAPON_AMMO is enabled by asserting the ammo item count is decremented/removed and player->updateSupplyTracker is called; create the test as a small, focused C++ unit/integration test that sets up a caster/player/weapon/ammo and advances the dispatcher to execute scheduled events, then assert no recursive scheduling, correct damage application only when conditions hold, and correct ammo bookkeeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@data/items/items.xml`:
- Line 53164: Update the inline comment for the attribute key
"extraattackchance" to state the encoding basis: explain that the value is in
hundredths of a percent (basis points) out of 10,000 (e.g., 500 = 500/10000 =
5%), so future editors understand how to compute percentages; modify the comment
on the line with <attribute key="extraattackchance" ... /> to reflect this
clarification.
- Line 53163: Update the inline comment next to the attribute with
key="extraattackdelay" value="500" to correctly state "500ms of delay between
attacks" (instead of "5ms"); locate the attribute element for extraattackdelay
and edit only the comment text to reflect the correct millisecond value.
In `@src/creatures/combat/combat.cpp`:
- Around line 1591-1611: Validate and clamp weapon-derived values before
scheduling: read extraAttacks via weapon->getExtraAttack(), extraDelay via
weapon->getExtraAttackDelay(), and extraAttackChance via
weapon->getExtraAttackChance() and enforce sane bounds (e.g., clamp
extraAttackChance to [0,10000], clamp extraAttacks to a small constant like
MAX_EXTRA_ATTACKS, and clamp extraDelay to [0,MAX_EXTRA_DELAY]). If any value is
out of range or after clamping extraAttacks == 0 or extraAttackChance == 0, skip
scheduling; also guard against overflow when computing i * extraDelay (use
64-bit or clamp the product to MAX_SCHEDULE_DELAY) before passing to
g_dispatcher().scheduleEvent; apply these checks where the current code calls
g_dispatcher().scheduleEvent inside the loop and use named constants
(MAX_EXTRA_ATTACKS, MAX_EXTRA_DELAY, MAX_EXTRA_ATTACK_CHANCE) so behavior is
explicit.
In `@src/items/functions/item/item_parse.cpp`:
- Line 32: ItemParse::initParse calls ItemParse::parseExtraAttack(stringValue,
valueAttribute, itemType) but there is no implementation for
ItemParse::parseExtraAttack in this translation unit, causing a linker error;
add a matching definition for ItemParse::parseExtraAttack with the same
signature (parameters and namespace/class scope) or remove the call if unused.
Locate the declaration (likely in item_parse.h) and implement the function in
item_parse.cpp (or the appropriate .cpp) with the expected behavior, ensuring
the function signature exactly matches the declaration (including const/ref
qualifiers) so the linker can resolve it; if the logic belongs elsewhere,
forward the call to the correct function or update initParse to use the existing
implementation.
---
Outside diff comments:
In `@src/items/functions/item/item_parse.hpp`:
- Around line 19-165: The ItemParseAttributesMap is missing entries for
extraattack-related XML attributes so Items::parseItemNode never invokes
ItemParse::initParse to call parseExtraAttack; add mapping entries for
"extraattack", "extraattackdelay", and "extraattackchance" to
ItemParseAttributesMap pointing to the appropriate ItemParseAttributes_t value
used by parseExtraAttack (use the same enum/identifier that other attack
attributes use) so ItemParse::initParse will route those attributes into
parseExtraAttack.
---
Nitpick comments:
In `@src/creatures/combat/combat.cpp`:
- Around line 1578-1716: Add a focused regression test that exercises the
extra-attack scheduling path triggered in the block using
g_dispatcher().scheduleEvent (the extra-attack loop that constructs extraDamage
and calls Combat::doCombatHealth); the test should (1) verify the
recursion/guard by ensuring an extra attack with isExtraAttack=true does not
itself schedule further extra attacks, (2) simulate movement or line-of-sight
changes so the scheduled job revalidates range/sight at execution time and does
not apply damage when out of range or not visible, and (3) validate ammo
consumption behavior when REMOVE_WEAPON_AMMO is enabled by asserting the ammo
item count is decremented/removed and player->updateSupplyTracker is called;
create the test as a small, focused C++ unit/integration test that sets up a
caster/player/weapon/ammo and advances the dispatcher to execute scheduled
events, then assert no recursive scheduling, correct damage application only
when conditions hold, and correct ammo bookkeeping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd7f00d9-1cdc-43d5-b987-9b58f3080b70
📒 Files selected for processing (8)
data/items/items.xmlsrc/creatures/combat/combat.cppsrc/creatures/creatures_definitions.hppsrc/items/functions/item/item_parse.cppsrc/items/functions/item/item_parse.hppsrc/items/item.hppsrc/items/items.hppsrc/items/items_definitions.hpp
| <attribute key="attack" value="6" /> | ||
| <attribute key="weight" value="4000" /> | ||
| <attribute key="extraattack" value="3" /> <!-- x3 rows as extra attacks --> | ||
| <attribute key="extraattackdelay" value="500" /> <!-- 5ms of delay between attacks --> |
There was a problem hiding this comment.
Fix the comment typo for delay timing.
The comment states "5ms of delay" but the value is 500. In OT servers, timing values are typically in milliseconds, so this should be "500ms of delay between attacks" not "5ms".
📝 Proposed fix
- <attribute key="extraattackdelay" value="500" /> <!-- 5ms of delay between attacks -->
+ <attribute key="extraattackdelay" value="500" /> <!-- 500ms of delay between attacks -->📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <attribute key="extraattackdelay" value="500" /> <!-- 5ms of delay between attacks --> | |
| <attribute key="extraattackdelay" value="500" /> <!-- 500ms of delay between attacks --> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data/items/items.xml` at line 53163, Update the inline comment next to the
attribute with key="extraattackdelay" value="500" to correctly state "500ms of
delay between attacks" (instead of "5ms"); locate the attribute element for
extraattackdelay and edit only the comment text to reflect the correct
millisecond value.
| <attribute key="weight" value="4000" /> | ||
| <attribute key="extraattack" value="3" /> <!-- x3 rows as extra attacks --> | ||
| <attribute key="extraattackdelay" value="500" /> <!-- 5ms of delay between attacks --> | ||
| <attribute key="extraattackchance" value="500" /> <!-- 5% chance to trigger auto attacks --> |
There was a problem hiding this comment.
Clarify the percentage calculation basis in the comment.
The comment states "5% chance" for a value of 500, which suggests the system uses a base of 10,000 for percentage calculations (500/10000 = 5%). While this is a common pattern in OT servers, the comment doesn't clarify this encoding, which could confuse developers trying to configure different chance values.
📝 Proposed fix
- <attribute key="extraattackchance" value="500" /> <!-- 5% chance to trigger auto attacks -->
+ <attribute key="extraattackchance" value="500" /> <!-- 5% chance to trigger auto attacks (500/10000) -->📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <attribute key="extraattackchance" value="500" /> <!-- 5% chance to trigger auto attacks --> | |
| <attribute key="extraattackchance" value="500" /> <!-- 5% chance to trigger auto attacks (500/10000) --> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data/items/items.xml` at line 53164, Update the inline comment for the
attribute key "extraattackchance" to state the encoding basis: explain that the
value is in hundredths of a percent (basis points) out of 10,000 (e.g., 500 =
500/10000 = 5%), so future editors understand how to compute percentages; modify
the comment on the line with <attribute key="extraattackchance" ... /> to
reflect this clarification.
| const int32_t extraAttacks = weapon->getExtraAttack(); | ||
| const int32_t extraDelay = weapon->getExtraAttackDelay(); | ||
| const int32_t extraAttackChance = weapon->getExtraAttackChance(); | ||
|
|
||
| if (extraAttacks > 0 && extraAttackChance > 0 | ||
| && uniform_random(1, 10000) <= extraAttackChance) { | ||
|
|
||
| const CombatType_t combatType = params.combatType; | ||
| const CombatOrigin combatOrigin = params.origin; | ||
| const uint16_t distEffect = params.distanceEffect != CONST_ANI_NONE | ||
| ? params.distanceEffect | ||
| : Item::items[weapon->getID()].shootType; | ||
|
|
||
| const uint8_t weaponRange = weapon->getShootRange() > 0 | ||
| ? weapon->getShootRange() | ||
| : Item::items[weapon->getID()].shootRange; | ||
|
|
||
| for (int32_t i = 1; i <= extraAttacks; ++i) { | ||
| g_dispatcher().scheduleEvent( | ||
| i * extraDelay, | ||
| [caster, target, combatType, combatOrigin, distEffect, weaponRange]() { |
There was a problem hiding this comment.
Add hard bounds before scheduling extra attacks.
extraAttacks, extraDelay, and extraAttackChance are used unchecked; malformed item data can queue excessive events or invalid timing/probability behavior.
💡 Proposed guardrails
- const int32_t extraAttacks = weapon->getExtraAttack();
- const int32_t extraDelay = weapon->getExtraAttackDelay();
- const int32_t extraAttackChance = weapon->getExtraAttackChance();
+ const int32_t extraAttacks = std::clamp<int32_t>(weapon->getExtraAttack(), 0, 10);
+ const int32_t extraDelay = std::max<int32_t>(0, weapon->getExtraAttackDelay());
+ const int32_t extraAttackChance = std::clamp<int32_t>(weapon->getExtraAttackChance(), 0, 10000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const int32_t extraAttacks = weapon->getExtraAttack(); | |
| const int32_t extraDelay = weapon->getExtraAttackDelay(); | |
| const int32_t extraAttackChance = weapon->getExtraAttackChance(); | |
| if (extraAttacks > 0 && extraAttackChance > 0 | |
| && uniform_random(1, 10000) <= extraAttackChance) { | |
| const CombatType_t combatType = params.combatType; | |
| const CombatOrigin combatOrigin = params.origin; | |
| const uint16_t distEffect = params.distanceEffect != CONST_ANI_NONE | |
| ? params.distanceEffect | |
| : Item::items[weapon->getID()].shootType; | |
| const uint8_t weaponRange = weapon->getShootRange() > 0 | |
| ? weapon->getShootRange() | |
| : Item::items[weapon->getID()].shootRange; | |
| for (int32_t i = 1; i <= extraAttacks; ++i) { | |
| g_dispatcher().scheduleEvent( | |
| i * extraDelay, | |
| [caster, target, combatType, combatOrigin, distEffect, weaponRange]() { | |
| const int32_t extraAttacks = std::clamp<int32_t>(weapon->getExtraAttack(), 0, 10); | |
| const int32_t extraDelay = std::max<int32_t>(0, weapon->getExtraAttackDelay()); | |
| const int32_t extraAttackChance = std::clamp<int32_t>(weapon->getExtraAttackChance(), 0, 10000); | |
| if (extraAttacks > 0 && extraAttackChance > 0 | |
| && uniform_random(1, 10000) <= extraAttackChance) { | |
| const CombatType_t combatType = params.combatType; | |
| const CombatOrigin combatOrigin = params.origin; | |
| const uint16_t distEffect = params.distanceEffect != CONST_ANI_NONE | |
| ? params.distanceEffect | |
| : Item::items[weapon->getID()].shootType; | |
| const uint8_t weaponRange = weapon->getShootRange() > 0 | |
| ? weapon->getShootRange() | |
| : Item::items[weapon->getID()].shootRange; | |
| for (int32_t i = 1; i <= extraAttacks; ++i) { | |
| g_dispatcher().scheduleEvent( | |
| i * extraDelay, | |
| [caster, target, combatType, combatOrigin, distEffect, weaponRange]() { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/creatures/combat/combat.cpp` around lines 1591 - 1611, Validate and clamp
weapon-derived values before scheduling: read extraAttacks via
weapon->getExtraAttack(), extraDelay via weapon->getExtraAttackDelay(), and
extraAttackChance via weapon->getExtraAttackChance() and enforce sane bounds
(e.g., clamp extraAttackChance to [0,10000], clamp extraAttacks to a small
constant like MAX_EXTRA_ATTACKS, and clamp extraDelay to [0,MAX_EXTRA_DELAY]).
If any value is out of range or after clamping extraAttacks == 0 or
extraAttackChance == 0, skip scheduling; also guard against overflow when
computing i * extraDelay (use 64-bit or clamp the product to MAX_SCHEDULE_DELAY)
before passing to g_dispatcher().scheduleEvent; apply these checks where the
current code calls g_dispatcher().scheduleEvent inside the loop and use named
constants (MAX_EXTRA_ATTACKS, MAX_EXTRA_DELAY, MAX_EXTRA_ATTACK_CHANCE) so
behavior is explicit.
| ItemParse::parseDefense(stringValue, valueAttribute, itemType); | ||
| ItemParse::parseExtraDefense(stringValue, valueAttribute, itemType); | ||
| ItemParse::parseAttack(stringValue, valueAttribute, itemType); | ||
| ItemParse::parseExtraAttack(stringValue, valueAttribute, itemType); |
There was a problem hiding this comment.
parseExtraAttack is called but not defined (linker break).
ItemParse::initParse invokes parseExtraAttack(...) on Line 32, but this translation unit has no implementation for it.
💡 Proposed fix
+void ItemParse::parseExtraAttack(const std::string &stringValue, pugi::xml_attribute valueAttribute, ItemType &itemType) {
+ if (stringValue == "extraattack") {
+ itemType.extraAttack = std::max<int32_t>(0, pugi::cast<int32_t>(valueAttribute.value()));
+ } else if (stringValue == "extraattackdelay") {
+ itemType.extraAttackDelay = std::max<int32_t>(0, pugi::cast<int32_t>(valueAttribute.value()));
+ } else if (stringValue == "extraattackchance") {
+ itemType.extraAttackChance = std::clamp<int32_t>(pugi::cast<int32_t>(valueAttribute.value()), 0, 10000);
+ }
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/items/functions/item/item_parse.cpp` at line 32, ItemParse::initParse
calls ItemParse::parseExtraAttack(stringValue, valueAttribute, itemType) but
there is no implementation for ItemParse::parseExtraAttack in this translation
unit, causing a linker error; add a matching definition for
ItemParse::parseExtraAttack with the same signature (parameters and
namespace/class scope) or remove the call if unused. Locate the declaration
(likely in item_parse.h) and implement the function in item_parse.cpp (or the
appropriate .cpp) with the expected behavior, ensuring the function signature
exactly matches the declaration (including const/ref qualifiers) so the linker
can resolve it; if the logic belongs elsewhere, forward the call to the correct
function or update initParse to use the existing implementation.
Re-adjusted for canary/crystal + safety checks.
|
❌ The last analysis has failed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/creatures/combat/combat.cpp (1)
1714-1718: ⚡ Quick winIgnoring return values from item operations.
The return values from
transformItemandinternalRemoveItemare explicitly cast to void. These operations can fail (e.g., if another event already modified the item), and silently ignoring failures could lead to inventory desync or duplicate ammo consumption tracking.♻️ Proposed improvement: Handle potential failures
if (ammoCount > 1) { - auto transformed = g_game().transformItem(ammoToRemove, ammoToRemove->getID(), ammoCount - 1); - (void)transformed; + if (!g_game().transformItem(ammoToRemove, ammoToRemove->getID(), ammoCount - 1)) { + g_logger().warn("[Combat::extraAttack] Failed to transform ammo for player {}", player->getName()); + } } else if (ammoCount == 1) { - auto removed = g_game().internalRemoveItem(ammoToRemove); - (void)removed; + if (g_game().internalRemoveItem(ammoToRemove) != RETURNVALUE_NOERROR) { + g_logger().warn("[Combat::extraAttack] Failed to remove ammo for player {}", player->getName()); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/creatures/combat/combat.cpp` around lines 1714 - 1718, The code currently ignores the return values of g_game().transformItem(...) and g_game().internalRemoveItem(...), which can fail; update the handlers in the branch that deals with ammoToRemove/ammoCount to check those return values (e.g., check the transformed pointer/result and the removed success flag), and on failure take a deterministic action such as logging an error with context (ammoToRemove->getID(), ammoCount, attacker/creature id), aborting or rolling back the attack sequence (returning false or stopping further ammo consumption), and/or re-fetching/validating the item before proceeding to avoid inventory desync; specifically modify the blocks that call transformItem and internalRemoveItem to perform explicit success checks and handle failure paths instead of casting results to void.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/creatures/combat/combat.cpp`:
- Around line 1709-1722: The call to player->updateSupplyTracker(ammoToRemove)
happens after g_game().transformItem or g_game().internalRemoveItem may
modify/remove the item, risking sending invalid item data; fix by recording the
needed supply info (e.g., ammoToRemove->getID() and ammoToRemove->getItemCount()
or any supply-identifier) or calling player->updateSupplyTracker with that
captured info before performing g_game().transformItem/ internalRemoveItem;
update the logic around ammoToRemove, g_game().transformItem,
g_game().internalRemoveItem and player->updateSupplyTracker so the tracker
receives valid pre-modification data.
- Around line 1692-1697: The catch-all around
weaponToUse->getWeaponDamage(player, target, damageTool, true) silently returns
and hides errors; replace the empty catch with logging the exception and context
before returning (capture std::exception_ptr, rethrow to extract
std::exception::what() if possible, or log that an unknown exception occurred)
and include identifiers like weaponToUse, player, target, damageTool and
variable weaponDamage in the log so you can trace failures to getWeaponDamage.
- Around line 1578-1581: Add the missing setup block before the extra-attack
loop: first check and return if damage.isExtraAttack to guard against recursion,
then obtain the player and weapon from caster (use the same accessor functions
used elsewhere in combat.cpp), compute extraAttacks and extraDelay by extracting
and clamping their values to allowed ranges, determine combatType, combatOrigin,
distEffect and weaponRange from the weapon/player state, and perform the
extra-attack chance roll (bail out if the roll fails) before entering the loop
that calls g_dispatcher().scheduleEvent; ensure the scheduled lambda still
captures the computed local variables (extraAttacks, extraDelay, combatType,
combatOrigin, distEffect, weaponRange) so they are in scope for the loop.
---
Nitpick comments:
In `@src/creatures/combat/combat.cpp`:
- Around line 1714-1718: The code currently ignores the return values of
g_game().transformItem(...) and g_game().internalRemoveItem(...), which can
fail; update the handlers in the branch that deals with ammoToRemove/ammoCount
to check those return values (e.g., check the transformed pointer/result and the
removed success flag), and on failure take a deterministic action such as
logging an error with context (ammoToRemove->getID(), ammoCount,
attacker/creature id), aborting or rolling back the attack sequence (returning
false or stopping further ammo consumption), and/or re-fetching/validating the
item before proceeding to avoid inventory desync; specifically modify the blocks
that call transformItem and internalRemoveItem to perform explicit success
checks and handle failure paths instead of casting results to void.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04383b17-5e5c-440c-80d7-5583a1f45d28
📒 Files selected for processing (1)
src/creatures/combat/combat.cpp
| for (int32_t i = 1; i <= extraAttacks; ++i) { | ||
| auto eventId = g_dispatcher().scheduleEvent( | ||
| i * extraDelay, | ||
| [caster, target, combatType, combatOrigin, distEffect, weaponRange]() { |
There was a problem hiding this comment.
Build failure: Missing variable declarations and setup block.
The code uses extraAttacks, extraDelay, combatType, combatOrigin, distEffect, and weaponRange without declaring them. The entire setup block before the extra attack loop is missing.
Based on the feature requirements and past review suggestions, this block should:
- Guard against recursive extra attacks using
damage.isExtraAttack - Retrieve the player and weapon from the caster
- Extract and clamp extra attack parameters
- Roll for extra attack chance before scheduling
🔧 Proposed fix: Add missing setup block before the loop
+ // Extra attack scheduling - skip if this is already an extra attack to prevent recursion
+ if (damage.isExtraAttack) {
+ return;
+ }
+
+ const auto &player = caster ? caster->getPlayer() : nullptr;
+ if (!player) {
+ return;
+ }
+
+ const auto &weapon = player->getWeapon(true);
+ if (!weapon || weapon->getExtraAttack() <= 0) {
+ return;
+ }
+
+ const int32_t extraAttacks = std::clamp<int32_t>(weapon->getExtraAttack(), 0, 10);
+ const int32_t extraDelay = std::max<int32_t>(0, weapon->getExtraAttackDelay());
+ const int32_t extraAttackChance = std::clamp<int32_t>(weapon->getExtraAttackChance(), 0, 10000);
+
+ if (extraAttackChance > 0 && uniform_random(1, 10000) > extraAttackChance) {
+ return;
+ }
+
+ const CombatType_t combatType = params.combatType;
+ const CombatOrigin combatOrigin = params.origin;
+ const uint16_t distEffect = params.distanceEffect;
+ const uint8_t weaponRange = weapon->getShootRange() > 0 ? weapon->getShootRange() : Item::items[weapon->getID()].shootRange;
+
for (int32_t i = 1; i <= extraAttacks; ++i) {
auto eventId = g_dispatcher().scheduleEvent(
i * extraDelay,
- [caster, target, combatType, combatOrigin, distEffect, weaponRange]() {
+ [caster, target, combatType, combatOrigin, distEffect, weaponRange]() {🧰 Tools
🪛 GitHub Check: Build - Linux / Compile (linux-debug)
[failure] 1581-1581:
‘weaponRange’ was not declared in this scope
[failure] 1581-1581:
‘distEffect’ was not declared in this scope; did you mean ‘Effect’?
[failure] 1581-1581:
‘combatOrigin’ was not declared in this scope; did you mean ‘CombatOrigin’?
[failure] 1581-1581:
‘combatType’ was not declared in this scope; did you mean ‘CombatType_t’?
[failure] 1580-1580:
‘extraDelay’ was not declared in this scope
[failure] 1578-1578:
‘extraAttacks’ was not declared in this scope
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/creatures/combat/combat.cpp` around lines 1578 - 1581, Add the missing
setup block before the extra-attack loop: first check and return if
damage.isExtraAttack to guard against recursion, then obtain the player and
weapon from caster (use the same accessor functions used elsewhere in
combat.cpp), compute extraAttacks and extraDelay by extracting and clamping
their values to allowed ranges, determine combatType, combatOrigin, distEffect
and weaponRange from the weapon/player state, and perform the extra-attack
chance roll (bail out if the roll fails) before entering the loop that calls
g_dispatcher().scheduleEvent; ensure the scheduled lambda still captures the
computed local variables (extraAttacks, extraDelay, combatType, combatOrigin,
distEffect, weaponRange) so they are in scope for the loop.
| try { | ||
| weaponDamage = std::abs(weaponToUse->getWeaponDamage(player, target, damageTool, true)); | ||
| } catch (...) { | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Silent exception swallowing hides potential bugs.
The catch block silently returns without logging. If getWeaponDamage throws, you'll have no indication of what went wrong during debugging or in production logs.
🛠️ Proposed fix: Add error logging
try {
weaponDamage = std::abs(weaponToUse->getWeaponDamage(player, target, damageTool, true));
} catch (...) {
-
+ g_logger().error("[Combat::extraAttack] Exception in getWeaponDamage for player {}", player->getName());
return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/creatures/combat/combat.cpp` around lines 1692 - 1697, The catch-all
around weaponToUse->getWeaponDamage(player, target, damageTool, true) silently
returns and hides errors; replace the empty catch with logging the exception and
context before returning (capture std::exception_ptr, rethrow to extract
std::exception::what() if possible, or log that an unknown exception occurred)
and include identifiers like weaponToUse, player, target, damageTool and
variable weaponDamage in the log so you can trace failures to getWeaponDamage.
| if (ammoToRemove && g_configManager().getBoolean(REMOVE_WEAPON_AMMO)) { | ||
|
|
||
| if (ammoToRemove->getID() != 0) { | ||
| const uint16_t ammoCount = ammoToRemove->getItemCount(); | ||
| if (ammoCount > 1) { | ||
| auto transformed = g_game().transformItem(ammoToRemove, ammoToRemove->getID(), ammoCount - 1); | ||
| (void)transformed; | ||
| } else if (ammoCount == 1) { | ||
| auto removed = g_game().internalRemoveItem(ammoToRemove); | ||
| (void)removed; | ||
| } | ||
| player->updateSupplyTracker(ammoToRemove); | ||
| } | ||
| } |
There was a problem hiding this comment.
Supply tracker called after item may be invalid.
updateSupplyTracker is called after the item has been transformed or removed. If internalRemoveItem succeeds, the ammoToRemove shared_ptr may reference an item no longer in the game world. While the shared_ptr keeps the object alive, updateSupplyTracker sends data to the client and party which expects a valid in-game item.
Consider calling the tracker before modification, or capturing the item ID/count beforehand.
🔧 Proposed fix: Track supply before modifying ammo
if (ammoToRemove && g_configManager().getBoolean(REMOVE_WEAPON_AMMO)) {
-
if (ammoToRemove->getID() != 0) {
+ // Track supply before modifying the item
+ player->updateSupplyTracker(ammoToRemove);
+
const uint16_t ammoCount = ammoToRemove->getItemCount();
if (ammoCount > 1) {
auto transformed = g_game().transformItem(ammoToRemove, ammoToRemove->getID(), ammoCount - 1);
(void)transformed;
} else if (ammoCount == 1) {
auto removed = g_game().internalRemoveItem(ammoToRemove);
(void)removed;
}
- player->updateSupplyTracker(ammoToRemove);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ammoToRemove && g_configManager().getBoolean(REMOVE_WEAPON_AMMO)) { | |
| if (ammoToRemove->getID() != 0) { | |
| const uint16_t ammoCount = ammoToRemove->getItemCount(); | |
| if (ammoCount > 1) { | |
| auto transformed = g_game().transformItem(ammoToRemove, ammoToRemove->getID(), ammoCount - 1); | |
| (void)transformed; | |
| } else if (ammoCount == 1) { | |
| auto removed = g_game().internalRemoveItem(ammoToRemove); | |
| (void)removed; | |
| } | |
| player->updateSupplyTracker(ammoToRemove); | |
| } | |
| } | |
| if (ammoToRemove && g_configManager().getBoolean(REMOVE_WEAPON_AMMO)) { | |
| if (ammoToRemove->getID() != 0) { | |
| // Track supply before modifying the item | |
| player->updateSupplyTracker(ammoToRemove); | |
| const uint16_t ammoCount = ammoToRemove->getItemCount(); | |
| if (ammoCount > 1) { | |
| auto transformed = g_game().transformItem(ammoToRemove, ammoToRemove->getID(), ammoCount - 1); | |
| (void)transformed; | |
| } else if (ammoCount == 1) { | |
| auto removed = g_game().internalRemoveItem(ammoToRemove); | |
| (void)removed; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/creatures/combat/combat.cpp` around lines 1709 - 1722, The call to
player->updateSupplyTracker(ammoToRemove) happens after g_game().transformItem
or g_game().internalRemoveItem may modify/remove the item, risking sending
invalid item data; fix by recording the needed supply info (e.g.,
ammoToRemove->getID() and ammoToRemove->getItemCount() or any supply-identifier)
or calling player->updateSupplyTracker with that captured info before performing
g_game().transformItem/ internalRemoveItem; update the logic around
ammoToRemove, g_game().transformItem, g_game().internalRemoveItem and
player->updateSupplyTracker so the tracker receives valid pre-modification data.
1 participant
Feature created for https://rookgaardtales.com
Useful if you plan to run baiak projects in canary or custom content related.
This new function allows you to enable rows of auto attacks in your weapons via xml without need of coding extra scripts. It handles animations and range for each try and it also ammo consumption for paladins. I did made a few test and anti crash methods to have this feature available on canary server.
This is only a proposal of a feature I wanted to share with the community, it's up to canary team to merge it or not.
Inspired by x4Cut ability from Final Fantasy 7
Everything has been debuged and tested at https://tibiatales.com
I hope you like it! - Con cariño para la comunidad.
Note - Probably this is safer:
But I've tested it for a while and didn't had any crash.