Implemented Procedural Process Recipe 实现了方块流程处理配方#3589
Conversation
# Conflicts: # src/main/java/dev/dubhe/anvilcraft/data/recipe/RecipeHandler.java # src/main/java/dev/dubhe/anvilcraft/init/block/ModBlockEntities.java # src/main/java/dev/dubhe/anvilcraft/init/recipe/ModRecipeTypes.java
# Conflicts: # src/main/java/dev/dubhe/anvilcraft/init/block/ModBlockEntities.java
Reasoning Details我们被问到:请 review 以下代码更改。然后给出了一个很长的diff文件列表。作为代码审查助理,我们需要识别潜在问题,如代码风格违规、逻辑错误、安全漏洞等,并提供改进建议。需要以简洁明了的方式列出这些问题及相应的改进建议。我们分析这些变更。这是一个Minecraft mod的代码更改,主要涉及新的“Procedural Process”配方系统,添加了redstone_computer相关的方块、物品、配方、语言文件等。 注意:review主要是针对代码更改本身,而不是整个项目。我们要从diff中看出有没有明显的问题。 让我们逐部分分析:
改进建议:
由于这是代码审查,我们应该给出具体的建议。 我们还要检查代码风格:使用lombok的@Getter,似乎可以。但注意ProceduralProcessStep中有@Setter。在加载时修改stepIndex和ppRecipeId,使用setter是可以的。但应该考虑使用final字段并在构造时设置?因为它们在后续可能会被修改?实际上它们是用于保存从数据包加载后的信息,加载后应该就固定了。但这里在initialize中修改了step的stepIndex和ppRecipeId,这意味着step对象是可变的。另一种做法是在构建配方时就设置好这些字段。但当前做法也可以工作。 在checkAnyMatches中,当wip为null且possibleSteps为空时,代码会返回false,没问题。但需要确保possibleSteps不为null(虽然Map.get返回null,但后面遍历会NullPointerException)。代码中直接for (ProceduralProcessStep step : possibleSteps),如果possibleSteps为null会抛NullPointerException。这是一个bug!在获取Map时没有做null检查。应该先检查possibleSteps是否为null。这个必须修复。 另外,在wip存在时,获取recipeHolder后,如果没有找到,那么后面的代码会尝试访问recipeHolder.get(),但recipeHolder是Optional,需要检查isPresent()。代码中已经检查了recipeHolder.isPresent(),所以没问题。但在获取recipeHolder.get().value()之后,需要确认它是ProceduralProcessRecipe。代码中也做了instanceof检查。所以安全。 还有一个性能问题:每次铁砧落地时都会调用checkAnyMatches,即使没有ProceduralProcess配方,也会检查PROCEDURAL_PROCESS_EXIST_STEP_INQUIRY。这个Set可能很大但只是contains操作,影响不大。 现在,列出问题和改进建议。 应该用中文回答,简洁明了。 潜在问题与改进建议1. 单步 Procedural Process 配方无法完成问题:当配方只有一个步骤( 2. 空指针风险:
|
# Conflicts: # src/main/java/dev/dubhe/anvilcraft/init/block/ModBlockEntities.java
ZhuRuoLing
left a comment
There was a problem hiding this comment.
Hermes Review — PR #3589
Implemented Procedural Process Recipe 实现了方块流程处理配方
| 元数据 | 值 |
|---|---|
| Author | CgAu3 (Abslb) |
| Base | dev/1.21/1.6 → procedural |
| Files Changed | 38 |
概述
该 PR 实现了一个完整的 序列装配配方(Procedural Process Recipe)系统,允许玩家通过多步骤铁砧处理流程来制作方块。主要变更包括:
- 新配方系统:
ProceduralProcessRecipe+ProceduralProcessStep+ProceduralProcessStepManager— 支持多步骤、多循环(loop)的方块处理流程 - 新方块:
RedstoneComputerBlock(红石计算机,#3360 的一部分)和WipBlock(Work In Progress 方块,用于中间态显示) - 新配方类型:
ReversedSmearAlikeRecipe— 序列装配中使用的"反向涂抹"配方(伪闪炼、时移、中子辐照) - 配方加载与注册:ProceduralProcessRecipeLoader、ModRecipeTypes、RecipeManagerMixin 集成
- 数据生成:redstone_computer 和 spacetime_supercomputer 的序列装配配方
🔴 严重问题
1. RedstoneComputerBlock 不发出红石信号
RedstoneComputerBlock.java 设置了 isSignalSource() → true 并注册了 POWERED 属性,但 没有重写 getSignal() 或 getDirectSignal() 方法。这意味着即使 POWERED=true,它也不会向相邻方块输出红石信号。实际上这个方块目前只有装饰效果。
// 缺少以下方法:
@Override
protected int getSignal(BlockState state, BlockGetter level, BlockPos pos, Direction direction) {
return state.getValue(POWERED) ? 15 : 0;
}建议: 添加上面的 getSignal() 方法,或者如果该方块不应发出信号,则移除 isSignalSource() 和 POWERED 属性。
2. WipBlockEntity.loadAdditional 空值风险
WipBlockEntity.java:58:
recipeId = ResourceLocation.parse(tag.getString("Recipe"));如果 NBT 中不存在 "Recipe" 标签(如初始放置或损坏存档),tag.getString("Recipe") 返回空字符串 "",然后 ResourceLocation.parse("") 会抛出 ResourceLocationSyntaxException。
建议: 加上 tag.contains("Recipe") 判断:
if (tag.contains("Recipe")) {
recipeId = ResourceLocation.parse(tag.getString("Recipe"));
}3. ProceduralProcessRecipe.matches() 不验证 recipeId
ProceduralProcessRecipe.java:106-107:
public boolean matches(...) {
WipBlockEntity wip = getWipBlockFromContext(ctx);
if (wip == null) return false;
return wip.getStepCount() >= steps.size();
}该方法只检查 WIP 方块的 stepCount 是否达到步骤总数,但 不验证该 WIP 方块是否正在执行同一个 recipe。虽然在实际执行中 checkAnyMatches 会根据 recipeId 找到正确的配方,但这里的 matches() 本身是不安全的。
⚠️ 警告
4. getResultItem 对 BlockState 的处理有误
ProceduralProcessRecipe.java:124:
BlockState state = resultBlock.state();
if (state.isEmpty() || state.isAir()) return Items.ANVIL.getDefaultInstance();ChanceBlockState.state() 返回的是 BlockState 而非 Optional<BlockState>,所以 state.isEmpty() 在这里无效(BlockState 没有 isEmpty() 方法——它用的是 isAir())。需要确认 ChanceBlockState 的 API。
5. WipBlockEntityRenderer 使用了错误的渲染类型
WipBlockEntityRenderer.java:49:
buffer.getBuffer(RenderTypeHelper.getMovingBlockRenderType(renderType)),getMovingBlockRenderType 是给 正在移动的方块(如活塞推动)使用的,不是给静态 BlockEntity 渲染的。对于静态渲染,应该直接使用原始 renderType:
buffer.getBuffer(renderType),这可能导致半透明或 cutout 类型的方块出现渲染异常。
6. ModBlocks 中 RedstoneComputer 的 datagen 引用了 PulseGeneratorBlock 的属性
ModBlocks.java:3747-3777:
.with(PulseGeneratorBlock.FACING, Direction.SOUTH) // 应该用 RedstoneComputerBlock 的
.with(PulseGeneratorBlock.POWERED, false) // 应该用 RedstoneComputerBlock 的虽然 PulseGeneratorBlock.FACING 和 PulseGeneratorBlock.POWERED(都来自 BlockStateProperties)功能上不会出错,但这是一个代码异味(code smell)——应该引用自己类的属性。
7. 缺少 JEI/Jade 集成
PR 代码中已标注 // TODO: JEI(查看配方)和Jade(查看方块实体内容:哪个配方第几步)支持。没有这些集成,玩家在游戏中无法发现或查看序列装配配方的流程,这将严重影响可用性。建议创建 issue 跟踪。
8. checkAnyMatches 中多余的 sendBlockUpdated
在 ProceduralProcessStepManager.java 中多处使用了 sl.setBlock(pos, entry.getKey(), Block.UPDATE_ALL) 后又立即调用 sl.sendBlockUpdated(pos, ...)。UPDATE_ALL flag 已经包含了客户端更新,所以 sendBlockUpdated 是多余的,会触发不必要的网络流量。
💡 建议
9. 性能:第一步查询时重复创建 context
在 checkAnyMatches 中处理第一步(无 WIP 方块时),代码为每个可能的 step 创建了一个新的 InWorldRecipeContext,并在循环中重复从 level 获取信息。如果 PROCEDURAL_PROCESS_FIRST_STEP_INQUIRY 中有大量第一步选项,这会带来不必要的开销。
10. ReversedSmearAlikeRecipe 的 codec 嵌套
ReversedSmearAlikeRecipe.java:74:
ChanceBlockState.CODEC.codec()
.fieldOf("result")ChanceBlockState.CODEC 已经是 MapCodec<ChanceBlockState> 了,这里又调用了 .codec() 再包装成 FieldDecoder,可简化为直接用 ChanceBlockState.CODEC.fieldOf("result")。
11. Grass Block 配方修改的副作用
PR 修改了 grass_block.json 的 block_compress 配方和 BlockSmearRecipeLoader 中的一条调用(将苔藓+土=草方块的配方从涂抹改回压合,同时新增了反向涂抹配方)。这个语义变更应该在 PR 描述中明确说明。
✅ 做得好的地方
- 架构设计清晰:将步骤(Step)和配方(Recipe)分离,支持 multi-loop,设计灵活
- 对
/reload的支持:每次数据包重载时清空并重建查询表 - 错误处理:当 step 不是
AbstractProcessRecipe时报警告并跳过 - 校验完善:
ProceduralProcessRecipeBuilder.validate()对 loop 次数、空步骤进行了校验 - WipBlock 的掉落物处理:
getDrops()根据initialBlock返回对应掉落,设计合理 - 比较器输出:WipBlock 实现了
getAnalogOutputSignal,让红石比较器可以读取进度
审核结论
| 类别 | 数量 |
|---|---|
| 🔴 严重 | 3 |
| 5 | |
| 💡 建议 | 3 |
建议: 至少需要修复第 1、2 项(红石信号输出、NPE 风险)再合并。第 4、5 项也建议在合并前处理。JEI 集成(第 7 项)可推迟到后续 PR,建议创建 issue 跟踪。
Reviewed by Hermes Agent
Resolved #3361
写了#3360 的一部分:注册了红石计算机及其配方,并让其放置朝向玩家,不过没有实现任何功能