[1.12.2] [SOLVED] addItemStackToInventory Doesn't Work In Blocks?

[KittenKoder]

For adding an item to the players inventory, if I setItemHeld it works fine, but I would prefer addItemStackToInventory to it will stack. But it seems to prematurely exit the method that uses it for some reason. Here's the method:

 

@Override
	public boolean onBlockActivated(World worldIn, BlockPos pos, IBlockState state, EntityPlayer playerIn, EnumHand hand, EnumFacing facing, float hitX, float hitY, float hitZ)
    {
        if (worldIn.isRemote) return false;

        TileEntity tile = worldIn.getTileEntity(pos);
        if(tile instanceof IMagitechTileInventory) {
        	IMagitechTileInventory ped = (IMagitechTileInventory)tile;
	        if(EnumHand.MAIN_HAND == hand) {
	        	ItemStack item = playerIn.getHeldItemMainhand();
	        	ItemStack inped = ped.getStackInSlot(0);
	        	if(item == ItemStack.EMPTY || item == null) {
	        		if(inped != null && inped != ItemStack.EMPTY) {
	        	        worldIn.playSound((EntityPlayer)null, pos, SoundEvents.BLOCK_ENCHANTMENT_TABLE_USE, SoundCategory.BLOCKS, 0.3F, 0.6F);
//	        	        if(playerIn.inventory.addItemStackToInventory(inped))
	        	        playerIn.setHeldItem(EnumHand.MAIN_HAND, inped);
	        			ped.setInventorySlotContents(0, ItemStack.EMPTY);
	        			tile.markDirty();
	        		}
	        	} else if(inped == ItemStack.EMPTY) {
        			if(item.getCount() > 1) {
        		        worldIn.playSound((EntityPlayer)null, pos, SoundEvents.BLOCK_ENCHANTMENT_TABLE_USE, SoundCategory.BLOCKS, 0.3F, 0.6F);
        				item.shrink(1);
        				ItemStack t = new ItemStack(item.getItem(), 1);
        				ped.setInventorySlotContents(0, t);
        				tile.markDirty();
        			} else {
        		        worldIn.playSound((EntityPlayer)null, pos, SoundEvents.BLOCK_ENCHANTMENT_TABLE_USE, SoundCategory.BLOCKS, 0.3F, 0.6F);
	        			playerIn.setHeldItem(EnumHand.MAIN_HAND, ItemStack.EMPTY);
	        			ped.setInventorySlotContents(0, item);
	        			tile.markDirty();
        			}
	        	}
	        }
        }
        return true;
    }

 

Is there a convention that should be used which I am unaware of, or is this the only way to do what I am trying to accomplish?

Edited January 15, 2018 by KittenKoder

[KittenKoder]

27 minutes ago, diesieben07 said:

• Don't use IInventory use IItemHandler instead.
• Don't directly check for ItemStack.EMPTY, use ItemStack::isEmpty. Checking if an ItemStack is null is pointless.

• This is not the way to copy an ItemStack. Use ItemStack::copy.

• Special-casing "item has stack size 1" and "item has stack size > 1" is not needed. Just decrease the stack size by one and then check if the stack is empty.

• addItemStackToInventory should work fine, please clarify what you mean by "prematurely exit".

Wrong. If I don't check for null I get nullpointer errors from vanilla all the time, even in this instance when checking player inventory I get null pointer sometimes. The rest of what you said is meaningless, I didn't do most of what you said, and in other cases it only works the way I did it. As I said, the problem is in addItemStackToInventory. I found that it is somehow preventing the ItemHandler from the pedestal from synching. Since this is handled by the net.minecraftforge.items.ItemStackHandler I didn't realize that was what was happening. The problem is that without changing vanilla code, it won't synchronize.

 

If I have to write another ItemHandler to make it work that way, then I will just keep it this way. Writing a new class to do something that everyone uses increases the bloat of the game and would explain why so many mods are not being updated or getting too cumbersome for multiplayer.

[KittenKoder]

 

22 minutes ago, diesieben07 said:

Show code and stack traces. This should not be happening.

 

No, it is not. If you don't want help, go fuck off.

 

You are mixing up so many things... Nothing in your code (or in vanilla code) uses ItemStackHandler. You are using IInventory for your inventory (as evidenced by the call to setInventorySlotContents). And vanilla uses IInventory anyways. So... what are you talking about?

I showed you the code, and there is nothing in the stack traces, and I mean nothing.

 

The player inventory is set by whatever sets it, I don't change those classes at all. It's the player inventory add method that causes problems, not mine. If I cannot use the vanilla classes with the Forge classes then I will choose the vanilla classes so I don't have to override everything.

[KittenKoder]

Oh, and the setInventorySlot method for my entity is my own method, the entity is not the inventory itself, that's a TileEntity that has my own custom interfaces.

[KittenKoder]

I get null pointer exceptions when I don't check what vanilla code returns, that's why there are checks there. The player inventory method for add item is what is causing the problem! Do you not follow that? It's only when I call addItemStackToInventory on the PLAYER inventory that everything messes up. I stated that from the beginning, look at what is commented out, that's the method that causes the problems, not the ones I added or customized.

[KittenKoder]

Here:

package com.kittenkoder.magitech.tile;

import net.minecraft.block.state.IBlockState;
import net.minecraft.item.ItemStack;
import net.minecraft.nbt.NBTTagCompound;
import net.minecraft.tileentity.TileEntity;
import net.minecraft.util.math.AxisAlignedBB;
import net.minecraft.util.math.BlockPos;
import net.minecraft.world.World;
import net.minecraftforge.fml.common.network.NetworkRegistry;
import net.minecraftforge.fml.relauncher.Side;
import net.minecraftforge.fml.relauncher.SideOnly;
import net.minecraftforge.items.ItemStackHandler;

import com.kittenkoder.magitech.Magitech;
import com.kittenkoder.magitech.network.MagitechPedestalRequestUpdate;
import com.kittenkoder.magitech.network.MagitechUpdatePedestalPacket;

public class MagitechTileEntityPedestal extends MagitechPartialTileEntity implements IMagitechTileInventory {
	public static final AxisAlignedBB BOUNDINGBOX = new AxisAlignedBB(0.0D, 0.0D, 0.0D, 1.0D, 0.25D, 1.0D);

	protected ItemStackHandler handler;
	
	public class MagitechPedestalStackHandler extends ItemStackHandler {
		protected MagitechTileEntityPedestal tile;
		
		public MagitechPedestalStackHandler(MagitechTileEntityPedestal ntile) {
			super(1);
			this.tile = ntile;
		}
		
		
		@Override
		protected void onContentsChanged(int slot) {
			if (!world.isRemote) {
				Magitech.net.sendToAllAround(new MagitechUpdatePedestalPacket(this.tile), new NetworkRegistry.TargetPoint(world.provider.getDimension(), pos.getX(), pos.getY(), pos.getZ(), 64));
			}
		}		
	}
	
	public MagitechTileEntityPedestal() {
		this.handler = new MagitechPedestalStackHandler(this);
	}
	
	@Override
	public void onLoad() {
		if (world.isRemote) {
			Magitech.net.sendToServer(new MagitechPedestalRequestUpdate(this));
		}
	}
	
	@SideOnly(Side.CLIENT)
    public net.minecraft.util.math.AxisAlignedBB getRenderBoundingBox()
    {
        net.minecraft.util.math.AxisAlignedBB bb = TileEntity.INFINITE_EXTENT_AABB;
        BlockPos pos = this.getPos();
        net.minecraft.util.math.AxisAlignedBB cbb = null;
        try
        {
            cbb = MagitechTileEntityPedestal.BOUNDINGBOX.offset(pos);
        }
        catch (Exception e)
        {
            cbb = new net.minecraft.util.math.AxisAlignedBB(getPos().add(-1, 0, -1), getPos().add(1, 1, 1));
        }
        if (cbb != null) bb = cbb;
        return bb;
    }

	@Override
	public NBTTagCompound writeToNBT(NBTTagCompound compound)
    {
        super.writeToNBT(compound);
        compound.setTag("Item", this.handler.serializeNBT());
        return compound;
    }
	
	@Override
	public boolean shouldRefresh(World world, BlockPos pos, IBlockState oldState, IBlockState newSate)
    {
        return (oldState.getBlock() != newSate.getBlock()) || oldState != newSate;
    }
	
	@Override
	public void readFromNBT(NBTTagCompound compound)
    {
        super.readFromNBT(compound);
        if(compound.hasKey("Item")) this.handler.deserializeNBT(compound.getCompoundTag("Item"));
    }

	public ItemStack getStackInSlot(int index) {
		ItemStack t = this.handler.getStackInSlot(index);
		return t;
	}

	public ItemStack removeStackFromSlot(int index) {
		ItemStack t = this.handler.getStackInSlot(index);
		this.handler.setStackInSlot(index, ItemStack.EMPTY);
		return t;
	}

	public void setInventorySlotContents(int index, ItemStack stack) {
		this.handler.setStackInSlot(index, stack);
	}
	
	@Override
	public void writeInventoryToNBT(NBTTagCompound tag) {
		tag.setTag("Item", this.handler.serializeNBT());
	}

	@Override
	public void readInventoryFromNBT(NBTTagCompound tag) {
		if(tag.hasKey("Item")) this.handler.deserializeNBT(tag.getCompoundTag("Item"));
	}
}

package com.kittenkoder.magitech.blocks;

import java.util.Random;

import javax.annotation.Nullable;

import net.minecraft.block.SoundType;
import net.minecraft.block.material.Material;
import net.minecraft.block.state.IBlockState;
import net.minecraft.entity.player.EntityPlayer;
import net.minecraft.init.SoundEvents;
import net.minecraft.inventory.InventoryHelper;
import net.minecraft.item.ItemStack;
import net.minecraft.tileentity.TileEntity;
import net.minecraft.util.EnumFacing;
import net.minecraft.util.EnumHand;
import net.minecraft.util.SoundCategory;
import net.minecraft.util.math.AxisAlignedBB;
import net.minecraft.util.math.BlockPos;
import net.minecraft.world.IBlockAccess;
import net.minecraft.world.World;
import net.minecraftforge.fml.relauncher.Side;
import net.minecraftforge.fml.relauncher.SideOnly;

import com.kittenkoder.magitech.tile.IMagitechTileInventory;
import com.kittenkoder.magitech.tile.MagitechTileEntityPedestal;

public class MagitechBlockPedestal extends MagitechBlockPartial {

	public MagitechBlockPedestal(String unlocalizedName, String name,
			Material material, float hardness, float resistance,
			SoundType sound, int opacity, float lightlevel, String harvest,
			int harvestlevel) {
		super(unlocalizedName, name, material, hardness, resistance, sound, opacity,
				lightlevel, harvest, harvestlevel);
	}

	@Override
	@Nullable
    public AxisAlignedBB getCollisionBoundingBox(IBlockState blockState, IBlockAccess worldIn, BlockPos pos)
    {
        return MagitechTileEntityPedestal.BOUNDINGBOX;
    }

	@SideOnly(Side.CLIENT)
    public AxisAlignedBB getSelectedBoundingBox(IBlockState state, World worldIn, BlockPos pos)
    {
        return MagitechTileEntityPedestal.BOUNDINGBOX.offset(pos);
    }

	@Override
	@Nullable
	public TileEntity createTileEntity(World worldIn, IBlockState state) {
		return new MagitechTileEntityPedestal();
	}
	
	@Override
	public void breakBlock(World world, BlockPos pos, IBlockState blockstate) {
		TileEntity tile = world.getTileEntity(pos);
		if(tile instanceof IMagitechTileInventory) {
			IMagitechTileInventory te = (IMagitechTileInventory) tile;
			InventoryHelper.spawnItemStack(world, pos.getX(), pos.getY(), pos.getZ(), te.getStackInSlot(0));
		}
	    super.breakBlock(world, pos, blockstate);
	}
	
	@SideOnly(Side.CLIENT)
    public void randomDisplayTick(IBlockState state, World world, BlockPos pos, Random rand)
    {
    }
	
	@Override
	public boolean onBlockActivated(World worldIn, BlockPos pos, IBlockState state, EntityPlayer playerIn, EnumHand hand, EnumFacing facing, float hitX, float hitY, float hitZ)
    {
        if (worldIn.isRemote) return false;

        TileEntity tile = worldIn.getTileEntity(pos);
        if(tile instanceof IMagitechTileInventory) {
        	IMagitechTileInventory ped = (IMagitechTileInventory)tile;
	        if(EnumHand.MAIN_HAND == hand) {
	        	ItemStack item = playerIn.getHeldItemMainhand();
	        	ItemStack inped = ped.getStackInSlot(0);
	        	if(item == ItemStack.EMPTY || item == null) {
	        		if(inped != null && inped != ItemStack.EMPTY) {
	        	        worldIn.playSound((EntityPlayer)null, pos, SoundEvents.BLOCK_ENCHANTMENT_TABLE_USE, SoundCategory.BLOCKS, 0.3F, 0.6F);
//	        	        if(playerIn.inventory.addItemStackToInventory(inped))
	        	        playerIn.setHeldItem(EnumHand.MAIN_HAND, inped);
	        			ped.setInventorySlotContents(0, ItemStack.EMPTY);
	        			tile.markDirty();
	        		}
	        	} else if(inped == ItemStack.EMPTY) {
        			if(item.getCount() > 1) {
        		        worldIn.playSound((EntityPlayer)null, pos, SoundEvents.BLOCK_ENCHANTMENT_TABLE_USE, SoundCategory.BLOCKS, 0.3F, 0.6F);
        				item.shrink(1);
        				ItemStack t = new ItemStack(item.getItem(), 1);
        				ped.setInventorySlotContents(0, t);
        				tile.markDirty();
        			} else {
        		        worldIn.playSound((EntityPlayer)null, pos, SoundEvents.BLOCK_ENCHANTMENT_TABLE_USE, SoundCategory.BLOCKS, 0.3F, 0.6F);
	        			playerIn.setHeldItem(EnumHand.MAIN_HAND, ItemStack.EMPTY);
	        			ped.setInventorySlotContents(0, item);
	        			tile.markDirty();
        			}
	        	}
	        }
        }
        return true;
    }
	
}

 

Please notice what is commented out before responding.

[KittenKoder]

1 minute ago, diesieben07 said:

"Sir, every time I try drinking my tea my eye starts hurting."

"Ok, let's examine how you drink your tea, maybe you are doing something wrong."

"No no, it is my eye, the tea does not matter."

(3 hours later)

"You are stabbing your eye with the spoon in your tea cup."

Nope, and this time I made sure to be clear as to what the problem was. Even if I remove the .inventory part of the player interaction, same result. The pedestal's inventory does not send an update to the client at all. Using setHeldItem they both update.

[KittenKoder]

Just now, diesieben07 said:

Why... oh why.

 

This is not how you update a tile entity client side.

 

You are re-using ItemStack instances incorrectly. You do not follow the contract of IItemHandler::getStackInSlot (read the javadocs).

That is not sent from the client. If I did send it from the client it would crash. I know because I learned that the hard way. You asked for the code, do you want the ENTIRE project? 

[KittenKoder]

Setting the inventory slot is necessary in many times, to prevent excessive RAM waste when you have different slots for different things. It works well, and will prevent memory waste (something that's REALLY important in Java). There is no reason to waste memory with a new object for every point in an object. In this matter the item is often completely replaced (destroying the current contents), as needed.

[KittenKoder]

1 minute ago, diesieben07 said:

I did not say anything about it being sent from the client.

 

No. I already told you the problem.

I don't understand what you stated. The javadocs for IItemHandler::getStackInSlot only state:

Quote

Returns the ItemStack in a given slot. The result's stack size may be greater than the itemstacks max size. If the result is null, then the slot is empty. If the result is not null but the stack size is zero, then it represents an empty slot that will only accept* a specific itemstack.

IMPORTANT: This ItemStack MUST NOT be modified. This method is not for altering an inventories contents. Any implementers who are able to detect modification through this method should throw an exception.

SERIOUSLY: DO NOT MODIFY THE RETURNED ITEMSTACK

I do not modify the itemstack return, and there is no other method that I can find with the same functionality. I need that functionality, if I can't get it then I'll stick with the vanilla methods.

[KittenKoder]

1 minute ago, diesieben07 said:

What are you talking about?

 

I don't think you know how Java works, or at least the GC. The GC hates long lived objects that eventually still die. The GC loves objects that live for a few milli- or even nanoseconds. If you want to learn about it, watch this: https://www.youtube.com/watch?v=we_enrM7TSY. If you don't want to learn about it, that's fine, too, but then don't run around claiming "this is fast" and "this is bad" and "this is REALLY important in Java", without knowing what the hell you are talking about.

 

And no, you cannot just ignore the contract of a method because you feel it is "bad for memory" (whatever that might mean). If a method says that you must not modify it's return value, THEN YOU MUST NOT MODIFY IT'S RETURN VALUE. No, even if you feel like that's inefficient and NO, not even if you sacrifice 25 raw pointers to the god of the managed heap. If you still do it, unexpected things will happen. Don't then run around saying methods are broken.

 

"Long lived objects that will eventually die" describes everything. But regardless, what you stated has nothing to do with what I stated. If an object is going to live long, reducing it's cost to the memory by consolidating them into vectors/containers/arrays is the best method, as an object uses up more memory than an element in an array or vector. A list like inventory should be consolidated if it persists, to have a different inventory container for every single stack that is going to be stored there is wasteful.

 

As for modifying it's return value, you mean the shrinking? If so then I tried making a new stack and that didn't help. I switched to the shrink method because I thought making a new stack was causing the problem. This portion of the code has been through almost every iteration I can think of (that's why a lot of these two classes looks so sloppy) just to get it functional.

[KittenKoder]

Anyhow, we have gone completely off track. The problem is here, no other actions have caused problems yet:

if(item == ItemStack.EMPTY || item == null) {
	        		if(inped != null && inped != ItemStack.EMPTY) {
	        	        worldIn.playSound((EntityPlayer)null, pos, SoundEvents.BLOCK_ENCHANTMENT_TABLE_USE, SoundCategory.BLOCKS, 0.3F, 0.6F);
//	        	        if(playerIn.addItemStackToInventory(inped))
	        	        playerIn.setHeldItem(EnumHand.MAIN_HAND, inped);
	        			ped.setInventorySlotContents(0, ItemStack.EMPTY);
	        			tile.markDirty();
	        		}
	        	} else if(inped == ItemStack.EMPTY) {

So while I will probably go back to recreating the item stack instead of the shrinking, that is a completely different event.

[KittenKoder]

1 minute ago, diesieben07 said:

 

 

No. I mean exactly what I said:

 

 

Are you saying that the addItemStackToInventory method eventually changes the ItemStack it's given?

 

Quote

I am really not sure what you are talking about here. You are throwing around words that have meaning, but in combination it does not make much sense.

This is because you responded to what you thought I said and not what I said about using a larger container versus a bunch of smaller containers when they are all going to have the same lifespan. RAM has limits, and mods that take 4 gb cause problems on most PCs. I would prefer to avoid making a mod that does that.

[KittenKoder]

Just now, diesieben07 said:

Yes. Again:

 

I think you are having the wrong perspective here. Using up 4 gigs of RAM is really hard.

Biomes O Plenty uses 3 gb during the loading process, and sticks around 2 gb on the server side after loading. Combining Biomes O Plenty with Twilight Forest in 1.12 is a nightmare of epic proportions. I tried it on a server, they both crashed the 6 gb server just by lagging out and consuming too much memory with only two people playing.

 

Anyhow, "look at what this does" did lead me to understanding you, but 

Quote

Why... oh why.

is why I don't like you. Thanks for pointing me in the right direction, I didn't realize that the devs broke their own contracts.

[KittenKoder]

Just now, diesieben07 said:

Nobody is breaking any contracts here...

They are modifying the stack they are receiving, and with no good reason for it. So to expect it to not be modified in one instance, but then modify it in another for no apparent reason is essentially breaking a convention set by the devs.

[KittenKoder]

1 minute ago, diesieben07 said:

I think the reason is quite good. The method "leaves behind" whatever of the stack it cannot store.

 

IItemHandler is an interface added by Forge. addItemStackToInventory is vanilla code. And even if, just because two methods are in the same code base does not mean that their method contracts must be magically compatible.

In most cases it "leaves behind" a copy of it. In this one instance it doesn't. They changes the convention in this one instance. This is why I gave up on Java coding, the conventions are no longer conventions as a whole. But having to deal with something that changes it's own conventions on a whim is .... annoying.

[KittenKoder]

1 minute ago, diesieben07 said:

This has nothing to do with Java in particular. In fact you will find that Java programmers in general have quite good discipline in keeping with conventions.

 

However Minecraft was written by an amateur programmer (Notch) and has had features tacked on and tacked on over and over again by many different people. Things are just "made to work" without much regard for code elegance. Minecraft code is not designed as a proper API. Don't expect it to act like one.

Minecraft is actually just one example of such. I hate dealing with amateur conventions and have avoiding modding anything because of it. However in this instance my ultimate goal is to find all the problems with Minecraft, logic and OpenGL wise, as the PC version is a mess. The Android/Win10 version, sadly, isn't much better. But with memory limitations of the Android version modding is pretty much never going to be a thing on it.

 

I also find many flaws in Forge's conventions, like the whole "capability" thing that is being used to replace the interfaces. Interfaces have stuck around so long for a reason.

[KittenKoder]

Just now, diesieben07 said:

"interfaces" and "capabilities" are orthogonal concepts. Capabilities use interfaces, too.

Also, capabilities are a form of ECS, which you will find is a commonly used pattern in game development, which also has "stuck around so long for a reason".

Yes, but you also have to ensure you provide example code for such things, where as with interfaces you can just provide the source code for the interface itself as the methods are pretty intuitive and every IDE picks up on what's missing. Also capabilities has an unchecked type casting, and that presents high risk of breaking if the modder using them doesn't test well enough (which happens too often in game development as a whole). Why break the Java convention of interfaces just because core engines find something useful if it does the same thing?

[KittenKoder]

Thanks Dies for your assistance, though I am often abrasive it was appreciated. Here is the final method that works how I want it to work, allowing items to be swapped out and inventory synchronized. I may tidy it up a bit more to reduce some of the excess. I still need to check for null from vanilla inventories, it may seem redundant to many but for some reason I still get nulls from it sometimes. It would explain why some mods break for me regularly.

 

	@Override
	public boolean onBlockActivated(World worldIn, BlockPos pos, IBlockState state, EntityPlayer playerIn, EnumHand hand, EnumFacing facing, float hitX, float hitY, float hitZ)
    {
        if (worldIn.isRemote) return true;

        TileEntity tile = worldIn.getTileEntity(pos);
        if(tile instanceof IMagitechTileInventory) {
        	IMagitechTileInventory ped = (IMagitechTileInventory)tile;
	        if(EnumHand.MAIN_HAND == hand) {
	        	ItemStack item = playerIn.getHeldItemMainhand();
	        	ItemStack inped = ped.getStackInSlot(0);
	        	
	        	if((item == null || item.isEmpty()) && !inped.isEmpty()) {
	        		inped = inped.copy();
	        		
        	        if(playerIn.addItemStackToInventory(inped))
        	        	ped.removeStackFromSlot(0);
	        		
        	        worldIn.playSound((EntityPlayer)null, pos, SoundEvents.BLOCK_ENCHANTMENT_TABLE_USE, SoundCategory.BLOCKS, 0.3F, 0.6F);
        			tile.markDirty();
	        	} else if(item != null && !item.isEmpty()) {
	        		ItemStack pitem = item.copy();
	        		pitem.setCount(1);
	        		
    				item = item.copy();
    		        item.shrink(1);
        		        
    		        inped = inped.copy();
    		        
        			if(inped.isEmpty() || playerIn.addItemStackToInventory(inped)) {
        				ped.setInventorySlotContents(0, pitem);
        				playerIn.setHeldItem(EnumHand.MAIN_HAND, item);
        			}
        			
    				tile.markDirty();
    		        worldIn.playSound((EntityPlayer)null, pos, SoundEvents.BLOCK_ENCHANTMENT_TABLE_USE, SoundCategory.BLOCKS, 0.3F, 0.6F);
	        	}
	        }
        }
        return true;
    }