From 1554c7b397c229ab5c288d4dea0185d938b0064b Mon Sep 17 00:00:00 2001 From: Jonathan Coates Date: Wed, 25 Jan 2023 18:37:14 +0000 Subject: [PATCH] Always expose nbt from turtle.getItemDetail - Document the thread safety of DetailRegistry a little better. - Turtles now duplicate their inventory to the "previous inventory" (now called inventorySnapshot) immediately, rather than when the block is ticked. This is slightly more resource intensive, but I don't think it's so bad we need to worry. - As this snapshot is now always up-to-date, we can read it from the computer thread. Given the item is immutable, it's safe to read NBT from it. _Technically_ this is not safe under the Java memory model, but in practice I don't think we'll observe the wrong value. Closes #1306 --- .../api/detail/DetailRegistry.java | 4 +++ .../api/detail/VanillaDetailRegistries.java | 7 +++- .../api/turtle/ITurtleAccess.java | 2 ++ .../shared/details/ItemDetails.java | 7 +--- .../shared/turtle/apis/TurtleAPI.java | 30 ++++++---------- .../turtle/blocks/TurtleBlockEntity.java | 34 +++++++++---------- .../turtle/core/TurtleAccessInternal.java | 27 +++++++++++++++ .../shared/turtle/core/TurtleBrain.java | 15 ++++++-- 8 files changed, 80 insertions(+), 46 deletions(-) create mode 100644 projects/common/src/main/java/dan200/computercraft/shared/turtle/core/TurtleAccessInternal.java diff --git a/projects/common-api/src/main/java/dan200/computercraft/api/detail/DetailRegistry.java b/projects/common-api/src/main/java/dan200/computercraft/api/detail/DetailRegistry.java index 2cd1cc041..e765a1f74 100644 --- a/projects/common-api/src/main/java/dan200/computercraft/api/detail/DetailRegistry.java +++ b/projects/common-api/src/main/java/dan200/computercraft/api/detail/DetailRegistry.java @@ -32,6 +32,8 @@ public interface DetailRegistry { /** * Compute basic details about an object. This is cheaper than computing all details operation, and so is suitable * for when you need to compute the details for a large number of values. + *

+ * This method MAY be thread safe: consult the instance's documentation for details. * * @param object The object to get details for. * @return The basic details. @@ -40,6 +42,8 @@ public interface DetailRegistry { /** * Compute all details about an object, using {@link #getBasicDetails(Object)} and any registered providers. + *

+ * This method is NOT thread safe. It should only be called from the computer thread. * * @param object The object to get details for. * @return The computed details. diff --git a/projects/common-api/src/main/java/dan200/computercraft/api/detail/VanillaDetailRegistries.java b/projects/common-api/src/main/java/dan200/computercraft/api/detail/VanillaDetailRegistries.java index 3e71b70b1..f1af2e593 100644 --- a/projects/common-api/src/main/java/dan200/computercraft/api/detail/VanillaDetailRegistries.java +++ b/projects/common-api/src/main/java/dan200/computercraft/api/detail/VanillaDetailRegistries.java @@ -15,12 +15,17 @@ import net.minecraft.world.level.block.Block; public class VanillaDetailRegistries { /** * Provides details for {@link ItemStack}s. + *

+ * This instance's {@link DetailRegistry#getBasicDetails(Object)} is thread safe (assuming the stack is immutable) + * and may be called from the computer thread. */ public static final DetailRegistry ITEM_STACK = ComputerCraftAPIService.get().getItemStackDetailRegistry(); /** * Provides details for {@link BlockReference}, a reference to a {@link Block} in the world. + *

+ * This instance's {@link DetailRegistry#getBasicDetails(Object)} is thread safe and may be called from the computer + * thread. */ public static final DetailRegistry BLOCK_IN_WORLD = ComputerCraftAPIService.get().getBlockInWorldDetailRegistry(); - } diff --git a/projects/common-api/src/main/java/dan200/computercraft/api/turtle/ITurtleAccess.java b/projects/common-api/src/main/java/dan200/computercraft/api/turtle/ITurtleAccess.java index 9921ebfe1..9f38cfd73 100644 --- a/projects/common-api/src/main/java/dan200/computercraft/api/turtle/ITurtleAccess.java +++ b/projects/common-api/src/main/java/dan200/computercraft/api/turtle/ITurtleAccess.java @@ -15,6 +15,7 @@ import net.minecraft.nbt.CompoundTag; import net.minecraft.world.Container; import net.minecraft.world.level.Level; import net.minecraft.world.phys.Vec3; +import org.jetbrains.annotations.ApiStatus; import javax.annotation.Nullable; @@ -24,6 +25,7 @@ import javax.annotation.Nullable; * This should not be implemented by your classes. Do not interact with turtles except via this interface and * {@link ITurtleUpgrade}. */ +@ApiStatus.NonExtendable public interface ITurtleAccess { /** * Returns the world in which the turtle resides. diff --git a/projects/common/src/main/java/dan200/computercraft/shared/details/ItemDetails.java b/projects/common/src/main/java/dan200/computercraft/shared/details/ItemDetails.java index 4096e50d9..f225be682 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/details/ItemDetails.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/details/ItemDetails.java @@ -25,14 +25,9 @@ import java.util.*; * Data providers for items. */ public class ItemDetails { - public static > T fillBasicSafe(T data, ItemStack stack) { + public static void fillBasic(Map data, ItemStack stack) { data.put("name", DetailHelpers.getId(RegistryWrappers.ITEMS, stack.getItem())); data.put("count", stack.getCount()); - return data; - } - - public static void fillBasic(Map data, ItemStack stack) { - fillBasicSafe(data, stack); var hash = NBTUtil.getNBTHash(stack.getTag()); if (hash != null) data.put("nbt", hash); } diff --git a/projects/common/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java b/projects/common/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java index fc5dfeaaf..3c9cacde4 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java @@ -7,16 +7,13 @@ package dan200.computercraft.shared.turtle.apis; import dan200.computercraft.api.detail.VanillaDetailRegistries; import dan200.computercraft.api.lua.*; -import dan200.computercraft.api.turtle.ITurtleAccess; import dan200.computercraft.api.turtle.TurtleCommand; import dan200.computercraft.api.turtle.TurtleCommandResult; import dan200.computercraft.api.turtle.TurtleSide; import dan200.computercraft.core.apis.IAPIEnvironment; import dan200.computercraft.core.metrics.Metrics; -import dan200.computercraft.shared.details.ItemDetails; import dan200.computercraft.shared.turtle.core.*; -import java.util.HashMap; import java.util.Optional; /** @@ -70,9 +67,9 @@ import java.util.Optional; */ public class TurtleAPI implements ILuaAPI { private final IAPIEnvironment environment; - private final ITurtleAccess turtle; + private final TurtleAccessInternal turtle; - public TurtleAPI(IAPIEnvironment environment, ITurtleAccess turtle) { + public TurtleAPI(IAPIEnvironment environment, TurtleAccessInternal turtle) { this.environment = environment; this.turtle = turtle; } @@ -760,20 +757,15 @@ public class TurtleAPI implements ILuaAPI { @LuaFunction public final MethodResult getItemDetail(ILuaContext context, Optional slot, Optional detailed) throws LuaException { int actualSlot = checkSlot(slot).orElse(turtle.getSelectedSlot()); - return detailed.orElse(false) - ? context.executeMainThreadTask(() -> getItemDetail(actualSlot, true)) - : MethodResult.of(getItemDetail(actualSlot, false)); - } - - private Object[] getItemDetail(int slot, boolean detailed) { - var stack = turtle.getInventory().getItem(slot); - if (stack.isEmpty()) return new Object[]{ null }; - - var table = detailed - ? VanillaDetailRegistries.ITEM_STACK.getDetails(stack) - : ItemDetails.fillBasicSafe(new HashMap<>(), stack); - - return new Object[]{ table }; + if (detailed.orElse(false)) { + return context.executeMainThreadTask(() -> { + var stack = turtle.getInventory().getItem(actualSlot); + return new Object[]{ stack.isEmpty() ? null : VanillaDetailRegistries.ITEM_STACK.getDetails(stack) }; + }); + } else { + var stack = turtle.getItemSnapshot(actualSlot); + return MethodResult.of(stack.isEmpty() ? null : VanillaDetailRegistries.ITEM_STACK.getBasicDetails(stack)); + } } diff --git a/projects/common/src/main/java/dan200/computercraft/shared/turtle/blocks/TurtleBlockEntity.java b/projects/common/src/main/java/dan200/computercraft/shared/turtle/blocks/TurtleBlockEntity.java index 9df677442..5c46cce36 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/turtle/blocks/TurtleBlockEntity.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/turtle/blocks/TurtleBlockEntity.java @@ -56,7 +56,7 @@ public class TurtleBlockEntity extends AbstractComputerBlockEntity implements Ba } private final NonNullList inventory = NonNullList.withSize(INVENTORY_SIZE, ItemStack.EMPTY); - private final NonNullList previousInventory = NonNullList.withSize(INVENTORY_SIZE, ItemStack.EMPTY); + private final NonNullList inventorySnapshot = NonNullList.withSize(INVENTORY_SIZE, ItemStack.EMPTY); private boolean inventoryChanged = false; private TurtleBrain brain = new TurtleBrain(this); private MoveState moveState = MoveState.NOT_MOVED; @@ -78,7 +78,7 @@ public class TurtleBlockEntity extends AbstractComputerBlockEntity implements Ba getFamily(), Config.turtleTermWidth, Config.turtleTermHeight ); - computer.addAPI(new TurtleAPI(computer.getAPIEnvironment(), getAccess())); + computer.addAPI(new TurtleAPI(computer.getAPIEnvironment(), brain)); brain.setupComputer(computer); return computer; } @@ -141,11 +141,7 @@ public class TurtleBlockEntity extends AbstractComputerBlockEntity implements Ba if (inventoryChanged) { var computer = getServerComputer(); if (computer != null) computer.queueEvent("turtle_inventory"); - inventoryChanged = false; - for (var n = 0; n < getContainerSize(); n++) { - previousInventory.set(n, getItem(n).copy()); - } } } @@ -178,13 +174,13 @@ public class TurtleBlockEntity extends AbstractComputerBlockEntity implements Ba // Read inventory var nbttaglist = nbt.getList("Items", Tag.TAG_COMPOUND); inventory.clear(); - previousInventory.clear(); + inventorySnapshot.clear(); for (var i = 0; i < nbttaglist.size(); i++) { var tag = nbttaglist.getCompound(i); var slot = tag.getByte("Slot") & 0xff; if (slot < getContainerSize()) { inventory.set(slot, ItemStack.of(tag)); - previousInventory.set(slot, inventory.get(slot).copy()); + inventorySnapshot.set(slot, inventory.get(slot).copy()); } } @@ -273,7 +269,7 @@ public class TurtleBlockEntity extends AbstractComputerBlockEntity implements Ba void setOwningPlayer(GameProfile player) { brain.setOwningPlayer(player); - setChanged(); + onTileEntityChange(); } // IInventory @@ -283,16 +279,20 @@ public class TurtleBlockEntity extends AbstractComputerBlockEntity implements Ba return inventory; } + public ItemStack getItemSnapshot(int slot) { + return slot >= 0 && slot < inventorySnapshot.size() ? inventorySnapshot.get(slot) : ItemStack.EMPTY; + } + @Override public void setChanged() { super.setChanged(); - if (!inventoryChanged) { - for (var n = 0; n < getContainerSize(); n++) { - if (!ItemStack.matches(getItem(n), previousInventory.get(n))) { - inventoryChanged = true; - break; - } - } + + for (var slot = 0; slot < getContainerSize(); slot++) { + var item = getItem(slot); + if (ItemStack.matches(item, inventorySnapshot.get(slot))) continue; + + inventoryChanged = true; + inventorySnapshot.set(slot, item.copy()); } } @@ -340,7 +340,7 @@ public class TurtleBlockEntity extends AbstractComputerBlockEntity implements Ba public void transferStateFrom(TurtleBlockEntity copy) { super.transferStateFrom(copy); Collections.copy(inventory, copy.inventory); - Collections.copy(previousInventory, copy.previousInventory); + Collections.copy(inventorySnapshot, copy.inventorySnapshot); inventoryChanged = copy.inventoryChanged; brain = copy.brain; brain.setOwner(this); diff --git a/projects/common/src/main/java/dan200/computercraft/shared/turtle/core/TurtleAccessInternal.java b/projects/common/src/main/java/dan200/computercraft/shared/turtle/core/TurtleAccessInternal.java new file mode 100644 index 000000000..dc8314caf --- /dev/null +++ b/projects/common/src/main/java/dan200/computercraft/shared/turtle/core/TurtleAccessInternal.java @@ -0,0 +1,27 @@ +/* + * This file is part of ComputerCraft - http://www.computercraft.info + * Copyright Daniel Ratcliffe, 2011-2022. Do not distribute without permission. + * Send enquiries to dratcliffe@gmail.com + */ +package dan200.computercraft.shared.turtle.core; + +import dan200.computercraft.api.turtle.ITurtleAccess; +import net.minecraft.world.item.ItemStack; + +/** + * An internal version of {@link ITurtleAccess}. + *

+ * This exposes additional functionality we don't want in the public API, but where we don't want access to the full + * {@link TurtleBrain} interface. + */ +public interface TurtleAccessInternal extends ITurtleAccess { + /** + * Get an immutable snapshot of an item in the inventory. This is a thread-safe version of + * {@code getInventory().getItem()}. + * + * @param slot The slot + * @return The current item. This should NOT be modified. + * @see net.minecraft.world.Container#getItem(int) + */ + ItemStack getItemSnapshot(int slot); +} diff --git a/projects/common/src/main/java/dan200/computercraft/shared/turtle/core/TurtleBrain.java b/projects/common/src/main/java/dan200/computercraft/shared/turtle/core/TurtleBrain.java index b0d251b16..45eed9661 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/turtle/core/TurtleBrain.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/turtle/core/TurtleBrain.java @@ -10,7 +10,10 @@ import com.mojang.authlib.GameProfile; import dan200.computercraft.api.lua.ILuaCallback; import dan200.computercraft.api.lua.MethodResult; import dan200.computercraft.api.peripheral.IPeripheral; -import dan200.computercraft.api.turtle.*; +import dan200.computercraft.api.turtle.ITurtleUpgrade; +import dan200.computercraft.api.turtle.TurtleAnimation; +import dan200.computercraft.api.turtle.TurtleCommand; +import dan200.computercraft.api.turtle.TurtleSide; import dan200.computercraft.core.computer.ComputerSide; import dan200.computercraft.core.util.Colour; import dan200.computercraft.impl.TurtleUpgrades; @@ -34,6 +37,7 @@ import net.minecraft.world.Container; import net.minecraft.world.entity.Entity; import net.minecraft.world.entity.MoverType; import net.minecraft.world.item.DyeColor; +import net.minecraft.world.item.ItemStack; import net.minecraft.world.level.Level; import net.minecraft.world.level.material.PushReaction; import net.minecraft.world.phys.AABB; @@ -47,7 +51,7 @@ import java.util.function.Predicate; import static dan200.computercraft.shared.common.IColouredItem.NBT_COLOUR; import static dan200.computercraft.shared.util.WaterloggableHelpers.WATERLOGGED; -public class TurtleBrain implements ITurtleAccess { +public class TurtleBrain implements TurtleAccessInternal { public static final String NBT_RIGHT_UPGRADE = "RightUpgrade"; public static final String NBT_RIGHT_UPGRADE_DATA = "RightUpgradeNbt"; public static final String NBT_LEFT_UPGRADE = "LeftUpgrade"; @@ -456,7 +460,7 @@ public class TurtleBrain implements ITurtleAccess { return overlay; } - public void setOverlay(ResourceLocation overlay) { + public void setOverlay(@Nullable ResourceLocation overlay) { if (!Objects.equal(this.overlay, overlay)) { this.overlay = overlay; BlockEntityHelpers.updateBlock(owner); @@ -768,6 +772,11 @@ public class TurtleBrain implements ITurtleAccess { return previous + (next - previous) * f; } + @Override + public ItemStack getItemSnapshot(int slot) { + return owner.getItemSnapshot(slot); + } + private static final class CommandCallback implements ILuaCallback { final MethodResult pull = MethodResult.pullEvent("turtle_response", this); private final int command;