From 75e2845c01532987026ed028e97bab74e274a9dd Mon Sep 17 00:00:00 2001 From: SquidDev Date: Thu, 1 Aug 2019 14:09:57 +0100 Subject: [PATCH] Remove synchronized from turtle inventory code These should never be called off the server thread, so this doesn't make much difference. --- .../api/turtle/ITurtleAccess.java | 6 +- .../shared/turtle/FurnaceRefuelHandler.java | 2 +- .../shared/turtle/apis/TurtleAPI.java | 6 +- .../shared/turtle/blocks/TileTurtle.java | 142 ++++++------------ 4 files changed, 59 insertions(+), 97 deletions(-) diff --git a/src/main/java/dan200/computercraft/api/turtle/ITurtleAccess.java b/src/main/java/dan200/computercraft/api/turtle/ITurtleAccess.java index 2c74d2543..b693de3a7 100644 --- a/src/main/java/dan200/computercraft/api/turtle/ITurtleAccess.java +++ b/src/main/java/dan200/computercraft/api/turtle/ITurtleAccess.java @@ -145,7 +145,9 @@ public interface ITurtleAccess GameProfile getOwningPlayer(); /** - * Get the inventory of this turtle + * Get the inventory of this turtle. + * + * Note: this inventory should only be accessed and modified on the server thread. * * @return This turtle's inventory * @see #getItemHandler() @@ -156,6 +158,8 @@ public interface ITurtleAccess /** * Get the inventory of this turtle as an {@link IItemHandlerModifiable}. * + * Note: this inventory should only be accessed and modified on the server thread. + * * @return This turtle's inventory * @see #getInventory() * @see IItemHandlerModifiable diff --git a/src/main/java/dan200/computercraft/shared/turtle/FurnaceRefuelHandler.java b/src/main/java/dan200/computercraft/shared/turtle/FurnaceRefuelHandler.java index d842b8615..feccb9543 100644 --- a/src/main/java/dan200/computercraft/shared/turtle/FurnaceRefuelHandler.java +++ b/src/main/java/dan200/computercraft/shared/turtle/FurnaceRefuelHandler.java @@ -33,7 +33,7 @@ public int refuel( @Nonnull ITurtleAccess turtle, @Nonnull ItemStack currentStac int fuelSpaceLeft = turtle.getFuelLimit() - turtle.getFuelLevel(); int fuelPerItem = getFuelPerItem( turtle.getItemHandler().getStackInSlot( slot ) ); int fuelItemLimit = (int) Math.ceil( fuelSpaceLeft / (double) fuelPerItem ); - if ( limit > fuelItemLimit ) limit = fuelItemLimit; + if( limit > fuelItemLimit ) limit = fuelItemLimit; ItemStack stack = turtle.getItemHandler().extractItem( slot, limit, false ); int fuelToGive = fuelPerItem * stack.getCount(); diff --git a/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java b/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java index 475400aaa..573400499 100644 --- a/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java +++ b/src/main/java/dan200/computercraft/shared/turtle/apis/TurtleAPI.java @@ -334,9 +334,11 @@ public Object[] callMethod( @Nonnull ILuaContext context, int method, @Nonnull O return tryCommand( context, new TurtleInspectCommand( InteractDirection.Up ) ); case 40: // inspectDown return tryCommand( context, new TurtleInspectCommand( InteractDirection.Down ) ); - case 41: + case 41: // getItemDetail { - // getItemDetail + // FIXME: There's a race condition here if the stack is being modified (mutating NBT, etc...) + // on another thread. The obvious solution is to move this into a command, but some programs rely + // on this having a 0-tick delay. int slot = parseOptionalSlotNumber( args, 0, m_turtle.getSelectedSlot() ); ItemStack stack = m_turtle.getInventory().getStackInSlot( slot ); if( stack.isEmpty() ) return new Object[] { null }; diff --git a/src/main/java/dan200/computercraft/shared/turtle/blocks/TileTurtle.java b/src/main/java/dan200/computercraft/shared/turtle/blocks/TileTurtle.java index 353ce8d5d..e70c7d333 100644 --- a/src/main/java/dan200/computercraft/shared/turtle/blocks/TileTurtle.java +++ b/src/main/java/dan200/computercraft/shared/turtle/blocks/TileTurtle.java @@ -43,19 +43,16 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; +import java.util.Collections; import static net.minecraftforge.items.CapabilityItemHandler.ITEM_HANDLER_CAPABILITY; public class TileTurtle extends TileComputerBase implements ITurtleTile, DefaultInventory { - // Statics - public static final int INVENTORY_SIZE = 16; public static final int INVENTORY_WIDTH = 4; public static final int INVENTORY_HEIGHT = 4; - // Members - enum MoveState { NOT_MOVED, @@ -63,12 +60,12 @@ enum MoveState MOVED } - private NonNullList m_inventory; - private NonNullList m_previousInventory; + private final NonNullList m_inventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY ); + private final NonNullList m_previousInventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY ); private final IItemHandlerModifiable m_itemHandler = new InvWrapper( this ); - private boolean m_inventoryChanged; - private TurtleBrain m_brain; - private MoveState m_moveState; + private boolean m_inventoryChanged = false; + private TurtleBrain m_brain = new TurtleBrain( this ); + private MoveState m_moveState = MoveState.NOT_MOVED; private ComputerFamily m_family; public TileTurtle() @@ -78,15 +75,10 @@ public TileTurtle() public TileTurtle( ComputerFamily family ) { - m_inventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY ); - m_previousInventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY ); - m_inventoryChanged = false; - m_brain = new TurtleBrain( this ); - m_moveState = MoveState.NOT_MOVED; m_family = family; } - public boolean hasMoved() + private boolean hasMoved() { return m_moveState == MoveState.MOVED; } @@ -224,18 +216,15 @@ public void update() { super.update(); m_brain.update(); - synchronized( m_inventory ) + if( !getWorld().isRemote && m_inventoryChanged ) { - if( !getWorld().isRemote && m_inventoryChanged ) - { - ServerComputer computer = getServerComputer(); - if( computer != null ) computer.queueEvent( "turtle_inventory" ); + ServerComputer computer = getServerComputer(); + if( computer != null ) computer.queueEvent( "turtle_inventory" ); - m_inventoryChanged = false; - for( int n = 0; n < getSizeInventory(); n++ ) - { - m_previousInventory.set( n, InventoryUtil.copyItem( getStackInSlot( n ) ) ); - } + m_inventoryChanged = false; + for( int n = 0; n < getSizeInventory(); n++ ) + { + m_previousInventory.set( n, InventoryUtil.copyItem( getStackInSlot( n ) ) ); } } } @@ -270,8 +259,8 @@ public void readFromNBT( NBTTagCompound nbt ) // Read inventory NBTTagList nbttaglist = nbt.getTagList( "Items", Constants.NBT.TAG_COMPOUND ); - m_inventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY ); - m_previousInventory = NonNullList.withSize( INVENTORY_SIZE, ItemStack.EMPTY ); + m_inventory.clear(); + m_previousInventory.clear(); for( int i = 0; i < nbttaglist.tagCount(); i++ ) { NBTTagCompound tag = nbttaglist.getCompoundTagAt( i ); @@ -382,7 +371,7 @@ public ComputerFamily getFamily() return m_family; } - public void setOwningPlayer( GameProfile player ) + void setOwningPlayer( GameProfile player ) { m_brain.setOwningPlayer( player ); markDirty(); @@ -410,109 +399,76 @@ public boolean isEmpty() @Override public ItemStack getStackInSlot( int slot ) { - if( slot >= 0 && slot < INVENTORY_SIZE ) - { - synchronized( m_inventory ) - { - return m_inventory.get( slot ); - } - } - return ItemStack.EMPTY; + return slot >= 0 && slot < INVENTORY_SIZE ? m_inventory.get( slot ) : ItemStack.EMPTY; } @Nonnull @Override public ItemStack removeStackFromSlot( int slot ) { - synchronized( m_inventory ) - { - ItemStack result = getStackInSlot( slot ); - setInventorySlotContents( slot, ItemStack.EMPTY ); - return result; - } + ItemStack result = getStackInSlot( slot ); + setInventorySlotContents( slot, ItemStack.EMPTY ); + return result; } @Nonnull @Override public ItemStack decrStackSize( int slot, int count ) { - if( count == 0 ) + if( count == 0 ) return ItemStack.EMPTY; + + ItemStack stack = getStackInSlot( slot ); + if( stack.isEmpty() ) return ItemStack.EMPTY; + + if( stack.getCount() <= count ) { - return ItemStack.EMPTY; + setInventorySlotContents( slot, ItemStack.EMPTY ); + return stack; } - synchronized( m_inventory ) - { - ItemStack stack = getStackInSlot( slot ); - if( stack.isEmpty() ) - { - return ItemStack.EMPTY; - } - - if( stack.getCount() <= count ) - { - setInventorySlotContents( slot, ItemStack.EMPTY ); - return stack; - } - - ItemStack part = stack.splitStack( count ); - onInventoryDefinitelyChanged(); - return part; - } + ItemStack part = stack.splitStack( count ); + onInventoryDefinitelyChanged(); + return part; } @Override public void setInventorySlotContents( int i, @Nonnull ItemStack stack ) { - if( i >= 0 && i < INVENTORY_SIZE ) + if( i >= 0 && i < INVENTORY_SIZE && !InventoryUtil.areItemsEqual( stack, m_inventory.get( i ) ) ) { - synchronized( m_inventory ) - { - if( !InventoryUtil.areItemsEqual( stack, m_inventory.get( i ) ) ) - { - m_inventory.set( i, stack ); - onInventoryDefinitelyChanged(); - } - } + m_inventory.set( i, stack ); + onInventoryDefinitelyChanged(); } } @Override public void clear() { - synchronized( m_inventory ) + boolean changed = false; + for( int i = 0; i < INVENTORY_SIZE; i++ ) { - boolean changed = false; - for( int i = 0; i < INVENTORY_SIZE; i++ ) + if( !m_inventory.get( i ).isEmpty() ) { - if( !m_inventory.get( i ).isEmpty() ) - { - m_inventory.set( i, ItemStack.EMPTY ); - changed = true; - } - } - if( changed ) - { - onInventoryDefinitelyChanged(); + m_inventory.set( i, ItemStack.EMPTY ); + changed = true; } } + + if( changed ) onInventoryDefinitelyChanged(); } @Override public void markDirty() { super.markDirty(); - synchronized( m_inventory ) + if( !m_inventoryChanged ) { - if( !m_inventoryChanged ) + for( int n = 0; n < getSizeInventory(); n++ ) { - for( int n = 0; n < getSizeInventory(); n++ ) + if( !ItemStack.areItemStacksEqual( getStackInSlot( n ), m_previousInventory.get( n ) ) ) { - if( !ItemStack.areItemStacksEqual( getStackInSlot( n ), m_previousInventory.get( n ) ) ) - { - m_inventoryChanged = true; - break; - } + m_inventoryChanged = true; + break; } } } @@ -574,8 +530,8 @@ private boolean hasPeripheralUpgradeOnSide( ComputerSide side ) public void transferStateFrom( TileTurtle copy ) { super.transferStateFrom( copy ); - m_inventory = copy.m_inventory; - m_previousInventory = copy.m_previousInventory; + Collections.copy( m_inventory, copy.m_inventory ); + Collections.copy( m_previousInventory, copy.m_previousInventory ); m_inventoryChanged = copy.m_inventoryChanged; m_brain = copy.m_brain; m_brain.setOwner( this );