Choonster

That's not the full FML log, that's the console output. It does show that this error is happening multiple times a second: The server is sending invalid chunk data, which could be the cause of the framerate drops. I don't think there's much you can do about this, apart from telling the server to fix the issue or playing on a different server. Please post the actual FML log from a session where you experienced the issue, gave Minecraft 2 GB or RAM and didn't have any mods installed (apart from Forge).

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). That looks correct, yes.

It looks correct, apart from this: 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).

In addition to what diesieben07 said, that's not the FML log, that's the console output. There's also no crash in the log you posted. Please post the actual FML log from a session where Minecraft crashes.

Do this by uploading to Gist and linking it here. Don't copy the log directly into your post.

Which version of Minecraft are you using? Does this happen without any mods installed (just Forge)? Upload the FML log (logs/fml-client-latest.log in the game directory) from a session where this framerate drop occurs to Gist and link it here.

You can also use the ChunkPos(BlockPos) constructor to convert a BlockPos to a ChunkPos.

Use your IDE to search for the class by name. In IDEA, use Ctrl-N or Navigate > Class. In Eclipse, use Ctrl-Shift-T or Navigate > Open Type.

InitMapGen is fired when ChunkProviderOverworld/ChunkProviderHell initialise their MapGenBase instances, you can use this to replace or modify them. InitMapGenEvent#getType will return InitMapGenEvent.EventType.VILLAGE when the MapGenBase being initialised (InitMapGenEvent#getOriginalGen) is a MapGenVillage.

"Chat" components were renamed to "Text" components in 1.9. ChatComponentText is now TextComponentString, EnumChatFormatting is now TextFormatting. This issue tracker documents most renames in 1.8+. You can also use MCPBot to find the current name of a field/method/parameter when you have its old name: Use the findf/findm/findp commands with the old MCP (deobfuscated) name and the old Minecraft version to find the SRG (obfuscated) name. Use the same command with the SRG name and the new Minecraft version (or omit the version argument for the current version) to find the current MCP name. Alternatively, use the fh/mh/ph commands with the SRG name to display the name history instead of just the current name.

If you control the Block but not all the Items, it's best to handle the interactions in Block#onBlockActivated. If you control the Items but not the Block, it's best to handle the interactions in Item#onItemUse. If you control neither the Block nor the Items, you can handle the interactions using PlayerInteractEvent.RightClickBlock.

In 1.8.9 (before item properties were added), this was handled by TextureCompass rather than the IItemPropertyGetters and overrides in the model.

No, it's an instance field so you need to specify which instance to change the field's value for. If you get/set the value of a field or call a method with reflection more than once, you should use ReflectionHelper.findField/findMethod to look up the Field/Method object, store it in a private static field and then use it whenever you need to interact with the field/method. Looking up the Field/Method object is relatively slow, so you should only do it once.

It's best to include the pack.mcmeta file so you're using the current resource pack format with lowercase names. LegacyV2Adapter is only meant for loading resource packs designed for previous versions of Minecraft and could be removed at some point in the future, so it's best not to rely on it.

Do you have a pack.mcmeta file in src/main/resources with pack_format set to 3? If you don't, Forge will load your mod's resources using the LegacyV2Adapter IResourcePack wrapper. This means that your lang files will be loaded with the old mixed-case names (e.g. en_US.lang) rather than the new lower-case names (e.g. en_us.lang). If you do, your lang files should be loaded with lower-case names. Try refreshing your IDE project (i.e. click the "Refresh all Gradle projects" button in IDEA's Gradle window or re-run the eclipse Gradle task) and/or rebuilding it.

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. 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.

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. I'm not sure what you're asking.

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. 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. To protect against malicious clients sending the packet when the player isn't actually near the villager.

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). The MessageSendEntityID name isn't much better than EntityIDProxy. The purpose of the packet is to hire a villager, the fact that it sends an entity ID is just an implementation detail. 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. 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. IvVillager#hire_Villager should check the hire cost and the hired status before setting the villager as hired and the player as its owner.

You're not doing it the way I told you to in my previous post, but it's mostly correct. The main issues are that you're running this code on the network thread instead of the main thread (see the Warning box on the Simple Network Wrapper documentation page for an explanation) and that you're checking that the villager is hired before attempting to hire it. Why are using slot 15 of the IItemHandler? You should expose the IItemHandler that stores the emeralds directly (either via ICapabilityProvider#getCapability or a custom method) and use that rather than using a wrapper of all of the villager's inventories (which I presume is why you're using slot 15). You only need to expose an object through ICapabilityProvider if you want external code to be able to access it. Use descriptive names for your classes, fields, methods, etc. that reflect their purpose: EntityIdProxy doesn't tell me that the class is an IMessage, nor does it tell me anything about what the packet does. I recommend naming your packets Message[Action], where [Action] is the action that the packet performs (i.e. why it's sent in the first place). This packet hires a villager, so name it something like MessageHireVillager. toSend doesn't tell me anything about the contents or purpose of the field, the fact that it's sent in a packet is already obvious from the context so it doesn't need to be included in the name. This field stores the villager's entity ID, so name it something like entityID or villagerEntityID. Only declare local variables in the scope where they're used. There's no reason to declare the remaining_i variable at the start of the method if you're only going to use inside the if statement.

You don't access the GUI, you access the IItemHandler inventory that the GUI interacts with. The IMessageHandler should check that the player is in range and then call the hire method. The hire method should take an EntityPlayer argument and check that there are enough emeralds in the villager's inventory and the villager isn't already hired (plus any other prerequisites) before setting the villager as hired and the player as its owner.

The IMessageHandler#onMessage implementation should call the IvVillager method that hires the villager (after checking that it's allowed). Look at the GuiHandler code you merged from my fork of the repository, I did the same thing there.

Look at EntitySlime#getCanSpawnHere (specifically the last if statement) to see how it determines whether the chunk it's currently in is a slime chunk. You'll need to iterate outward through all chunks surrounding the current position until you find one. Minecraft records the positions of large structures like Nether Fortresses or Strongholds (that use the MapGenStructure system), but it doesn't record the positions of smaller structures like Dungeons (that use Vanilla's WorldGenerator or Forge's IWorldGenerator systems). @diesieben07 explained how to search for a TileEntity in a Chunk in the post linked below, you can use this method to find nearby mob spawners.

Whether or not each side needs a Forge mod installed depends on what the mod does. If it adds a new Block or Item, both the client and server must have the mod installed. If it adds a new server-side command, only the server needs to have the mod installed (though the client will also need it installed if you want to use any non-vanilla localisations). If it changes client-side things like rendering, only the client needs to have the mod installed. Set @Mod#clientSideOnly or @Mod#serverSideOnly to true if the mod is client-only or server-only to prevent it from being loaded on the opposite physical side. Set @Mod#acceptableRemoteVersions to "*" (i.e. accept any version, including none) if the mod is only required on one side. Edit: acceptableRemoteVersions, not acceptedMinecraftVersions.

This will fail if the ITextComponent isn't a TextComponentString. ITextComponent already provides the getFormattedText method, you shouldn't need to cast it. You can use ITextComponent#getUnformattedComponentText to get the text without the formatting codes. All vanilla ITextComponents also override Object#equals, so you can use this to compare the two messages.