Skip to content

fix: 修复多重飞镖命中多个怪物时可能不扣血的问题#733

Open
skykiseki wants to merge 1 commit into
BeiDouMS:masterfrom
skykiseki:fix-avenger-distance-check-20260612
Open

fix: 修复多重飞镖命中多个怪物时可能不扣血的问题#733
skykiseki wants to merge 1 commit into
BeiDouMS:masterfrom
skykiseki:fix-avenger-distance-check-20260612

Conversation

@skykiseki

Copy link
Copy Markdown
Contributor

改动点汇总

  • 为隐士多重飞镖和夜行者多重飞镖增加专门的距离检测豁免判断。
  • 修复这类穿透型飞镖技能缺少 Skill.wz 攻击范围框时,通用距离检测依赖怪物 bbox 导致正常命中被跳过的问题。
  • 保留原有全屏技能、蓄能类技能的距离检测豁免逻辑,仅追加多重飞镖相关判断,降低对其他技能的影响。

客户端影响

  • 本次改动仅调整服务端伤害结算前的距离校验逻辑。
  • 不涉及 WZ/XML、客户端资源或补丁内容,客户端无需更新。

验证

  • 已确认分支相对 upstream/master 只有本次 1 个提交。
  • 已确认 diff 只包含本次服务端修复文件。
  • 已完成服务端编译验证。
  • 已完成乱码校验,未发现异常字符。

@github-actions

Copy link
Copy Markdown

🤖 AI Code Review

Let me analyze this PR carefully.

This is a Java code change in a MapleStory v83 server project. The change is in AbstractDealDamageHandler.java, which deals with damage handling in the game.

The diff adds a new method isPiercingProjectileWithoutAttackBox that checks if a skill should be exempt from the distance hack check, specifically for the Avenger skill (which exists in both Hermit and NightWalker classes).

Let me analyze the code:

  1. The change modifies shouldSkipDistanceHackCheck to include the new condition
  2. The new method isPiercingProjectileWithoutAttackBox:
    • First checks if the attack effect has a bounding box - if yes, returns false (don't skip)
    • If no bounding box, checks if it's the Avenger skill (Hermit or NightWalker version)

Potential issues to consider:

  1. Magic numbers/skill IDs: The code references Hermit.AVENGER and NightWalker.AVENGER which are likely constants. This is good practice.

  2. Logic correctness: The Avenger is a piercing projectile skill. The comment explains that the skill has no level/lt or level/rb in Skill.wz (the game's data file), and the client sends hits from the piercing projectile path. This makes sense - if there's no attack bounding box available, the generic distance check might falsely flag valid hits as distance hacks.

  3. Order of checks: The new check is placed last in the OR chain, which is good. The bounding box check is done first inside the method, which is an early return - efficient.

  4. Null safety: The method checks attackEffect != null before calling hasBoundingBox(). Good.

  5. Maintainability: The method has a clear Javadoc comment explaining why this exception exists. Good.

  6. Consistency: The naming and style follow the existing code conventions.

  7. Potential concern: The check attackEffect.hasBoundingBox() - I need to trust this is the correct method name. If this method doesn't exist or is named differently, it would be a compile error.

  8. Logic concern: The condition says "if hasBoundingBox is true, return false (don't skip)". This means: only skip the distance check when there's NO bounding box AND it's Avenger. This is the correct logic for the exception.

Let me also think about whether this could be exploited. Could a player somehow trigger this exception for other skills? The check is very specific - it requires the skill ID to be exactly Hermit.AVENGER or NightWalker.AVENGER. These are specific skill IDs that can't be faked by the client (they come from the server's skill data). So this should be safe.

Actually, wait - I need to think more carefully. The shouldSkipDistanceHackCheck function controls whether the distance hack check is skipped. By adding this exception, Avenger attacks from these two classes will skip the distance check under certain conditions. This could potentially be a vector for a speed hack or teleport hack if the condition is too broad.

The condition requires:

  1. attackEffect is not null (skill was used)
  2. attackEffect.hasBoundingBox() returns false (no attack bounding box in the skill data)
  3. The skill is Avenger (either Hermit or NightWalker version)

Since condition 2 and 3 are both based on server-side data (the attack effect comes from the server's skill definitions, and the skill ID is validated by the server), this should be safe from client-side exploitation.

Overall, this looks like a reasonable bug fix that addresses a specific issue with the Avenger skill's hit detection. The code is well-documented and follows existing patterns.

Let me think about the severity of potential issues:

  • Security: Low risk. The exemption is gated by server-side skill data.
  • Code quality: Good. Follows existing patterns, has documentation.
  • Performance: Negligible. One more method call in a conditional check.

I don't see any significant issues with this PR. The change is targeted, well-documented, and addresses a specific bug.

One minor observation: The method name isPiercingProjectileWithoutAttackBox is descriptive but the implementation actually only checks for Avenger specifically. The name suggests it might be more general than it is. But this is a minor naming concern.

Let me write the review.

📋 PR 概览

为「穿刺型投射物」技能(双职业的 Avenger)补充了距离外挂检测的豁免逻辑,因其 Skill.wz 缺少 attack box 数据,原有的几何距离校验会误判合法攻击。

⚠️ 潜在问题

[🟢 低危] 方法名与实现范围略有偏差
isPiercingProjectileWithoutAttackBox 名称暗示通用性,但实现仅硬编码了 Avenger 两个职业分支。若后续出现其他同类技能(同样是穿刺投射物且无 attack box),需再次修改此方法。建议在 Javadoc 中明确标注"当前仅覆盖 Avenger",或考虑抽取技能集合为常量列表。

[🟢 低危] 豁免条件仅依赖 attackEffect 状态
当前逻辑是"无 bounding box 即豁免"。若未来 Avenger 某个等级/变种补齐了 attack box 数据,hasBoundingBox() 变为 true,豁免将自动失效——这是期望行为,但建议在 Javadoc 中显式说明此退化路径,避免后续维护者误以为是 bug。

[🟢 低危] 安全性边界
豁免条件同时依赖 skillId(服务端常量)和 attackEffect(服务端数据),客户端无法伪造,因此不存在利用面。但作为对外挂检测的弱化点,建议在 commit/PR 描述中明确这是官方客户端行为兼容而非容忍外挂,便于审计追溯。

💡 改进建议

  1. 抽取技能集合为命名常量:将 Hermit.AVENGERNightWalker.AVENGER 放入类似 PIERCING_PROJECTILE_SKILL_IDSSet<Integer> 中,配合 isPiercingProjectileWithoutAttackBox 中的 stream 判断,扩展性更佳,方法名也名副其实。

  2. 补充回归测试用例:建议在 AbstractDealDamageHandler 的测试目录下增加 Avenger 双职业的命中场景测试,断言在"无 attack box"条件下不会触发距离外挂标记,防止后续重构破坏该豁免。

  3. Javadoc 完善:补充说明"当 Skill.wz 后续补齐 lt/rb 后,本豁免将自动失效"以及"两个职业 ID 引用同一逻辑的原因(双职业同名技能)",降低维护成本。

📊 评分

维度 评分 说明
代码质量 ⭐⭐⭐⭐ 风格统一、注释清晰、提前 return 控制流合理
安全性 ⭐⭐⭐⭐⭐ 双重服务端数据校验,无客户端可利用面
可维护性 ⭐⭐⭐⭐ 命名略大但可接受,扩展性有改进空间

总结:这是一次定位精准、文档充分的小型 bugfix,未发现高危问题。代码质量整体良好,可直接合入;如能在命名/常量化上做小幅优化则更佳。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant