[1.11.2] [Unsolved] Accessing an entity's gui container

[OrangeVillager61]

11 hours ago, Choonster said:

 

You can't safely interact with normal Minecraft classes from the network thread, so the first thing you do in IMessageHandler#onMessage should be to schedule the task to run on the main thread. Only get the villager and hire it inside the scheduled task.

 

WorldServer#addScheduledTask isn't a static method, you need to call it on an instance of WorldServer. The MinecraftServer class also implements IThreadListener, so you can use the server instance (EntityPlayerMP#mcServer) to schedule the task instead of a WorldServer instance (the WorldServer implementation of IThreadListener delegates to the MinecraftServer implementation anyway).

 

Alright, but what do I do with it after I call the method? You said I need to use the code after I get the villager into addScheduledTask so do I make a seperate method and put my code in said method or this?

@Override public IMessage onMessage(MessageSendEntityId message, MessageContext ctx) {
		 // This is the player the packet was sent to the server from
		 EntityPlayerMP serverPlayer = ctx.getServerHandler().player;
		 // The value that was sent
		 int amount = message.EntityID;
		 IvVillager villager = (IvVillager) ctx.getServerHandler().player.world.getEntityByID(amount);
		 serverPlayer.mcServer.addScheduledTask(villager.hire_Villager(serverPlayer);
		 // No response packet
		 return null;
	}

11 hours ago, Choonster said:

 

Entity already implements ICapabilityProvider, so IvVillager implements it as well. You don't need to create a new implementation. What I was trying to say is that you only need to expose an IItemHandler (or any other capability handler instance) through the ICapabilityProvider methods if you want vanilla or other mods to interact with it. If only your code needs to interact with it, you can create your own method in IvVillager to return it.

If I do the below, I would still need to use different ids for all containers I have added to my villager or is that intended?

	public ItemStackHandler getItemHandler()
	{
		return this.item_handler;
	}

 

11 hours ago, Choonster said:

 

The IMessageHandler shouldn't really be checking the hire cost itself, it should simply check that the player is within range of the villager before calling IvVillager#hire_Villager.

Why do I check if the player is close enough if the GUI that the player uses cannot be opened unless they are close enough or is just for malicious code? 

[Choonster]

7 hours ago, OrangeVillager61 said:

Alright, but what do I do with it after I call the method? You said I need to use the code after I get the villager into addScheduledTask so do I make a seperate method and put my code in said method or this?

 

You need to call IThreadListener#scheduleTask with a Runnable implementation that performs the appropriate action in its run method (an anonymous class or lambda is usually used here). Currently you're immediately calling IvVillager#hire_Villager (on the network thread) and passing the result to IThreadListener#scheduleTask.

 

The only thing you do on the network thread should be scheduling the task to run on the main thread.

 

 

7 hours ago, OrangeVillager61 said:

If I do the below, I would still need to use different ids for all containers I have added to my villager or is that intended?

 

What do you mean by "ids"?

 

If you mean the index argument of the Slot/SlotItemHandler constructors (assigned to Slot#slotIndex/SlotItemHandler#index), that's the slot index within the inventory (IItemHandler) that the Slot should read from/write to. If you have Slots for multiple inventories in a Container (which is often the case), you'll often have multiple Slots with the same index argument but different inventories (e.g. one for slot 0 of the villager's inventory and one for slot 0 of the player's inventory).

 

Slot#slotNumber is the unique index of the Slot in the Container, this is automatically assigned when you call Container#addSlot.

 

 

7 hours ago, OrangeVillager61 said:

Why do I check if the player is close enough if the GUI that the player uses cannot be opened unless they are close enough or is just for malicious code? 

 

To protect against malicious clients sending the packet when the player isn't actually near the villager.

Edited June 4, 2017 by Choonster

[OrangeVillager61]

11 hours ago, Choonster said:

 

You need to call IThreadListener#scheduleTask with a Runnable implementation that performs the appropriate action in its run method (an anonymous class or lambda is usually used here). Currently you're immediately calling IvVillager#hire_Villager (on the network thread) and passing the result to IThreadListener#scheduleTask.

 

The only thing you do on the network thread should be scheduling the task to run on the main thread.

 

Is this what you meant?

public class HireVillagerPacket implements IMessageHandler<MessageSendEntityId, IMessage> {
// Do note that the default constructor is required, but implicitly defined in this case
	
	@Override public IMessage onMessage(MessageSendEntityId message, MessageContext ctx) {
		 // This is the player the packet was sent to the server from
	     EntityPlayerMP serverPlayer = ctx.getServerHandler().player;
		 // The value that was sent
		 int amount = message.EntityID;
		 serverPlayer.mcServer.addScheduledTask(Run_Hire_Villager.run_hire_villager(ctx.getServerHandler().player.mcServer, message.EntityID, ctx.getServerHandler().player));
		 // No response packet
		 return null;
	}
}

public class Run_Hire_Villager {

	 public static Runnable run_hire_villager(MinecraftServer server, int EntityID, EntityPlayerMP player)
	 {
		 World world = server.getEntityWorld();
		 IvVillager villager = (IvVillager) world.getEntityByID(EntityID);
		 if (player.getDistanceToEntity(villager) <= 10)
		 {
			 villager.hire_Villager(player);
			 return null;
		 }
		return null;
	 }
}

Edited June 4, 2017 by OrangeVillager61

[OrangeVillager61]

11 hours ago, Choonster said:

What do you mean by "ids"?

 

If you mean the index argument of the Slot/SlotItemHandler constructors (assigned to Slot#slotIndex/SlotItemHandler#index), that's the slot index within the inventory (IItemHandler) that the Slot should read from/write to. If you have Slots for multiple inventories in a Container (which is often the case), you'll often have multiple Slots with the same index argument but different inventories (e.g. one for slot 0 of the villager's inventory and one for slot 0 of the player's inventory).

 

Slot#slotNumber is the unique index of the Slot in the Container, this is automatically assigned when you call Container#addSlot.

Alright, but how do I get the container for the different slot ids since I have two different containers for it in addition to the already villager stuff?

[Choonster]

14 hours ago, OrangeVillager61 said:

Is this what you meant?

 

No, you're still not scheduling a task. You can't just create a method that returns Runnable and return null from it, you need to actually create a new instance of an anonymous (or named) class that implements Runnable and overrides Runnable#run to perform the action.

 

You're still retrieving the entity ID from the packet outside of what would be the scheduled task (even though you're not using it), which you shouldn't really do.

 

14 hours ago, OrangeVillager61 said:

Alright, but how do I get the container for the different slot ids since I have two different containers for it in addition to the already villager stuff?

 

I'm not sure what you're asking.

Edited June 5, 2017 by Choonster

[OrangeVillager61]

10 hours ago, Choonster said:

 

No, you're still not scheduling a task. You can't just create a method that returns Runnable and return null from it, you need to actually create a new instance of an anonymous (or named) class that implements Runnable and overrides Runnable#run to perform the action.

 

You're still retrieving the entity ID from the packet outside of what would be the scheduled task (even though you're not using it), which you shouldn't really do.

Is this below what you meant then?

public class HireVillagerPacket implements IMessageHandler<MessageSendEntityId, IMessage> {
// Do note that the default constructor is required, but implicitly defined in this case
	
	@Override public IMessage onMessage(MessageSendEntityId message, MessageContext ctx) {
		 // This is the player the packet was sent to the server from
		 EntityPlayerMP serverPlayer = ctx.getServerHandler().player;
		 // The value that was sent
		 serverPlayer.mcServer.addScheduledTask(Runnable_Hire_Villager(ctx.getServerHandler().player.mcServer, message, ctx));
		 // No response packet
		 return null;
	}
}

public class Runnable_Hire_Villager implements Runnable{
	
	private IvVillager villager;
	private int EntityID;
	private EntityPlayerMP player;
	private World world;
	
	public Runnable_Hire_Villager(MinecraftServer mcServer, MessageSendEntityId message, MessageContext ctx){
	     this.player = ctx.getServerHandler().player;
		 this.EntityID = message.EntityID;
		 this.world = mcServer.getEntityWorld();
		 this.villager = (IvVillager) world.getEntityByID(EntityID);
	}

	@Override
	public void run() {
		if (this.player.getDistanceToEntity(this.villager) <= 10)
		 {
			 this.villager.hire_Villager(this.player);
		 }		
	}
}

10 hours ago, Choonster said:

 

I'm not sure what you're asking.

I mean, how do I get individual containers and get their inv slots separately out of the villager item_handler?

[Choonster]

On 2017-6-6 at 2:29 AM, OrangeVillager61 said:

Is this below what you meant then?

 

That's close, but you're trying to call the constructor like a regular method instead of using the new operator, which won't compile.

 

You're using MinecraftServer#getEntityWorld to get the World, but this is always dimension 0, which the player may not be in. You should use the player's World instead.

 

You're also getting the villager from the World in the constructor, which is still called on the network thread. Don't do this, it's not safe to call methods like this from the network thread.

 

The Runnable implementation doesn't have to be a named class, it could be an anonymous class or a lambda. If you want it to be named, I suggest making it a nested class of the IMessage or the IMessageHandler.

 

 

On 2017-6-6 at 2:29 AM, OrangeVillager61 said:

I mean, how do I get individual containers and get their inv slots separately out of the villager item_handler?

 

You don't get a container out of an inventory, I'm not entirely sure what you're talking about.

 

You can have multiple Container classes that interact with the same slots of an inventory without issue. For example, almost every Container includes the same slots of the player's inventory.

 

An IItemHandler is a persistent inventory, it's generally created once when the containing object (e.g. an Entity or TileEntity) is created and read from and written to NBT with the containing object.

 

A Container is a temporary view of specific slots of one or more inventories, it's generally created when a player opens a GUI and released for garbage collection when the GUI is closed. Its sole purpose is to handle client/server interaction and synchronisation for a GUI.

Edited June 9, 2017 by Choonster

[OrangeVillager61]

Ah, I understand now regarding the packet (I'll still post it below as a finished solution/in case I still screwed up).

public class HireVillagerPacket implements IMessageHandler<MessageSendEntityId, IMessage> {
// Do note that the default constructor is required, but implicitly defined in this case
	
	@Override public IMessage onMessage(MessageSendEntityId message, MessageContext ctx) {
		 // This is the player the packet was sent to the server from
		 EntityPlayerMP serverPlayer = ctx.getServerHandler().player;
		 // The value that was sent
		 serverPlayer.mcServer.addScheduledTask(new Runnable_Hire_Villager(message, ctx));
		 // No response packet
		 return null;
	}
}
class Runnable_Hire_Villager implements Runnable{
	
	private IvVillager villager;
	private int EntityID;
	private EntityPlayerMP player;
	private World world;
	
	public Runnable_Hire_Villager(MessageSendEntityId message, MessageContext ctx){
	     this.player = ctx.getServerHandler().player;
		 this.EntityID = message.EntityID;
		 this.world = this.player.world;
		 this.villager = (IvVillager) world.getEntityByID(EntityID);
	}

	@Override
	public void run() {
		if (this.player.getDistanceToEntity(this.villager) <= 10)
		 {
			 this.villager.hire_Villager(this.player);
		 }		
	}
}

With the containers I assumed that each container contains different slots, if they always refer to the same slot id throughout the item handler, then how do I only use my mod's inventory or is starting from 0 okay?

 

-Edit I have started preliminary testing and results don't look good. 

Edited June 9, 2017 by OrangeVillager61

[Choonster]

18 minutes ago, OrangeVillager61 said:

Ah, I understand now regarding the packet (I'll still post it below as a finished solution/in case I still screwed up).

 

It looks correct, apart from this:

On 2017-6-6 at 3:01 PM, Choonster said:

You're also getting the villager from the World in the constructor, which is still called on the network thread. Don't do this, it's not safe to call methods like this from the network thread.

 

 

19 minutes ago, OrangeVillager61 said:

With the containers I assumed that each container contains different slots, if they always refer to the same slot id throughout the item handler, then how do I only use my mod's inventory or is starting from 0 okay?

 

When you create a Slot, you pass it an inventory (IItemHandler) and a slot index (the slot in that inventory to read from/write to). Two instances of the same Container class will generally have Slots with the same inventories and slot indexes.

 

Your villager Containers should have one Slot for slot 0 of the villager's inventory (the only slot) and 36 Slots for slots 0-35 of the player's inventory (the main inventory section).

[OrangeVillager61]

4 minutes ago, Choonster said:

 

It looks correct, apart from this:

Um, how do I get the world without using the ctx?

4 minutes ago, Choonster said:

When you create a Slot, you pass it an inventory (IItemHandler) and a slot index (the slot in that inventory to read from/write to). Two instances of the same Container class will generally have Slots with the same inventories and slot indexes.

 

Your villager Containers should have one Slot for slot 0 of the villager's inventory (the only slot) and 36 Slots for slots 0-35 of the player's inventory (the main inventory section).

So this container is correct (my other just has more slots?

public class ContainerIvVillagerHireNitwit extends Container{

	private IvVillager villager;
	private IItemHandler handler;
	
	public ContainerIvVillagerHireNitwit(IvVillager villager, IInventory playerInv){
		this.villager = villager;
		this.handler = this.villager.getCapability(CapabilityItemHandler.ITEM_HANDLER_CAPABILITY, null);

		this.addSlotToContainer(new SlotItemHandler(handler, 0, 76, 47));
		int xPos = 8;
		int yPos = 84;
		
		for (int y = 0; y < 3; ++y) {
			for (int x = 0; x < 9; ++x) {
				this.addSlotToContainer(new Slot(playerInv, x + y * 9 + 9, xPos + x * 18, yPos + y * 18));
			}
		}
				
		for (int x = 0; x < 9; ++x) {
			this.addSlotToContainer(new Slot(playerInv, x, xPos + x * 18, yPos + 58));
		}
	}
}
                              

 

[Choonster]

Just now, OrangeVillager61 said:

Um, how do I get the world without using the ctx?

 

Store the World in the constructor (i.e. on the network thread), but only call World#getEntityByID in the run method (i.e. on the main thread).

 

 

2 minutes ago, OrangeVillager61 said:

So this container is correct (my other just has more slots?

 

That looks correct, yes.

[OrangeVillager61]

Alright, but the hiring of the villager button and then the enable/disable follow on the hired villager still don't work (do nothing) and the console doesn't print any errors or anything. I think it has something with my GUI but it may be something else, github.

 

public class GuiIvVillagerHireNitwit extends GuiContainer{

	private IvVillager villager;
	private IInventory playerInv;
	private EntityPlayer player;
	protected int remaining_i = 0;
	
	public GuiIvVillagerHireNitwit(IvVillager villager, IInventory playerInv, EntityPlayer player) {
		super(new ContainerIvVillagerHireNitwit(villager, playerInv));
		
		this.xSize = 176;
		this.ySize = 166;
		this.player = player;
		
		this.villager = villager;
		this.playerInv = playerInv;

	}

	@Override
	public void initGui()
	{
		super.initGui();
        this.addButton(new Button_Hire(0, this.getGuiLeft() + 115, this.getGuiTop() + 20, 50, 25, "Hire", this.villager, this.player, this.remaining_i));
	}
	@Override
	protected void drawGuiContainerBackgroundLayer(float partialTicks, int mouseX, int mouseY) {
		GlStateManager.color(1.0F, 1.0F, 1.0F, 1.0F);
		this.mc.getTextureManager().bindTexture(new ResourceLocation(Reference.MOD_ID, "gui/hire_nitwit.png"));
		this.drawTexturedModalRect(this.getGuiLeft(), this.getGuiTop(), 0, 0, this.xSize, this.ySize);
	}
	
	@Override
    protected void actionPerformed(GuiButton button)
    {
		Boolean has_emeralds;
        if (this.inventorySlots.getSlot(0).getStack().getCount() >= villager.getHireCost() && this.inventorySlots.getSlot(0).getStack().getItem().equals(Items.EMERALD)){ 
        	has_emeralds = true;
        }
        else 
        {
        	has_emeralds = false;
        }
        if (has_emeralds)
        {
        	Reference.PACKET_MODID.sendToServer(new MessageSendEntityId(this.villager.getEntityId()));
        }
    }
	
	@Override
	protected void drawGuiContainerForegroundLayer(int mouseX, int mouseY)
    {
        String s = this.villager.getName();

        this.mc.fontRenderer.drawString(String.valueOf(villager.getHireCost()), 46, 50, 4210752);
        this.mc.fontRenderer.drawString(s, this.xSize / 2 - this.mc.fontRenderer.getStringWidth(s) / 2, 6, 4210752);
        this.mc.fontRenderer.drawString(this.playerInv.getDisplayName().getFormattedText(), 8, 72, 4210752);
    }

}
public class Button_Hire extends GuiButton{
	
	public IvVillager villager;
	
	
	public Button_Hire(int buttonId, int x, int y, int widthIn, int heightIn, String buttonText, IvVillager villager, EntityPlayer player, int remaining_i) {
		super(buttonId, x, y, widthIn, heightIn, buttonText);
		
		PointerInfo a = MouseInfo.getPointerInfo();
		Point b = a.getLocation();
		int mouseX = (int) b.getX();
		int mouseY = (int) b.getY();
	}
	public boolean mousePressed(int mouseX, int mouseY)
    {
        return this.enabled && this.visible && mouseX >= this.xPosition && mouseY >= this.yPosition && mouseX < this.xPosition + this.width && mouseY < this.yPosition + this.height;
    }

}

 

[Choonster]

I encountered several issues with your code:

 

You're registering two different IMessageHandlers (HireVillagerPacket and ChangeFollowPacket) for the same IMessage class (MessageSendEntityId), which doesn't work. Each IMessage class can only have a single IMessageHandler.

 

These classes are poorly named. HireVillagerPacket and ChangeFollowPacket are packet handlers, not packets. MessageSendEntityId tells me what the packet sends, but it doesn't tell me the most important piece of information: why the packet is sent, i.e. what action does it perform? You use Packet in the handler names but you use Message in the packet name, pick one and stick with it.

 

GuiHandler#getClientGuiElement incorrectly returns a Container instead of a GuiScreen for the Hauler ID.

 

GuiIvVillagerHauler uses ContainerIvVillagerHireNitwit instead of ContainerIvVillagerHauler.

 

ContainerIvVillagerHauler adds 15 Slots for the villager's inventory, but the IItemHandler created in IvVillager only has one slot.

 

The Button_Hire and Button_Follow classes no longer serve a purpose, you can replace their usages with regular GuiButtons.

 

When you have an if statement that does nothing but set the value of a boolean variable (like in GuiIvVillagerHireNitwit#actionPerformed), you can move the expression used as the condition of the if statement directly to the initialisation of the boolean variable and remove the if statement.

 

When you have a boolean variable declared directly before and only used in the condition of the if statement, you can move the expression used to initialise it directly into the condition of the if statement and remove the boolean variable. If the condition of an if statement is particularly complex, it can be clearer to keep some parts of it as variables.

 

I've fixed these issues and pushed the changes to GitHub. You can view and/or merge the changes here.

 

You appear to be using GitHub's web UI to upload changes to your repository, I highly recommend using a proper local Git client instead. This will allow you to create more fine-grained commits with descriptive messages rather than lumping all your changes into a single commit with a message like "Add files via upload".

[OrangeVillager61]

2 hours ago, Choonster said:

You appear to be using GitHub's web UI to upload changes to your repository, I highly recommend using a proper local Git client instead. This will allow you to create more fine-grained commits with descriptive messages rather than lumping all your changes into a single commit with a message like "Add files via upload".

Yeah, I've had many issues with linking my workspace with a local Git, I'll try to continue this after I complete this update.

 

I will go through the changes and I'll reply if there are remaining issues.

[OrangeVillager61]

The hiring mostly works but I have one major issue:

 

The inventories are reset on reload, I have no clue how to prevent this.

[Choonster]

19 minutes ago, OrangeVillager61 said:

The hiring mostly works but I have one major issue:

 

The inventories are reset on reload, I have no clue how to prevent this.

 

You never write the IItemHandler to or read it from NBT, so it's not persisted when the entity is unloaded/reloaded. Do this using the INBTSerialializable methods implemented by ItemStackHandler.

Edited June 13, 2017 by Choonster

[OrangeVillager61]

How do I do that? What do I use to handle the serializeNBT?

 

@Override
	 public void writeEntityToNBT(NBTTagCompound compound)
	    {
	        super.writeEntityToNBT(compound);
	        if (world.isRemote == false){
		        this.item_handler.serializeNBT();
			}

 

Edited June 13, 2017 by OrangeVillager61

[Choonster]

36 minutes ago, OrangeVillager61 said:

How do I do that? Is this what you suggested?

 

That's the right method, but you actually need to store the compound tag returned by it in the compound tag you receive as an argument to writeEntityToNBT. Currently you're just discarding it.

 

You also need to do the reverse with INBTSerializable#deserializeNBT in the readEntityFromNBT method.

 

Why are you checking World#isRemote? It's not necessary in these methods and may break things.

[OrangeVillager61]

1 minute ago, Choonster said:

 

That's the right method, but you actually need to store the compound tag returned by it in the compound tag you receive as an argument to writeEntityToNBT. Currently you're just discarding it.

 

You also need to do the reverse with INBTSerializable#deserializeNBT in the readEntityFromNBT method.

I know but what kind of compound I use to set the serialization?

1 minute ago, Choonster said:

Why are you checking World#isRemote? It's not necessary in these methods and may break things.

Alright, I'll remove this.

[OrangeVillager61]

Also, I've been getting this error, do you know what it means?

 

[Server thread/INFO]: [STDOUT]: 7c91ca05-60d3-468c-9c4e-5a2202557afe

 

[Choonster]

3 minutes ago, OrangeVillager61 said:

I know but what kind of compound I use to set the serialization?

 

In writeEntityToNBT, call INBTSerializable#serializeNBT to serialise the ItemStackHandler to a compound tag and then call NBTTagCompound#setTag on the compound tag argument, passing the serialised ItemStackHandler compound tag as the second argument.

 

In readEntityFromNBT, call NBTTagCompound#getCompoundTag on the compound tag argument to get the serialised ItemStackHandler compound tag, then pass it to INBTSerializable#deserializeNBT to deserialise the ItemStackHandler.

 

 

8 minutes ago, OrangeVillager61 said:

Also, I've been getting this error, do you know what it means?

 

That's not an error, that's someone printing a UUID to stdout. I don't think Vanilla or Forge do this, so it's probably your code.

[OrangeVillager61]

4 minutes ago, Choonster said:

 

In writeEntityToNBT, call INBTSerializable#serializeNBT to serialise the ItemStackHandler to a compound tag and then call NBTTagCompound#setTag on the compound tag argument, passing the serialised ItemStackHandler compound tag as the second argument.

 

In readEntityFromNBT, call NBTTagCompound#getCompoundTag on the compound tag argument to get the serialised ItemStackHandler compound tag, then pass it to INBTSerializable#deserializeNBT to deserialise the ItemStackHandler.

 

So this then?

@Override
	 public void writeEntityToNBT(NBTTagCompound compound)
	    {
	        super.writeEntityToNBT(compound);
		        compound.setInteger("Profession", this.getProfession());
		        compound.setString("ProfessionName", this.getProfessionForge().getRegistryName().toString());
		        compound.setInteger("Riches", this.wealth);
	        	compound.setInteger("Gender", this.getGender());
		        compound.setInteger("Int_Age", this.getIntAge());
		        compound.setInteger("Hire_Cost", this.getHireCost());
		        compound.setBoolean("Is_Hired", this.getHired());
		        compound.setInteger("Career", this.careerId);
		        compound.setInteger("CareerLevel", this.careerLevel);
		        compound.setBoolean("Willing", this.isWillingToMate);
		        compound.setTag("Villager_Inv", this.item_handler.serializeNBT());

}

	 @Override
	 public void readEntityFromNBT(NBTTagCompound compound){
		 super.readEntityFromNBT(compound);
		 this.item_handler.deserializeNBT((NBTTagCompound) compound.getTag("Villager_Inv"));
}

4 minutes ago, Choonster said:

 

That's not an error, that's someone printing a UUID to stdout. I don't think Vanilla or Forge do this, so it's probably your code.

Huh, some code that I forgot to remove when I got the player id fixed.

[Choonster]

2 minutes ago, OrangeVillager61 said:

So this then?

 

That looks correct, but you should use NBTTagCompound#getCompoundTag rather than NBTTagCompoundTag#getTag in readEntityFromNBT.

 

There's no need to read/write values that are already handled by the parent class, e.g. the Forge profession.

[OrangeVillager61]

Lastly, how do I allow the button to dynamically change from following to follow and vice versa in the text as a confirmation that the packet had been sent and worked?

[Choonster]

2 minutes ago, OrangeVillager61 said:

Lastly, how do I allow the button to dynamically change from following to follow and vice versa in the text as a confirmation that the packet had been sent and worked?

 

Override GuiButton#drawButton to set GuiButton#displayString based on the villager's following state and then call the super method.