search

Pyrbu / ZNPCsPlus

A Spigot plugin for creating interactable fake entities
https://www.spigotmc.org/resources/znpcsplus.109380/
GNU General Public License v3.0
118 stars 29 forks source link

Fixed errors and ddded blacklist api methods #129

Closed alexdev03 closed 9 months ago

alexdev03 commented 9 months ago

Fixed packet events error Fixed itemstack/equitment errors

Pyrbu commented 9 months ago

What are the blacklist methods for? If you want to define your own logic for how a specific NPC is to be shown to players then you can disable processing on it's entry and write your own processor that will only show it to the players you desire.

alexdev03 commented 9 months ago

What are the blacklist methods for? If you want to define your own logic for how a specific NPC is to be shown to players then you can disable processing on it's entry and write your own processor that will only show it to the players you desire.

Ok I think at this point only the itemstack fix should remain. Right?

Pyrbu commented 9 months ago

Ok I think at this point only the itemstack fix should remain. Right?

I'd prefer if you found the actual cause of the errors and changed that instead of just doing simple patch, do you have a stack trace of the error? I forgot which properties the issue occurs with.

alexdev03 commented 9 months ago

[16:37:43] [DefaultDispatcher-worker-1/WARN]: java.lang.ClassCastException: class org.bukkit.craftbukkit.v1_20_R3.inventory.CraftItemStack cannot be cast to class lol.pyr.znpcsplus.libraries.packetevents.api.protocol.item.ItemStack (org.bukkit.craftbukkit.v1_20_R3.inventory.CraftItemStack is in unnamed module of loader com.universeprojects.uloader.R @5b0a12b0; lol.pyr.znpcsplus.libraries.packetevents.api.protocol.item.ItemStack is in unnamed module of loader 'ZNPCsPlus-2.0.0.jar' @51045cb) [16:37:43] [DefaultDispatcher-worker-1/WARN]: at ZNPCsPlus-2.0.0.jar//lol.pyr.znpcsplus.entity.properties.EquipmentProperty.apply(EquipmentProperty.java:26) [16:37:43] [DefaultDispatcher-worker-1/WARN]: at ZNPCsPlus-2.0.0.jar//lol.pyr.znpcsplus.entity.EntityPropertyImpl.applyStandalone(EntityPropertyImpl.java:56) [16:37:43] [DefaultDispatcher-worker-1/WARN]: at ZNPCsPlus-2.0.0.jar//lol.pyr.znpcsplus.npc.NpcImpl.UNSAFE_refreshProperty(NpcImpl.java:135) [16:37:43] [DefaultDispatcher-worker-1/WARN]: at ZNPCsPlus-2.0.0.jar//lol.pyr.znpcsplus.npc.NpcImpl.setProperty(NpcImpl.java:158) [16:37:43] [DefaultDispatcher-worker-1/WARN]: at ZNPCsPlus-2.0.0.jar//lol.pyr.znpcsplus.npc.NpcImpl.setProperty(NpcImpl.java:151) [16:37:43] [DefaultDispatcher-worker-1/WARN]: at me.gabber235.typewriter.entries.cinematic.ZNPCData$DefaultImpls.handleInventory(ZNPCData.kt:67)

This is the stack trace.

Pyrbu commented 9 months ago

Also I've just realised the way you've done it will most likely just break more things than it fixes, the code expects the map to contain a specific type which is enforced by the type parameter of the set method which could cause even more class cast exceptions. You're also changing how the method works fundamentally because you got rid of the else that was before the set line.

Pyrbu commented 9 months ago

After looking at this for a while longer, this will probably be the least performance-effecting way of making these properties accessible through the API since if we changed the type of the properties they would have to be converted every time a metadata packet is sent, I will fix the other issue I mentioned with your code and merge it.

Pyrbu commented 9 months ago

Unfortunately what TypeWriter does here shouldn't even be possible and is what is causing this (I'm assuming its kotlin not respecting type parameters), however this is still very much an issue with this plugin because there is currently no way of setting ItemStack properties through the API. I will include your patch to fix this issue with plugins that mistakenly use the API like this but I will also make a new method in the API for specifically ItemStack properties. I can't simply change the type of the property to a Bukkit ItemStack because that would incur massive overhead when sending the metadata packets to each player since the item would have to be re-serialized for every viewer. There is another way of fixing this which is using a wrapper class around items kind of like we do with most of the other properties and while it may be cleaner than this fix, I think its still better to do it this way because the alternative will be extremely confusing for people using the API

Pyrbu commented 9 months ago

Not sure how I managed to fuck up my push so badly, we don't talk about it.