Skip to content

421 amulets will not deactivate its subtypes when not equipped 修复四合一护符在未装备时,使得子类型护符失效的问题#3806

Open
YocyCraft wants to merge 2 commits into
Anvil-Dev:dev/1.21/1.6from
YocyCraft:dev/1.21/amulet_fix
Open

421 amulets will not deactivate its subtypes when not equipped 修复四合一护符在未装备时,使得子类型护符失效的问题#3806
YocyCraft wants to merge 2 commits into
Anvil-Dev:dev/1.21/1.6from
YocyCraft:dev/1.21/amulet_fix

Conversation

@YocyCraft

Copy link
Copy Markdown
Contributor

@anvil-craft

Copy link
Copy Markdown

⚠️ Dangerous command requires approval:

# Search for isEnabled usage in the codebase via GitHub search
curl -sL "https://api.github.com/search/code?q=repo:Anvil-Dev/AnvilCraft+isEnabled+language:java&per_page=10" 2>/dev/null \
  | python3 -...

Reason: Security scan — [HIGH] Pipe to interpreter: curl | python3: Command pipes output from 'curl' directly to interpreter 'python3'. Downloaded content will be executed without inspection.
Safer: tirith run https://api.github.com/search/code?q=repo:Anvil-Dev/AnvilCraft+isEnabled+language:java&per_page=10 — or: vet https://api.github.com/search/code?q=repo:Anvil-Dev/AnvilCraft+isEnabled+language:java&per_page=10 (https://getvet.sh); script execution via -e/-c flag

Reply /approve to execute, /approve session to approve this pattern for the session, /approve always to approve permanently, or /deny to cancel.

public void inventoryTick(ServerPlayer player, ItemStack amulet, boolean isEnabled) {
for (AmuletType type : this.types) {
type.inventoryTick(player, amulet, isEnabled);
type.inventoryTick(player, amulet, isEnabled

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [Checkstyle] <com.puppycrawl.tools.checkstyle.checks.whitespace.OperatorWrapCheck> reported by reviewdog 🐶
'||' should be on a new line.

@anvil-craft

Copy link
Copy Markdown

代码审查摘要

PR #3806 — 修复四合一护符在未装备时使子类型护符失效的问题
作者: YocyCraft
文件变更: 1 个文件,+3 行 / -1 行


变更概述

修改 FourToOneAmuletType.inventoryTick() 方法,在四合一护符未装备(isEnabled=false)时,不是直接传 false 给子类型护符,而是检查玩家背包中是否还单独持有该子类型护符。如果持有,则仍传 true 使效果保持激活。


🔴 关键问题

无。 修复逻辑正确,最小化改动,精准定位问题。


💡 建议

1. amulet 参数在回退路径中的语义
isEnabled=false 但玩家有子类型护符时,修复将 true 传给 type.inventoryTick(player, amulet, true),但 amulet 参数仍指向四合一护符的 ItemStack,而非子类型自身的。若某个子类型的 Effect.inventoryTick() 实现依赖 amulet 参数获取物品数据(如耐久、NBT),可能行为异常。

不过该行为与原来四合一激活时的逻辑一致(原来也是传四合一的 amulet 给所有子类型),因此这不是新引入的问题。可留作后续改进。

2. equals() 依赖引用相等性
AmuletType 未覆写 equals() 方法,entry.getKey().value().equals(type) 依赖 JVM 引用相等性。这在当前代码中是正确的——两边都指向同一个注册单例(Holder.value() 返回注册对象,this.types 中的也是通过 Supplier::get 从注册表获取)。

但如果未来有人直接 new AmuletType(...) 而非通过注册表创建实例,此处的 equals 可能不匹配。建议在 AmuletType 中添加基于 registry key 的 equals() / hashCode(),或改用 Holder 做直接比较。


✅ 看起来不错

  • 修复精准:只在 isEnabled=false 的分支增加额外检查,不影响正常装备时的行为
  • 短路优化isEnabled || stream().anyMatch(...) 中,当四合一装备时 isEnabled=true,OR 短路不会触发 stream 求值,无额外性能开销
  • 正确的导入:新增 import dev.dubhe.anvilcraft.api.amulet.AmuletManager; 语句添加正确
  • 空安全HashMultimap.entries() 返回空集合而非 null.stream() 调用安全
  • 无递归风险:四合一护符的 AmuletTypeFourToOneAmuletType,而子类型是独立的注册条目,anyMatch 不会把四合一本身误认为子类型

结论:✅ Approve(合并建议)

修复清晰、准确、最小化,正确解决了 #2868 所述的问题。可合并。

@anvil-craft

Copy link
Copy Markdown

现在我已经获得了足够的上下文,可以进行完整的审查。


代码审查摘要:PR #3806

结论:评论(1 条建议,无阻塞性问题)


🎯 变更范围

仅修改了一个文件:FourToOneAmuletType.java

src/main/java/dev/dubhe/anvilcraft/api/amulet/type/FourToOneAmuletType.java  |  1 file, +3 lines, -1 line

✅ 问题理解正确

原 bug(#2868):当四合一护符未装备时(isEnabled = false),inventoryTick 直接对所有子类型护符传入 false,导致即使玩家在手中持有子类型护符,该子类型的 effect 也会被重置/禁用。

修复逻辑:在 isEnabled = false 时,额外检查玩家当前持有的物品中是否有该子类型的独立护符。如果有,则对该子类型传入 true,保持其效果激活。


✅ 代码实现分析

变更的核心行(第 17-20 行):

// 改前
type.inventoryTick(player, amulet, isEnabled);

// 改后
type.inventoryTick(player, amulet, isEnabled ||
    AmuletManager.INSTANCE.getAmuletsFromInventory(player).entries().stream()
        .anyMatch(entry -> entry.getKey().value().equals(type)));

逐层分析链路的正确性:

链路节点 分析 结论
getAmuletsFromInventory(player) 返回 HashMultimap — 内部新建 ArrayList,不可能返回 null ✅ 安全
.entries() Guava SetMultimap.entries() 返回 Set<Entry<K,V>>,空集时也不会 null ✅ 安全
.stream().anyMatch(...) 空流返回 false,`
entry.getKey().value() Holder<AmuletType> 解包出实际类型实例 ✅ 正确
.equals(type) 比较子类型实例 ✅ 依赖 AmuletType 的 equals(需确认继承自 Object 或重写了)

💡 建议

1. 考虑在四合一护符已装备时跳过库存检查(性能优化)

当前代码中,当 isEnabled = true(四合一已装备)时,|| 短路求值,getAmuletsFromInventory 不会被调用 → 已经是最优路径 ✅

isEnabled = false,每个 tick 对每个子类型(最多 4 个)都调用 getAmuletsFromInventory。该方法内部遍历 amuletFinders(主手 + 副手),对于 AmuletBox 还会递归展开。这是一个微小但可接受的性能开销,因为:

  • 通常只会查到 1-3 个物品
  • 仅在四合一未装备时触发(玩家切换装备的场景)
  • 不涉及网络/IO

无需优化,但值得留意。

2. 关于 AmuletType.equals() 的隐式依赖

entry.getKey().value().equals(type) 依赖于 AmuletTypeequals() 实现。查看 AmuletType 类,它使用了 @Getter@Accessors(fluent = true) 注解,但没有 @EqualsAndHashCode。这意味着 equals()Object 的默认引用比较(==)。

这在此上下文中是安全的,因为:

  • type 来自 this.types 列表(Lists.transform 中的 Supplier::get),每个子类型从注册表懒加载一次
  • getKey().value()HashMultimap 返回的 Holder 中解包,对应的是同一个注册表对象

但如果子类型是通过不同方式创建的副本(clone),引用比较会失败。不过从当前架构看,护符类型是通过 ModRegistries 注册的单例,不存在副本问题。

结论:当前 OK,如果将来有副逻辑需注意。


✅ 看起来不错

  • Import 已添加dev.dubhe.anvilcraft.api.amulet.AmuletManager
  • 无空安全风险:所有方法链都有明确保证的非 null 返回
  • 短路优化:已装备时跳过查询
  • 修复逻辑清晰:3 行变更,语义精确对应 bug 描述
  • 无资源泄露/副作用:纯查询操作,不修改状态
  • 兼容性问题:不影响 matches()shouldImmuneDamage() 等其他方法

📋 总结

这是一个精确、低风险、高针对性的修复。3 行代码 + 1 行 import 就完整解决了 #2868 描述的问题。逻辑上无阻塞性问题,建议直接合并。


由 Hermes Agent 审查

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.

[Bug] 猫、狗、寂静护符不生效

2 participants