Fix stackoverflow by converting TradeLogs to Strings on write #63
Conversation
This is totally untested as I wasn't able to pull the PluginBase dependency easily, but should fix #60 and fix #62 if I didn't mess anything up. I believe the root of the stackoverflow has something to do with ItemStack not being serializable (`org.bukkit.configuration.serialization.ConfigurationSerializable` != `java.io.Serializable`), so I've switched to using strings. I'm making an assumption in this patch that there shouldn't be a reason we would need to restore a TradeLog from what is written to the file since that seems like overkill for basic logging. If that's a bad assumption, we should implement a better TypeAdapter that will handle the serialization of ItemStacks.
leviem1
changed the title
|
Thank you so much for your effort 😄 I am loading it up to test it, I think I'm going to want to implement ItemStack serialization, though. I think this will look like {DIAMOND_SWORD x 1}, {POTATO x 50} and some players use it to trade specific special items so the admin would need to see the display name or enchantments to know the information they need. 😃 I didn't realize this was the problem causing the stack overflows. The lag at the beginning of a trade might is a separate issue. When I do my first trade on my server, it says loading legacy material support even on 1.16, which I thought I had avoided with my versioning but it still does it for some reason. Maybe because of the api-version in my plugin.yml? Or it could just be the way I am instantiating ItemStacks, I need to figure that one out. |
|
Yeah, serialization seems like the more "correct" way of doing it, though then you have to worry about the deserialization which was more than I wanted to do with a quick hack 😆 Feel free to either commit to this branch with any changes you'd like to make or close it. I could definitely look into the serialization too, but it could take a me a bit. I happen to not get the material support issue since I'm running essentials or something that loads that already at server start. |
|
Also, a few possible culprits Trade.java MsgUtils1_8.java |
|
Thinking about it weeks later... you could probably make a wrapper object for the item stack that is serializable. |
This is totally untested as I wasn't able to pull the PluginBase dependency easily (#57), but should fix #60 and fix #62 if I didn't mess anything up.
I believe the root of the stackoverflow has something to do with ItemStack not being serializable (
org.bukkit.configuration.serialization.ConfigurationSerializable!=java.io.Serializable), so I've switched to using Strings.I'm making an assumption in this patch that there shouldn't be a reason we would create a TradeLog from the string that is written to the file since that seems like overkill for basic logging. If that's a bad assumption, we should implement a better TypeAdapter that will handle the serialization of ItemStacks.
This small fix took me way longer than it should have, so if it works you owe me a coffee! 😆
Always happy to help!