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

projects/common-api/src/main/java/dan200/computercraft/api/detail/DetailRegistry.java

@@ -32,6 +32,8 @@ public interface DetailRegistry<T> {
     /**
      * 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.
+     * <p>
+     * This method <em>MAY</em> 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<T> {
 
     /**
      * Compute all details about an object, using {@link #getBasicDetails(Object)} and any registered providers.
+     * <p>
+     * This method is <em>NOT</em> thread safe. It should only be called from the computer thread.
      *
      * @param object The object to get details for.
      * @return The computed details.

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.
+     * <p>
+     * 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<ItemStack> ITEM_STACK = ComputerCraftAPIService.get().getItemStackDetailRegistry();
 
     /**
      * Provides details for {@link BlockReference}, a reference to a {@link Block} in the world.
+     * <p>
+     * This instance's {@link DetailRegistry#getBasicDetails(Object)} is thread safe and may be called from the computer
+     * thread.
      */
     public static final DetailRegistry<BlockReference> BLOCK_IN_WORLD = ComputerCraftAPIService.get().getBlockInWorldDetailRegistry();
-
 }

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.

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 extends Map<? super String, Object>> T fillBasicSafe(T data, ItemStack stack) {
+    public static void fillBasic(Map<? super String, Object> data, ItemStack stack) {
         data.put("name", DetailHelpers.getId(RegistryWrappers.ITEMS, stack.getItem()));
         data.put("count", stack.getCount());
-        return data;
-    }
-
-    public static void fillBasic(Map<? super String, Object> data, ItemStack stack) {
-        fillBasicSafe(data, stack);
         var hash = NBTUtil.getNBTHash(stack.getTag());
         if (hash != null) data.put("nbt", hash);
     }

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<Integer> slot, Optional<Boolean> 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));
+        }
     }
 
 

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<ItemStack> inventory = NonNullList.withSize(INVENTORY_SIZE, ItemStack.EMPTY);
-    private final NonNullList<ItemStack> previousInventory = NonNullList.withSize(INVENTORY_SIZE, ItemStack.EMPTY);
+    private final NonNullList<ItemStack> 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);

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}.
+ * <p>
+ * 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);
+}

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;