Heliostat operation optimization 定日镜操作优化

[burin-ljx]

• Resolved [TODO] 定日镜操作优化 #3753

[github-actions]

好的，作为一名专业的代码审查助理，我对您提供的 Pull Request 代码变更进行了分析。以下是发现的问题及改进建议：

1. 语言文件 (en_us.json / en_ud.json) 问题

问题 1：语言键值对与代码逻辑不匹配

• 问题描述：在 en_us.json 和 en_ud.json 中，新增了两个语言键：
  • screen.anvilcraft.range_no_overlap: "两个相同方块的工作范围不可重叠"
  • screen.anvilcraft.range_overlap: "%1$s×%1$s范围内不允许有另一个相同方块"
• 分析：在 PlacementIntervalsBlockItem.java 中，当 doRangeNoOverlap() 返回 true 时，使用 screen.anvilcraft.range_no_overlap；当返回 false 时，使用 screen.anvilcraft.range_overlap。然而，en_us.json 中的中文翻译与 en_ud.json 中的倒置英文翻译不一致，且 en_us.json 中的翻译是中文，这不符合语言文件的命名规范。
• 建议：
  1. en_us.json 应包含英文翻译。例如：
     • screen.anvilcraft.range_no_overlap: "The working ranges of two identical blocks must not overlap"
     • screen.anvilcraft.range_overlap: "No other identical block is allowed within the %1$s×%1$s range."
  2. en_ud.json 应包含对应的倒置英文翻译。
  3. 检查 ScreenTooltipLang.java 中的英文翻译，确保与 en_us.json 一致。当前 ScreenTooltipLang.java 中的英文翻译是正确的，但 en_us.json 中的中文翻译是错误的。

问题 2：语言文件中的中文翻译

• 问题描述：en_us.json 文件中包含了中文文本，如 "两个相同方块的工作范围不可重叠" 和 "%1$s×%1$s范围内不允许有另一个相同方块"。
• 分析：en_us.json 是美式英语的语言文件，不应包含中文。这会导致游戏在英文环境下显示中文，影响用户体验。
• 建议：将 en_us.json 中的中文翻译替换为正确的英文翻译。中文翻译应放在对应的中文语言文件（如 zh_cn.json）中。

2. PlacementIntervalsBlockItem.java 代码问题

问题 3：canPlace 方法中的坐标计算逻辑错误

• 问题描述：在 canPlace 方法中，BlockPos.betweenClosed 的坐标计算有误。

  Iterable<BlockPos> blockPoss = BlockPos.betweenClosed(
      clickedPos.offset(getIntervalsRadius(), getIntervalsRadius(), getIntervalsRadius()),
      clickedPos.offset(-getIntervalsRadius(), -getIntervalsRadius(), -getIntervalsRadius())
  );

• 分析：BlockPos.betweenClosed 需要两个坐标作为参数，分别代表最小坐标和最大坐标。当前代码中，第一个参数是 clickedPos 加上半径，第二个参数是 clickedPos 减去半径。这导致第一个参数（最大坐标）小于第二个参数（最小坐标），从而生成的迭代范围是空的，无法正确检查周围方块。
• 建议：修正坐标计算，确保第一个参数是最小坐标，第二个参数是最大坐标。

  Iterable<BlockPos> blockPoss = BlockPos.betweenClosed(
      clickedPos.offset(-getIntervalsRadius(), -getIntervalsRadius(), -getIntervalsRadius()),
      clickedPos.offset(getIntervalsRadius(), getIntervalsRadius(), getIntervalsRadius())
  );

问题 4：canPlace 方法中的客户端消息提示逻辑

• 问题描述：在 canPlace 方法中，当检测到重叠时，会向客户端玩家发送消息。但这段逻辑被包裹在 if (level.isClientSide() && player instanceof LocalPlayer localPlayer) 中。
• 分析：canPlace 方法在服务端和客户端都会被调用。在服务端，level.isClientSide() 为 false，因此不会发送消息。在客户端，player 是 LocalPlayer 的实例，因此会发送消息。这看起来是合理的，但需要注意，如果服务端也想要发送消息（例如，通过聊天框），则需要额外的逻辑。
• 建议：当前逻辑对于客户端提示是有效的。如果希望服务端也发送消息（例如，用于服务器日志或管理员通知），可以考虑在服务端也添加类似的逻辑，但使用 player.sendSystemMessage() 而不是 displayClientMessage()。

问题 5：doRangeNoOverlap 方法的命名和语义

• 问题描述：doRangeNoOverlap 方法名暗示“执行范围不重叠”，但其实际作用是返回一个布尔值，指示是否使用“不重叠”的提示信息。
• 分析：方法名 doRangeNoOverlap 不够直观，容易让人误解为它会执行某种操作。实际上，它只是一个配置标志，用于决定显示哪种提示信息。
• 建议：将方法名改为更具描述性的名称，例如 showNoOverlapMessage 或 useStrictOverlapCheck，以更清晰地表达其意图。

3. HeliostatsItem.java 代码问题

问题 6：useOn 方法中的逻辑分支

• 问题描述：useOn 方法中的逻辑分支较多，且存在一些潜在问题。
  • 分支 1：当玩家潜行时，如果物品有数据，则删除数据。但无论是否有数据，都返回 InteractionResult.sidedSuccess(level.isClientSide())。这意味着即使没有数据，也会返回成功，这可能会影响后续的交互。
  • 分支 2：当玩家点击的是定日镜方块时，如果物品有数据，则尝试设置定日镜的照射位置；如果物品没有数据，则尝试从定日镜中读取数据。但这里存在一个逻辑问题：如果定日镜已经有照射位置，且物品有数据，代码会直接返回成功，而没有更新定日镜的照射位置。这可能是设计意图，但需要确认。
  • 分支 3：当玩家点击的是可加热方块时，如果物品有数据，则尝试放置定日镜；如果物品没有数据，则将点击位置存储到物品中。
• 分析：逻辑分支较多，容易出错。特别是分支 2 中，当定日镜已有照射位置且物品有数据时，行为是“静默成功”，这可能不是用户期望的。
• 建议：
  1. 简化逻辑，将不同的交互场景（潜行、点击定日镜、点击可加热方块）拆分为独立的私有方法，以提高代码可读性和可维护性。
  2. 在分支 2 中，当定日镜已有照射位置且物品有数据时，考虑是否应该更新定日镜的照射位置，或者给用户一个明确的反馈（例如，发送一条消息）。
  3. 在分支 1 中，当物品没有数据时，返回 InteractionResult.PASS 而不是 InteractionResult.sidedSuccess，以允许其他交互（如打开 GUI）发生。

4. HeatCollectorBlock.java 代码问题

问题 7：删除了 getStateForPlacement 方法

• 问题描述：删除了 HeatCollectorBlock 中的 getStateForPlacement 方法，该方法原本用于在放置时检查是否与其他集热器重叠。
• 分析：这个检查逻辑被移到了 PlacementIntervalsBlockItem 的 canPlace 方法中。这是一个合理的重构，将放置检查逻辑从方块类移到了物品类中，使得逻辑更集中。
• 建议：确认 HeatCollectorManager 类是否还有其他地方被使用。如果不再需要，可以考虑删除该类以清理代码。

5. ModBlocks.java 代码问题

问题 8：HEAT_COLLECTOR 的注册方式

• 问题描述：HEAT_COLLECTOR 的注册方式从 .simpleItem() 改为了 .item(HeatCollectorBlockItem::new).build()。
• 分析：这是一个正确的修改，因为 HeatCollectorBlockItem 是自定义的 BlockItem，需要显式指定。
• 建议：无。

总结

本次 Pull Request 的主要目的是重构集热器的放置检查逻辑，并引入一个通用的 PlacementIntervalsBlockItem 类来支持其他类似方块（如定日镜）的放置间距限制。整体思路是好的，但在实现细节上存在一些问题，特别是语言文件中的中文翻译和 canPlace 方法中的坐标计算错误，需要修复。此外，HeliostatsItem 中的逻辑可以进一步优化以提高可读性和健壮性。

[github-actions]

代码审查报告

1. 潜在问题：PlacementIntervalsBlockItem.canPlace() 中的循环边界错误

文件： src/main/java/dev/dubhe/anvilcraft/block/item/PlacementIntervalsBlockItem.java

问题描述：

Iterable<BlockPos> blockPoss = BlockPos.betweenClosed(
    clickedPos.offset(getIntervalsRadius(), getIntervalsRadius(), getIntervalsRadius()),
    clickedPos.offset(-getIntervalsRadius(), -getIntervalsRadius(), -getIntervalsRadius())
);

BlockPos.betweenClosed() 的第一个参数应为最小坐标，第二个参数应为最大坐标。但这里第一个参数使用了 +radius，第二个参数使用了 -radius，导致 min > max，生成的迭代器可能为空或行为异常。

改进建议：

Iterable<BlockPos> blockPoss = BlockPos.betweenClosed(
    clickedPos.offset(-getIntervalsRadius(), -getIntervalsRadius(), -getIntervalsRadius()),
    clickedPos.offset(getIntervalsRadius(), getIntervalsRadius(), getIntervalsRadius())
);

2. 潜在问题：HeliostatsItem 构造函数未调用父类构造函数

文件： src/main/java/dev/dubhe/anvilcraft/block/item/HeliostatsItem.java

问题描述：

public class HeliostatsItem extends PlacementIntervalsBlockItem {
    public HeliostatsItem(Block block, Properties properties) {
        super(block, properties);  // 这里调用的是 BlockItem 的构造函数，而非 PlacementIntervalsBlockItem
    }
}

由于 PlacementIntervalsBlockItem 继承自 BlockItem，且未定义新的构造函数，super(block, properties) 实际上调用的是 BlockItem 的构造函数，这是正确的。但建议添加注释说明。

改进建议：
无需修改，但建议添加注释以明确意图。

3. 潜在问题：HeliostatsItem.useOn() 中的逻辑分支

文件： src/main/java/dev/dubhe/anvilcraft/block/item/HeliostatsItem.java

问题描述：
当玩家右键点击已放置的定日镜时，如果物品有数据且方块实体存在，会设置照射位置并返回 SUCCESS。但如果 irritatePos 不为 null，代码仅记录日志后返回 SUCCESS，没有执行任何操作。这可能不是预期的行为。

改进建议：

if (irritatePos == null) {
    blockEntity.setIrritatePos(getData(itemInHand));
    return InteractionResult.SUCCESS;
} else {
    // 处理已有照射位置的情况，例如更新或提示
    AnvilCraft.LOGGER.debug("irritate pos already set: {}", irritatePos);
    // 考虑添加玩家提示
    return InteractionResult.FAIL;
}

4. 潜在问题：语言文件中的键值对删除

文件： src/generated/resources/assets/anvilcraft/lang/en_us.json 和 en_ud.json

问题描述：
删除了 block.anvilcraft.heat_collector.placement_too_close_to_another 键值对，但未检查是否有其他代码引用该键。如果仍有代码使用此键，会导致翻译缺失。

改进建议：
确认所有引用该键的代码已被更新为使用新的键 screen.anvilcraft.range_no_overlap 或 screen.anvilcraft.range_overlap。

5. 潜在问题：PlacementIntervalsBlockItem 的 doRangeNoOverlap() 默认实现

文件： src/main/java/dev/dubhe/anvilcraft/block/item/PlacementIntervalsBlockItem.java

问题描述：
doRangeNoOverlap() 默认返回 false，但 getIntervalsRadius() 是抽象方法。这意味着所有子类都必须实现 getIntervalsRadius()，但可能忘记重写 doRangeNoOverlap()，导致显示错误的提示信息。

改进建议：
考虑将 doRangeNoOverlap() 也设为抽象方法，或提供更明确的文档说明。

6. 潜在问题：HeliostatsItem 中的日志级别

文件： src/main/java/dev/dubhe/anvilcraft/block/item/HeliostatsItem.java

问题描述：

AnvilCraft.LOGGER.debug("irritate pos: {}", irritatePos);

使用 debug 级别，但在生产环境中可能不会输出。如果这是重要的调试信息，建议使用 info 级别或添加条件判断。

改进建议：
根据实际需求调整日志级别，或添加配置控制。

7. 潜在问题：PlacementIntervalsBlockItem 中的客户端消息显示

文件： src/main/java/dev/dubhe/anvilcraft/block/item/PlacementIntervalsBlockItem.java

问题描述：

if (level.isClientSide() && player instanceof LocalPlayer localPlayer) {
    localPlayer.displayClientMessage(...);
}

仅在客户端显示消息，但 canPlace() 方法在服务端也会被调用。服务端玩家不会收到提示，可能导致困惑。

改进建议：
考虑使用网络包或 Player.displayClientMessage() 的替代方案，确保服务端也能向玩家发送消息。

[github-actions]

好的，我已经审查了您提交的代码更改。以下是我发现的问题及改进建议：

1. 潜在逻辑错误：BlockPos.betweenClosed 参数顺序

文件: PlacementIntervalsBlockItem.java (第 65-66 行)

问题: BlockPos.betweenClosed 方法期望第一个参数是最小坐标，第二个参数是最大坐标。但您的代码中，第一个参数是 clickedPos.offset(radius, radius, radius)（更大坐标），第二个参数是 clickedPos.offset(-radius, -radius, -radius)（更小坐标）。这会导致迭代范围为空，从而无法正确检测重叠方块。

Iterable<BlockPos> blockPoss = BlockPos.betweenClosed(
    clickedPos.offset(getIntervalsRadius(), getIntervalsRadius(), getIntervalsRadius()),
    clickedPos.offset(-getIntervalsRadius(), -getIntervalsRadius(), -getIntervalsRadius())
);

建议: 交换两个参数的位置，确保最小坐标在前，最大坐标在后：

Iterable<BlockPos> blockPoss = BlockPos.betweenClosed(
    clickedPos.offset(-getIntervalsRadius(), -getIntervalsRadius(), -getIntervalsRadius()),
    clickedPos.offset(getIntervalsRadius(), getIntervalsRadius(), getIntervalsRadius())
);

2. 潜在逻辑错误：HeliostatsItem.useOn 方法中 hasDataStored 判断

文件: HeliostatsItem.java (第 130 行)

问题: 当玩家右键点击一个已放置的定日镜（Heliostats）时，如果物品中没有存储数据（hasDataStored 为 false），代码会尝试从方块实体中获取 irritatePos 并存储到物品中。但如果 irritatePos 为 null，则 itemInHand.set(ModComponents.HELIOSTATS_DATA, new HeliostatsData(irritatePos)) 会存储一个 null 值，可能导致后续使用时出现 NullPointerException。

} else {
    if (level.getBlockEntity(clickedPos) instanceof HeliostatsBlockEntity blockEntity) {
        BlockPos irritatePos = blockEntity.getIrritatePos();
        if (irritatePos == null) {
            return InteractionResult.SUCCESS;
        } else {
            itemInHand.set(ModComponents.HELIOSTATS_DATA, new HeliostatsData(irritatePos));
            return InteractionResult.SUCCESS;
        }
    }
}

建议: 在存储数据前检查 irritatePos 是否为 null，如果为 null 则不应存储数据，并给玩家适当的反馈。

3. 代码风格问题：HeliostatsItem 构造函数未调用父类 PlacementIntervalsBlockItem

文件: HeliostatsItem.java (第 18-20 行)

问题: HeliostatsItem 继承自 PlacementIntervalsBlockItem，但其构造函数直接调用了 super(block, properties)，而 PlacementIntervalsBlockItem 的构造函数也是 super(block, properties)。这本身没有错误，但 HeliostatsItem 没有实现 getIntervalsRadius() 方法，而是直接继承了 PlacementIntervalsBlockItem 的抽象方法。虽然代码中已经实现了 getIntervalsRadius()，但构造函数中应该明确调用父类构造函数。

建议: 当前代码实际上没有问题，但为了清晰起见，可以添加注释说明 HeliostatsItem 的间距半径为 1。

4. 潜在性能问题：PlacementIntervalsBlockItem.canPlace 中的循环

文件: PlacementIntervalsBlockItem.java (第 64-87 行)

问题: 当 getIntervalsRadius() 返回较大值时（如 HeatCollectorBlockItem 中的 4），BlockPos.betweenClosed 会生成大量方块位置（9x9x9 = 729 个），每次放置时都需要遍历这些位置并检查方块状态，可能会对性能产生一定影响。

建议: 考虑使用更高效的范围检测方法，例如：

• 使用 AABB 和 BlockPos.betweenClosed 但提前计算好范围
• 或者维护一个已放置方块的集合，直接进行集合查找

5. 代码风格问题：HeliostatsItem.useOn 方法过于复杂

文件: HeliostatsItem.java (第 115-170 行)

问题: useOn 方法包含了多层嵌套的条件判断，逻辑较为复杂，难以阅读和维护。

建议: 将不同场景的处理逻辑提取为独立的私有方法，例如：

• handleShiftClickOnHeliostats()
• handleDataTransferToHeliostats()
• handleDataTransferFromHeliostats()
• handlePlaceOnHeatSource()

6. 潜在问题：HeliostatsItem.useOn 中 AnvilCraft.LOGGER.debug 调用

文件: HeliostatsItem.java (第 140 行)

问题: 当定日镜已有 irritatePos 时，代码只打印了调试日志，但没有给玩家任何反馈。玩家可能不知道操作已成功或失败。

} else {
    AnvilCraft.LOGGER.debug("irritate pos: {}", irritatePos);
    return InteractionResult.SUCCESS;
}

建议: 给玩家显示一条消息，告知定日镜已配置或无法重复配置。

7. 代码风格问题：PlacementIntervalsBlockItem 中 doRangeNoOverlap 方法命名

文件: PlacementIntervalsBlockItem.java (第 33 行)

问题: 方法名 doRangeNoOverlap 的命名不够清晰，do 前缀通常用于执行操作的方法，而这里是一个查询方法。

建议: 重命名为 isRangeNoOverlap() 或 shouldCheckNoOverlap()，以更清晰地表明这是一个布尔查询方法。

8. 潜在问题：HeatCollectorBlockItem 中 doRangeNoOverlap 返回 true

文件: HeatCollectorBlockItem.java (第 12-14 行)

问题: HeatCollectorBlockItem 的 doRangeNoOverlap 返回 true，这意味着当检测到重叠时会显示 "The working ranges of two identical blocks must not overlap" 的消息。但根据 PlacementIntervalsBlockItem.canPlace 的逻辑，当 doRangeNoOverlap 为 true 时，显示的是 screen.anvilcraft.range_no_overlap 消息，而当为 false 时，显示的是 screen.anvilcraft.range_overlap 消息。请确认这个行为是否符合预期。

建议: 确认 HeatCollectorBlockItem 是否真的需要显示 "工作范围不能重叠" 的消息，还是应该显示 "范围内不能有其他方块" 的消息。

总结

主要问题集中在：

1. 严重问题: BlockPos.betweenClosed 参数顺序错误，会导致范围检测完全失效
2. 潜在问题: HeliostatsItem 中可能存储 null 数据
3. 代码质量: 部分方法过于复杂，可读性差
4. 用户体验: 缺少给玩家的反馈消息

请优先修复第 1 个问题，因为它会导致功能完全失效。