Enhance GroundCraft animations and recipes#1171
Merged
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了一个问题，并留下了一些高层次的反馈：
- 在
TryStartCraftAnimation中，如果后续某个IngredientTake校验失败，你会在已经向_lockedItemIndexes添加了一些索引并发送了SyncItemNoGrab之后返回false，但没有回滚这些更改；建议要么在加锁/同步之前先对所有的 take 做完整校验，要么增加一条回滚路径，以避免部分被锁定的物品处于不一致状态。 IsLockedItemPacket/OnGetData的逻辑把所有相关消息的前两个字节都当作物品索引来处理，但不同的数据包类型（例如SyncItemDespawn、ItemOwner）可能有不同的数据布局；更安全的做法可能是按消息类型分别解析索引，或使用 tModLoader/TShock 的数据包辅助工具，以避免读取到错误的索引。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- In `TryStartCraftAnimation`, if a later `IngredientTake` validation fails you return `false` after having already added some indices to `_lockedItemIndexes` and sent `SyncItemNoGrab`, but without rolling back those changes; consider either validating all takes before locking/syncing or adding a rollback path to avoid leaving partially locked items in an inconsistent state.
- The `IsLockedItemPacket`/`OnGetData` logic treats the first two bytes of all relevant messages as an item index, but different packet types (e.g. `SyncItemDespawn`, `ItemOwner`) may have different layouts; it may be safer to parse indices per-message-type or use the tModLoader/TShock packet helpers to avoid reading the wrong index.
## Individual Comments
### Comment 1
<location path="src/GroundCraft/GroundCraft.Crafting.cs" line_range="141-150" />
<code_context>
+ foreach (DropRecipe recipe in _recipes)
</code_context>
<issue_to_address>
**question (bug_risk):** New crafting loop now stops after the first matching recipe, changing multi-recipe behavior per cluster
Previously `TryCraftCluster` could apply multiple recipes per scan (up to `MaxCraftsPerClusterPerScan`), tracking `craftedRecipeSignatures` to avoid repeats, and only stopped when `available` was exhausted or the limit was hit. The new code returns on the first successful recipe/animation, so clusters that can satisfy multiple recipes now craft only one per scan. If this isn’t intended, consider reintroducing a loop that continues trying other recipes while honoring `MaxCraftsPerClusterPerScan`, or restricting early returns only to cases where animation must be exclusive. If it is intended, please document how `MaxCraftsPerClusterPerScan` should now be interpreted given this changed behavior.
</issue_to_address>帮助我变得更有用！请对每条评论点击 👍 或 👎，我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
TryStartCraftAnimation, if a laterIngredientTakevalidation fails you returnfalseafter having already added some indices to_lockedItemIndexesand sentSyncItemNoGrab, but without rolling back those changes; consider either validating all takes before locking/syncing or adding a rollback path to avoid leaving partially locked items in an inconsistent state. - The
IsLockedItemPacket/OnGetDatalogic treats the first two bytes of all relevant messages as an item index, but different packet types (e.g.SyncItemDespawn,ItemOwner) may have different layouts; it may be safer to parse indices per-message-type or use the tModLoader/TShock packet helpers to avoid reading the wrong index.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TryStartCraftAnimation`, if a later `IngredientTake` validation fails you return `false` after having already added some indices to `_lockedItemIndexes` and sent `SyncItemNoGrab`, but without rolling back those changes; consider either validating all takes before locking/syncing or adding a rollback path to avoid leaving partially locked items in an inconsistent state.
- The `IsLockedItemPacket`/`OnGetData` logic treats the first two bytes of all relevant messages as an item index, but different packet types (e.g. `SyncItemDespawn`, `ItemOwner`) may have different layouts; it may be safer to parse indices per-message-type or use the tModLoader/TShock packet helpers to avoid reading the wrong index.
## Individual Comments
### Comment 1
<location path="src/GroundCraft/GroundCraft.Crafting.cs" line_range="141-150" />
<code_context>
+ foreach (DropRecipe recipe in _recipes)
</code_context>
<issue_to_address>
**question (bug_risk):** New crafting loop now stops after the first matching recipe, changing multi-recipe behavior per cluster
Previously `TryCraftCluster` could apply multiple recipes per scan (up to `MaxCraftsPerClusterPerScan`), tracking `craftedRecipeSignatures` to avoid repeats, and only stopped when `available` was exhausted or the limit was hit. The new code returns on the first successful recipe/animation, so clusters that can satisfy multiple recipes now craft only one per scan. If this isn’t intended, consider reintroducing a loop that continues trying other recipes while honoring `MaxCraftsPerClusterPerScan`, or restricting early returns only to cases where animation must be exclusive. If it is intended, please document how `MaxCraftsPerClusterPerScan` should now be interpreted given this changed behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些高层次的反馈：
- 新的
_craftAnimations和_lockedItemIndexes集合在OnGameUpdate和OnGetData中都会被修改，但没有任何同步机制；可以考虑使用一个专门的锁（或复用_stateLock）来保护它们，或者使用并发集合，以避免游戏循环与网络数据钩子之间产生竞争条件。 - 目前与 Zenith 相关的特定行为是通过
IsZenithRecipe（检查recipe.OutputType == ItemID.Zenith）硬编码的；如果你预期之后会有其他配方需要相同的动画路径，把这部分逻辑迁移到配方的一个标记或配置字段中，可能会比依赖特定的物品 ID 更加清晰。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_craftAnimations` and `_lockedItemIndexes` collections are mutated from both `OnGameUpdate` and `OnGetData` without any synchronization; consider guarding them with a dedicated lock (or reusing `_stateLock`) or using concurrent collections to avoid race conditions between the game loop and net data hooks.
- The Zenith-specific behavior is currently hard-coded via `IsZenithRecipe` (checking `recipe.OutputType == ItemID.Zenith`); if you anticipate other recipes needing the same animation path, it might be cleaner to move this to a recipe flag or configuration field rather than relying on a specific item ID.
## Individual Comments
### Comment 1
<location path="src/GroundCraft/GroundCraft.Animation.cs" line_range="168-155" />
<code_context>
+ NotifyNearby(target, animation.Recipe, animation.OutputStack, animation.ConsumedStacks);
+ }
+
+ private void CancelCraftAnimation(CraftAnimation animation)
+ {
+ foreach (AnimatedIngredient ingredient in animation.Ingredients)
+ {
+ _lockedItemIndexes.Remove(ingredient.Index);
+ if (ingredient.Index < 0 || ingredient.Index >= Main.item.Length)
+ continue;
+
+ WorldItem item = Main.item[ingredient.Index];
+ if (!item.active)
+ continue;
+
+ item.noGrabDelay = 0;
+ item.playerIndexTheItemIsReservedFor = 255;
+ item.velocity = Vector2.Zero;
+ SyncItem(ingredient.Index);
+ NetMessage.SendData(MessageID.ItemOwner, -1, -1, null, ingredient.Index);
+ }
+
+ SpawnDeferredLeftovers(animation);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Cancelling a craft animation appears to duplicate ingredient stacks.
`TryStartCraftAnimation` reduces each ingredient item’s `stack` to the consumed amount and stores the remainder in `DeferredLeftover`. On completion, `CompleteCraftAnimation` removes the ingredient items (`TurnToAir`) and spawns the leftovers, so total stacks stay correct.
`CancelCraftAnimation` instead leaves the reduced-stack items in the world (only unlocking/resetting them) and still calls `SpawnDeferredLeftovers`. So after cancellation you have both the consumed stacks and the leftovers, effectively duplicating items.
Cancellation should either restore the original stacks and skip spawning leftovers, or mirror completion by removing the reduced items and only spawning the appropriate stacks. Alternatively, defer any `stack` modification until completion so both completion and cancellation paths are simpler and non-duplicating.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的代码审查。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The new
_craftAnimationsand_lockedItemIndexescollections are mutated from bothOnGameUpdateandOnGetDatawithout any synchronization; consider guarding them with a dedicated lock (or reusing_stateLock) or using concurrent collections to avoid race conditions between the game loop and net data hooks. - The Zenith-specific behavior is currently hard-coded via
IsZenithRecipe(checkingrecipe.OutputType == ItemID.Zenith); if you anticipate other recipes needing the same animation path, it might be cleaner to move this to a recipe flag or configuration field rather than relying on a specific item ID.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_craftAnimations` and `_lockedItemIndexes` collections are mutated from both `OnGameUpdate` and `OnGetData` without any synchronization; consider guarding them with a dedicated lock (or reusing `_stateLock`) or using concurrent collections to avoid race conditions between the game loop and net data hooks.
- The Zenith-specific behavior is currently hard-coded via `IsZenithRecipe` (checking `recipe.OutputType == ItemID.Zenith`); if you anticipate other recipes needing the same animation path, it might be cleaner to move this to a recipe flag or configuration field rather than relying on a specific item ID.
## Individual Comments
### Comment 1
<location path="src/GroundCraft/GroundCraft.Animation.cs" line_range="168-155" />
<code_context>
+ NotifyNearby(target, animation.Recipe, animation.OutputStack, animation.ConsumedStacks);
+ }
+
+ private void CancelCraftAnimation(CraftAnimation animation)
+ {
+ foreach (AnimatedIngredient ingredient in animation.Ingredients)
+ {
+ _lockedItemIndexes.Remove(ingredient.Index);
+ if (ingredient.Index < 0 || ingredient.Index >= Main.item.Length)
+ continue;
+
+ WorldItem item = Main.item[ingredient.Index];
+ if (!item.active)
+ continue;
+
+ item.noGrabDelay = 0;
+ item.playerIndexTheItemIsReservedFor = 255;
+ item.velocity = Vector2.Zero;
+ SyncItem(ingredient.Index);
+ NetMessage.SendData(MessageID.ItemOwner, -1, -1, null, ingredient.Index);
+ }
+
+ SpawnDeferredLeftovers(animation);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Cancelling a craft animation appears to duplicate ingredient stacks.
`TryStartCraftAnimation` reduces each ingredient item’s `stack` to the consumed amount and stores the remainder in `DeferredLeftover`. On completion, `CompleteCraftAnimation` removes the ingredient items (`TurnToAir`) and spawns the leftovers, so total stacks stay correct.
`CancelCraftAnimation` instead leaves the reduced-stack items in the world (only unlocking/resetting them) and still calls `SpawnDeferredLeftovers`. So after cancellation you have both the consumed stacks and the leftovers, effectively duplicating items.
Cancellation should either restore the original stacks and skip spawning leftovers, or mirror completion by removing the reduced items and only spawning the appropriate stacks. Alternatively, defer any `stack` modification until completion so both completion and cancellation paths are simpler and non-duplicating.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| _stableScans.Remove(ingredient.Index); | ||
| } | ||
|
|
||
| SpawnDeferredLeftovers(animation); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): 取消合成动画时似乎会导致配方材料堆叠数量被复制。
TryStartCraftAnimation 会将每个材料物品的 stack 减少到被消耗的数量，并把多余的部分存放在 DeferredLeftover 中。完成时，CompleteCraftAnimation 会移除这些材料物品（TurnToAir），并生成这些剩余物品，所以总的堆叠数量是正确的。
而 CancelCraftAnimation 则是把减少过堆叠的物品保留在世界中（只解锁/重置它们），同时仍然调用 SpawnDeferredLeftovers。因此在取消之后，世界中会同时存在被消耗的堆叠和剩余的堆叠，相当于复制了物品。
取消操作要么应该恢复原始堆叠并跳过生成剩余物品，要么像完成路径那样移除减少后的物品，只生成对应数量的堆叠。另一种方案是将任何对 stack 的修改推迟到完成时再执行，这样完成和取消路径都会更简单，并且不会产生重复的物品。
Original comment in English
issue (bug_risk): Cancelling a craft animation appears to duplicate ingredient stacks.
TryStartCraftAnimation reduces each ingredient item’s stack to the consumed amount and stores the remainder in DeferredLeftover. On completion, CompleteCraftAnimation removes the ingredient items (TurnToAir) and spawns the leftovers, so total stacks stay correct.
CancelCraftAnimation instead leaves the reduced-stack items in the world (only unlocking/resetting them) and still calls SpawnDeferredLeftovers. So after cancellation you have both the consumed stacks and the leftovers, effectively duplicating items.
Cancellation should either restore the original stacks and skip spawning leftovers, or mirror completion by removing the reduced items and only spawning the appropriate stacks. Alternatively, defer any stack modification until completion so both completion and cancellation paths are simpler and non-duplicating.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR upgrades the GroundCraft plugin with new craft animations (including a dedicated Zenith sequence), expands the built-in default recipe set, and tightens recipe triggering by requiring exact ingredient type matching by default.
Changes:
- Added animated “consumed drop” crafting flow (spiral animation + Zenith finale/trail) and hooked NetGetData to keep animating items unpickable.
- Enabled exact ingredient type matching by default and added runtime stats to track “extra type” rejects.
- Expanded default recipe examples and updated documentation/manifest/plugin index for v1.1.0.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/GroundCraft/README.md | Documents new defaults/options and adds v1.1.0 changelog entry (CN). |
| src/GroundCraft/README.en-US.md | Documents new defaults/options and adds v1.1.0 changelog entry (EN). |
| src/GroundCraft/manifest.json | Updates manifest descriptions to mention exact matching + spiral animations. |
| src/GroundCraft/GroundCraft.Models.cs | Adds animation model types, new config flags/defaults, new runtime stat, and new default recipes. |
| src/GroundCraft/GroundCraft.Effects.cs | Adds FX helpers for craft effect + Zenith finale. |
| src/GroundCraft/GroundCraft.cs | Bumps plugin version to 1.1.0, adds animation constants/state, registers NetGetData hook, disposes animations. |
| src/GroundCraft/GroundCraft.Crafting.cs | Integrates animation updates, exact-type matching, station center selection, and animation-based craft start path. |
| src/GroundCraft/GroundCraft.Commands.cs | Extends /groundcraft info and audit output with new config/stats. |
| src/GroundCraft/GroundCraft.Animation.cs | Implements animation lifecycle, item locking, trail, completion/cancel, and NetGetData reassertion. |
| src/GroundCraft/examples/config.example.json | Updates example config with new defaults and new flags. |
| Plugins.json | Adds/updates plugin index entry for GroundCraft v1.1.0.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ACaiCat
approved these changes
Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by Sourcery
引入带有专属 Zenith 视觉效果的螺旋工艺动画，收紧材料匹配规则，并扩展默认 GroundCraft 配方和文档。
New Features:
Enhancements:
Documentation:
Original summary in English
Summary by Sourcery
Introduce spiral craft animations with dedicated Zenith visuals, tighten ingredient matching, and expand default GroundCraft recipes and docs.
New Features:
Enhancements:
Documentation:
Original summary in English
Summary by Sourcery
引入带有专属 Zenith 视觉效果的螺旋工艺动画，收紧材料匹配规则，并扩展默认 GroundCraft 配方和文档。
New Features:
Enhancements:
Documentation:
Original summary in English
Summary by Sourcery
Introduce spiral craft animations with dedicated Zenith visuals, tighten ingredient matching, and expand default GroundCraft recipes and docs.
New Features:
Enhancements:
Documentation: