feat: attr element and damage calculing#3518
Conversation
Auto Sync Fork
majestyotbr
changed the title
|
Testado e funcionando 100%. Falta apenas incluir em data\scripts\talkactions\god\attributes.lua |
Na verdade não já tem attr por padrão no código por isso não adicionei já vem no comando attr o element a menos que você tenha um datapack antigo o que eu usei de base para fazer esse PR já tinha incluido |
|
|
This PR is stale because it has been open 45 days with no activity. |
📝 WalkthroughWalkthroughThese changes introduce an ELEMENT item attribute and widen element damage representation from int16_t to int32_t across items, weapons, and combat calculations. The ELEMENT attribute is added as an enumerated value, integrated into item serialization/deserialization, and used in damage calculations and descriptions as a stored alternative to the default element type value. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/items/weapons/weapons.cpp`:
- Around line 288-295: The current damage split computes ratios using
(physicalAttack + elementalAttack) which can be zero; calculate int32_t denom =
physicalAttack + elementalAttack and only perform the division when denom > 0,
otherwise set physicalDamage and elementalDamage to 0 (or another safe default);
update the blocks around physicalDamage/elementalDamage calculations (variables
physicalDamage, elementalDamage, totalDamage, physicalAttack, elementalAttack)
to use denom to avoid division-by-zero.
🧹 Nitpick comments (5)
src/items/weapons/weapons.hpp (2)
282-282: Redundantvirtualkeyword on override methods.The
virtualkeyword is redundant whenoverrideis already specified. While this is a minor style issue, modern C++ guidelines recommend using onlyoverridefor clarity.♻️ Optional cleanup
- virtual int32_t getElementDamageValue() const override; + int32_t getElementDamageValue() const override;Apply similar changes to Lines 306 and 329.
Also applies to: 306-306, 329-329
286-286: Type consistency: member isuint32_tbut getter returnsint32_t.The
elementDamagemember is declared asuint32_t, butgetElementDamageValue()returnsint32_t. This implicit conversion is generally safe for values withinint32_trange, but for consistency and to avoid potential issues with very large values, consider aligning the types.Also applies to: 312-312
src/items/weapons/weapons.cpp (3)
267-267: Non-English comment should be translated.The comment "Aplica o dano para varinhas" (Portuguese for "Apply damage for wands") should be in English for codebase consistency.
♻️ Suggested translation
- // Aplica o dano para varinhas + // Apply damage for wands
260-273: Code duplication: wand damage handling duplicated between getCombatDamage and internalUseWeapon.The wand-specific damage logic (setting primary damage from weapon damage, clearing secondary damage) is now duplicated in both
getCombatDamage(Lines 181-187) andinternalUseWeapon(Lines 260-273). Consider extracting this to a helper or consolidating the paths to reduce maintenance burden.
475-481: Variable shadowing:itemreassigned within nested scope.At Line 475,
itemis reassigned viaplayer->getWeapon(true), which shadows theitemvariable declared at Line 460. This can cause confusion since the twoitemvariables serve different purposes (weapon slot vs ammo). Consider using a distinct variable name for clarity.♻️ Suggested rename for clarity
if (elementType != COMBAT_NONE) { shouldCalculateSecondaryDamage = true; - item = player->getWeapon(true); - if (item) { - int32_t element = item->getElementDamage(); + const auto &elementItem = player->getWeapon(true); + if (elementItem) { + int32_t element = elementItem->getElementDamage(); elementAttack = (element > 0) ? element : getElementDamageValue(); } else { elementAttack = getElementDamageValue(); }
| int32_t physicalDamage = 0; | ||
| if (physicalAttack > 0) { | ||
| physicalDamage = static_cast<int32_t>(totalDamage * (static_cast<float>(physicalAttack) / (physicalAttack + elementalAttack))); | ||
| } | ||
| int32_t elementalDamage = 0; | ||
| if (elementalAttack > 0) { | ||
| elementalDamage = static_cast<int32_t>(totalDamage * (static_cast<float>(elementalAttack) / (physicalAttack + elementalAttack))); | ||
| } |
There was a problem hiding this comment.
Potential division by zero when both physicalAttack and elementalAttack are zero.
The division (physicalAttack + elementalAttack) at Lines 290 and 294 can be zero if both values are zero, leading to undefined behavior. Although this scenario may be rare for valid weapons, defensive coding is recommended.
🐛 Proposed fix to add division guard
const int32_t physicalAttack = item->getAttack();
const int32_t element = item->getElementDamage();
const int32_t elementalAttack = (element > 0) ? element : getElementDamageValue();
+ const int32_t totalAttackValue = physicalAttack + elementalAttack;
int32_t physicalDamage = 0;
- if (physicalAttack > 0) {
- physicalDamage = static_cast<int32_t>(totalDamage * (static_cast<float>(physicalAttack) / (physicalAttack + elementalAttack)));
+ if (physicalAttack > 0 && totalAttackValue > 0) {
+ physicalDamage = static_cast<int32_t>(totalDamage * (static_cast<float>(physicalAttack) / totalAttackValue));
}
int32_t elementalDamage = 0;
- if (elementalAttack > 0) {
- elementalDamage = static_cast<int32_t>(totalDamage * (static_cast<float>(elementalAttack) / (physicalAttack + elementalAttack)));
+ if (elementalAttack > 0 && totalAttackValue > 0) {
+ elementalDamage = static_cast<int32_t>(totalDamage * (static_cast<float>(elementalAttack) / totalAttackValue));
}🤖 Prompt for AI Agents
In `@src/items/weapons/weapons.cpp` around lines 288 - 295, The current damage
split computes ratios using (physicalAttack + elementalAttack) which can be
zero; calculate int32_t denom = physicalAttack + elementalAttack and only
perform the division when denom > 0, otherwise set physicalDamage and
elementalDamage to 0 (or another safe default); update the blocks around
physicalDamage/elementalDamage calculations (variables physicalDamage,
elementalDamage, totalDamage, physicalAttack, elementalAttack) to use denom to
avoid division-by-zero.
Resolved conflicts: - item_attribute.hpp / items_definitions.hpp: ELEMENT renumbered to 38/46 to avoid collision with MANTRA (37/45) added in HEAD - item.cpp: kept both ATTR_MANTRA and ATTR_ELEMENT serialize/deserialize cases - weapons.cpp: took PR's wand handling (direct damage, secondary=COMBAT_NONE); non-wand else block with item->getElementDamage() was already cleanly merged Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nd new imbuement scroll) Resolved conflict in WeaponMelee::getWeaponDamage: kept HEAD's item->getElementDamage() check (from PR opentibiabr#3518) over PR opentibiabr#3845's plain getElementDamageValue(); dropped unused combinedAttack variable introduced by PR opentibiabr#3845. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
This modification aims to fix the elemental damage, being able to modify the elemental value, working with magic, the damage calculation was also redone, before the elemental or physical damage was too high, leaving the other at zero, so a weapon with atk: 25 and elemental: 100, 1000 simply made the physical not do any damage, this was changed now even if it is 10000 it will do physical damage even if it is little.
Fire sword, elemental damage 100:
Fire sword, elemental damage 1000:
Fire sword, elemental damage 10000:
Of course the physical damage was very low however and compared to the exorbitant value of the elemental damage there still needs to be a balance between the values but one will not cancel the other leaving the value 0
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.