I got a bug reported near the beginning of the month about recipes not working properly. Shift-clicking on one of the custom recipes would cause a whole bunch of useless items to be created. I had accounted for this, but not properly, and when I tried to fix it I found out I had made some assumptions that all but break our custom recipes.
The Report
I got a notification about a new issue in my bug tracker. Basically, I hadn’t thoroughly tested shift-clicking, and it led to some odd behaviour:
“You can craft an Empty butterfly scroll using an empty net, paper, and iron nugget. This means that if I have a butterfly in the net, and I hold shift while crafting, it makes one filled butterfly scroll, and 63 empty ones.”
Ah. This can break the flow when crafting items, as it behaves in a way that players wouldn’t expect. I had implemented the ability to get the paper back if you use an empty scroll, but I had forgotten about the iron nuggets.
But the real problem is that you can craft an empty scroll at all. Really, the recipe shouldn’t be useable at all if you have an empty butterfly net.
Thankfully, these are both things that are easy to fix.
Fixing All the Things
The first fix was to enable the player to get the nuggets back if they somehow did end up with empty scrolls. I just added nuggets to the player’s inventory when they use the scroll.
/**
* For replacing empty scrolls with paper.
* @param player The interacting player.
* @param hand The hand holding the scrolls.
* @param stack The scroll item stack.
*/
private void replaceWithPaper(@NotNull Player player,
@NotNull InteractionHand hand,
ItemStack stack) {
player.setItemInHand(hand, new ItemStack(Items.PAPER, stack.getCount()));
player.addItem(new ItemStack(Items.IRON_NUGGET, stack.getCount()));
}This works with full stacks, so it should allow players who experience this bug to get their resources back. This isn’t a real fix though. We need to prevent them from being crafted altogether.
There’s no easy way to programmatically block crafting (if there’s any at all). The only way I could see to do it was to create separate items for the Empty Butterfly Net and Full Butterfly Net. By doing this, I can set the recipe to require the full net, and it will no longer work if the butterfly net is empty.
public static final RegistryObject<Item> BUTTERFLY_NET = INSTANCE.register(ButterflyNetItem.EMPTY_NAME,
() -> new ButterflyNetItem(new Item.Properties().stacksTo(1)));
public static final RegistryObject<Item> BUTTERFLY_NET_FULL = INSTANCE.register(ButterflyNetItem.FULL_NAME,
() -> new ButterflyNetItem(new Item.Properties().stacksTo(1)));By changing the recipes to reference butterfly_net_full specifically, the player won’t be able to use an empty net in the recipe.
This isn’t quite everything though. We need to switch between the two items when a player catches or releases a butterfly. We modify the ButterflyNetItem in order to support this. First, we set the item in hand to a full net when the player catches a butterfly.
@Override
public boolean onLeftClickEntity(ItemStack stack, Player player, Entity entity) {
if (entity instanceof Butterfly) {
ItemStack newStack = new ItemStack(ItemRegistry.BUTTERFLY_NET_FULL.get(), 1);
CompoundTag tag = newStack.getOrCreateTag();
if (!tag.contains(CompoundTagId.CUSTOM_MODEL_DATA) ||
!tag.contains(CompoundTagId.ENTITY_ID)) {
tag.putInt(CompoundTagId.CUSTOM_MODEL_DATA,1);
tag.putString(CompoundTagId.ENTITY_ID, Objects.requireNonNull(entity.getEncodeId()));
entity.discard();
player.setItemInHand(InteractionHand.MAIN_HAND, newStack);
player.playSound(SoundEvents.PLAYER_ATTACK_SWEEP, 1F, 1F);
return true;
}
}
return super.onLeftClickEntity(stack, player, entity);
}Then we switch back to an empty net when the player releases a butterfly.
/**
* Right-clicking with a full net will release the net.
* @param level The current level.
* @param player The player holding the net.
* @param hand The player's hand.
* @return The result of the action, if any.
*/
@Override
@NotNull
public InteractionResultHolder<ItemStack> use(@NotNull Level level,
@NotNull Player player,
@NotNull InteractionHand hand) {
ItemStack stack = player.getItemInHand(hand);
@@ -128,8 +133,9 @@
(int) lookAngle.z);
Butterfly.spawn(player.level(), new ResourceLocation(entityId), positionToSpawn, false);
ItemStack newStack = new ItemStack(ItemRegistry.BUTTERFLY_NET.get(), 1);
player.setItemInHand(hand, newStack);
return InteractionResultHolder.success(stack);
}You’ll notice I still use the same class for the empty net as I do for the full net. These could be split into two separate classes, and I may do that in the future, but for now it helps maintain backwards compatibility.
When players upgrade to this new version, all their full nets will be “empty” versions of the item. Since the release code still exists, players will be able to release the butterfly, then catch it again so they have the new “full” butterfly net. In this way players don’t lose anything but will have a minor inconvenience to get the mod working properly again.
It Wasn’t That Easy
After all this work, I went into the game, shift clicked a recipe. Only one butterfly scroll was crafted, leaving an empty butterfly net behind. All as designed. Except, the butterfly scroll was empty. What was going on?
For this I had to break out the debugger. I broke into the code as the item was being crafted to see what was going on. As suspected, the NBT data wasn’t being passed to the scroll properly. But why?
After a serious debugging session and going through the crafting code I finally figured out what was happening. When you craft an item, an ItemCraftedEvent is sent out. You can detect this event in the mod and respond to it. I use this event to add NBT data to the crafted item.
This works if the player left clicks, since the item sent in the event is the item that is crafted. However, if the player shift-clicks, a copy of the item that has been crafted. Thus, any changes you make do not affect the result of the crafting.
I discovered that the crafted item was already added to the player’s inventory in this case, so I created a rather hacky solution to fix this.
// Check for shift-clicked item.
Player player = event.getEntity();
if (player != null) {
Inventory inventory = player.getInventory();
for (int j = 0; j < inventory.getContainerSize(); ++j) {
ItemStack inventoryItem = inventory.getItem(j);
if (inventoryItem.getItem() instanceof BottledButterflyItem ||
inventoryItem.getItem() instanceof ButterflyScrollItem) {
CompoundTag inventoryItemTag = inventoryItem.getTag();
if (inventoryItemTag == null || !inventoryItemTag.contains(CompoundTagId.ENTITY_ID)) {
ButterflyContainerItem.setButterfly(inventoryItem, index);
}
}
}
}
break;
}This finds the item without the NBT data and adds it. This works for 99% of cases, but this is still exploitable in game, and will lead to some slightly iffy behaviour in corner cases.
So Why ItemCraftedEvent?
Some of you may assume that this is a bug in Forge’s crafting hooks. It isn’t. The real problem is that I was using the event for something it wasn’t intended for. ItemCraftedEvent isn’t supposed to be used in order to change the item being crafted. It’s just a notification that an item has been crafted.
You’re supposed to use this event to create a response to an item being crafted, such as unlocking a recipe or an achievement. You’re not supposed to use this event in any way like I have in this mod.
So, in order to fix this properly, I need to rewrite things so that none of these items use NBT data. There should be one of each item for each butterfly, and the recipes should require specific combinations. I’m postponing this fix until after I look at data driving the butterflies, since I think this will make implementing the true fix a whole lot easier.
