Request to modify BTW-edited vanilla class to fix deco bug

[dawnraider]

I have been trying to fix the Deco addon's villager trading bug where you can only complete the first trade in the list. I've finally narrowed down exactly what the cause of it is, which is in how Deco tries to add more functionality to the NetServerHandler by using a decorator pattern to add functionality to the NetServerHandler when it is added to the list of connections in NetworkListenThread.

Code: Select all

public void addPlayer(NetServerHandler par1NetServerHandler)
    {
      //DNCode Start
      //this.connections.add(par1NetServerHandler);
      this.connections.add(new GenericBTWAddonNetServerHandler(this.getServer(), par1NetServerHandler.netManager, par1NetServerHandler.playerEntity));
      //DNCode End
    }

This would work except that it is being done after the NetServerHandler object is already in use elsewhere, from the ServerConfigurationManager method initializeConnectionToPlayer().

Spoiler

Show

Code: Select all

public void initializeConnectionToPlayer(INetworkManager par1INetworkManager, EntityPlayerMP par2EntityPlayerMP)
    {
        NBTTagCompound var3 = this.readPlayerDataFromFile(par2EntityPlayerMP);
        par2EntityPlayerMP.setWorld(this.mcServer.worldServerForDimension(par2EntityPlayerMP.dimension));
        par2EntityPlayerMP.theItemInWorldManager.setWorld((WorldServer)par2EntityPlayerMP.worldObj);
        String var4 = "local";

        if (par1INetworkManager.getSocketAddress() != null)
        {
            var4 = par1INetworkManager.getSocketAddress().toString();
        }

        this.mcServer.getLogAgent().logInfo(par2EntityPlayerMP.username + "[" + var4 + "] logged in with entity id " + par2EntityPlayerMP.entityId + " at (" + par2EntityPlayerMP.posX + ", " + par2EntityPlayerMP.posY + ", " + par2EntityPlayerMP.posZ + ")");
        WorldServer var5 = this.mcServer.worldServerForDimension(par2EntityPlayerMP.dimension);
        ChunkCoordinates var6 = var5.getSpawnPoint();
        this.func_72381_a(par2EntityPlayerMP, (EntityPlayerMP)null, var5);
        NetServerHandler var7 = new NetServerHandler(this.mcServer, par1INetworkManager, par2EntityPlayerMP);
        var7.sendPacketToPlayer(new Packet1Login(par2EntityPlayerMP.entityId, var5.getWorldInfo().getTerrainType(), par2EntityPlayerMP.theItemInWorldManager.getGameType(), var5.getWorldInfo().isHardcoreModeEnabled(), var5.provider.dimensionId, var5.difficultySetting, var5.getHeight(), this.getMaxPlayers()));
        var7.sendPacketToPlayer(new Packet6SpawnPosition(var6.posX, var6.posY, var6.posZ));
        var7.sendPacketToPlayer(new Packet202PlayerAbilities(par2EntityPlayerMP.capabilities));
        var7.sendPacketToPlayer(new Packet16BlockItemSwitch(par2EntityPlayerMP.inventory.currentItem));
        this.func_96456_a((ServerScoreboard)var5.getScoreboard(), par2EntityPlayerMP);
        this.updateTimeAndWeatherForPlayer(par2EntityPlayerMP, var5);
        this.sendPacketToAllPlayers(new Packet3Chat(EnumChatFormatting.YELLOW + par2EntityPlayerMP.getTranslatedEntityName() + EnumChatFormatting.YELLOW + " joined the game."));
        this.playerLoggedIn(par2EntityPlayerMP);
        var7.setPlayerLocation(par2EntityPlayerMP.posX, par2EntityPlayerMP.posY, par2EntityPlayerMP.posZ, par2EntityPlayerMP.rotationYaw, par2EntityPlayerMP.rotationPitch);
        this.mcServer.getNetworkThread().addPlayer(var7);
        var7.sendPacketToPlayer(new Packet4UpdateTime(var5.getTotalWorldTime(), var5.getWorldTime()));

        if (this.mcServer.getTexturePack().length() > 0)
        {
            par2EntityPlayerMP.requestTexturePackLoad(this.mcServer.getTexturePack(), this.mcServer.textureSize());
        }

        Iterator var8 = par2EntityPlayerMP.getActivePotionEffects().iterator();

        while (var8.hasNext())
        {
            PotionEffect var9 = (PotionEffect)var8.next();
            var7.sendPacketToPlayer(new Packet41EntityEffect(par2EntityPlayerMP.entityId, var9));
        }

        par2EntityPlayerMP.addSelfToInternalCraftingInventory();

        if (var3 != null && var3.hasKey("Riding"))
        {
            Entity var10 = EntityList.createEntityFromNBT(var3.getCompoundTag("Riding"), var5);

            if (var10 != null)
            {
                var10.field_98038_p = true;
                var5.spawnEntityInWorld(var10);
                par2EntityPlayerMP.mountEntity(var10);
                var10.field_98038_p = false;
            }
        }

        FCBetterThanWolves.ServerPlayerConnectionInitialized(var7, par2EntityPlayerMP);
    }

Notable among this method are the lines:

Code: Select all

NetServerHandler var7 = new NetServerHandler(this.mcServer, par1INetworkManager, par2EntityPlayerMP);
this.mcServer.getNetworkThread().addPlayer(var7);
FCBetterThanWolves.ServerPlayerConnectionInitialized(var7, par2EntityPlayerMP);

The NetServerHandler is created, passed to addPlayer(), where Deco makes a new version using the GenericBTWAddonNetServerHandler, which extends NetServerHandler, and adds it to the list of connections. However, the original NetServerHandler is sent to BTW, and this discrepancy is what breaks villager trading.

This bug would be easy to fix if I could modify ServerConfigurationManager by changing the object type of var7 to be GenericBTWAddonNetServerHandler. It should be possible using bytecode manipulation to fix the bug otherwise, but it would be much easier to just change the class, so I'd like to request to distribute the BTW edits done to ServerConfigurationManger in the deco addon.

[FlowerChild]

a) No. I don't want to go down the redistribution path.

b) I don't have time to look at this today, but I'll dig into it and try to setup a hook for it in the next couple of days.

[FlowerChild]

Btw, do you have an overview of what this functionality is actually being used for? That could be useful in coming up with an alternative approach for this.

I was just taking a quick look at the code, and I think it's essentially just a custom packet handler for the server, whereas BTW already has similar functionality for the client. My first guess is that I could just extend that functionality slightly to make this whole class unnecessary.

But yeah, an overview of what the actual intention of this thing is would be awesome :)

[FlowerChild]

Yeah, I'm thinking it all comes down to this, from the Deco source on Github, in AddOnManager:

Code: Select all

public static void serverCustomPacketReceived(MinecraftServer ms, EntityPlayerMP epmp, Packet250CustomPayload packet) {
		try {
			DataInputStream dis = new DataInputStream(new ByteArrayInputStream(packet.data));

			if (packet.channel.equals("DECO|OLDGLASS")) {
				int size = dis.readInt();
				int damage = dis.readInt();
								
				ItemStack stack = new ItemStack(30008+256,size,damage);
				epmp.inventory.setInventorySlotContents(epmp.inventory.currentItem, stack);
			}
		} catch (IOException e) {
			e.printStackTrace();
		}
}

Which seems to largely mirror the functionality from BTW's FCAddOn

Code: Select all

    public boolean ClientCustomPacketReceived( Minecraft mcInstance, Packet250CustomPayload packet )
    {
    	return false;
    }

From which Deco's AddOnManager inherits.

Correct me if I'm wrong, but I suspect if I add a ServerCustomPacketReceived() function to FCAddOn, you change the above Deco code to override that instead, and you rip out the whole GenericBTWAddonNetServerHandler thing, this whole problem can go away.

[jorgebonafe]

One comment I'd like to add. This particular piece of code was added by me, for one reason. I had to create a new stained glass for rendering issues with transparent stained blocks. But the old stained glass items still might exist on player inventories, so the only way I could come up with to fix that would be to replace the whole stack in the player's hand as soon as he tried to place it, and I used the custom packets to do that. I don't know if there was a different way to achieve that, but it's the solution I found.

The thing is, this was a while ago, so I think it would be pretty safe to assume no one playing today with deco still have those blocks, and if they do, it would be an exception and not such a big deal. So I don't see why this whole code couldn't just be removed now.

[FlowerChild]

So I don't see why this whole code couldn't just be removed now.

Even better then. Destruction of code is a beautiful thing :)

[jorgebonafe]

Correct me if I'm wrong, but I suspect if I add a ServerCustomPacketReceived() function to FCAddOn, you change the above Deco code to override that instead, and you rip out the whole GenericBTWAddonNetServerHandler thing, this whole problem can go away.

Hey FC, can you still add this hook? Turns out my addon also causes the same villager trader problem for the same reason. I need to be able to handle custom packets for a few things, like so the player can create and name the star maps for Astrolabe, and for a safety test I perform to check server and players have the same version installed.

By the way, I tested your theory about adding a

Code: Select all

ServerCustomPacketReceived(MinecraftServer ms, EntityPlayerMP epmp, Packet250CustomPayload var2)

function to FCAddOn and that indeed worked and was enough to fix all the issues.

[FlowerChild]

Hey FC, can you still add this hook?

I'll take a look. It's been about a year since I opened up the btw code, so I have so serious cobwebs to clear before I can answer that :)

[jorgebonafe]

No worries, thank you :)

[FlowerChild]

Added for next release: FCAddOn.ServerCustomPacketReceived( NetServerHandler handler, Packet250CustomPayload packet )

I changed NetServerHandler.mcServer to public for simplicity's sake, but the server is also a singleton which can always be accessed through MinecraftServer.getServer() anyways.

[jorgebonafe]

Thanks a lot, man :)