TechnicallyCoded / Inventory-Rollback-Plus

Spigot/Bukkit plugin that allows server moderators to restore player items and data from backups.
https://www.spigotmc.org/resources/85811/
Other
78 stars 44 forks source link

Feature Request: make husksync compatible #92

Closed xF3d33 closed 2 years ago

xF3d33 commented 2 years ago

What is missing:

support for husksync

Specific changes to make:

husksync is a plugin like mysqlplayerdatabridge, but with husksync irp can't save the player inventory ONLY on join and leave. it still can on world change and death because the player still has the inv. husksync on player leave remove the inventory.

this plugin is paid but it's also open source, and with his api is pretty simple to make irp compatible. https://github.com/WiIIiam278/HuskSync

https://jitpack.io/#WiIIiam278/HuskSync/Tag https://javadoc.jitpack.io/com/github/WiIIiam278/HuskSync/latest/javadoc/index.html

https://www.spigotmc.org/resources/husksync-1-16-1-17-1-18-synchronize-player-inventories-data-cross-server.97144/

TechnicallyCoded commented 2 years ago

Send a bug report to their github/discord. If the inventory is correctly loaded on the player before the last possible "on join" event priority (priority = "monitor"), IRP will log the join correctly. Same goes for the quit event. IRP logs the inventory on priority = "normal". Husksync should only remove their items on the last possible priority (highest)

TechnicallyCoded commented 2 years ago

FYI, I am gonna make a random guess that they are doing one of 2 things:

  1. Loading the player's inventory AFTER the player has fully joined the server using an async task. That is terrible practice. The inventory contents should be retrieved from the DB on AsyncPlayerPreLoginEvent, cached, and then applied on PlayerJoinEvent (priority = LOWEST).
  2. Correctly fetching the inv at async pre login event but only applying the inventory at priority NORMAL, HIGH, or HIGHEST (aka the last 3 to fire before the final MONITOR priority). LOWEST is the first to fire, contrary to popular belief.

You can send this to their dev team to help them fix it.

TechnicallyCoded commented 2 years ago

I just looked at their plugin page btw: first of all 12 GBP for a inventory sync plugin is like 3x too expensive lol.. and secondly it really does look like they are doing what I said in possibility 1 which means that they are charging money for a plugin which isn't written correctly and won't work with any other plugin. Possibility 1 is honestly what I would consider an amateur mistake. Too bad most people will only notice this too late.

xF3d33 commented 2 years ago

i don't know if you looked at the source or not, but the plugin loads the inventory from redis, which receives it from the proxy that takes it from the db. but i don't program java, and i don't know what to tell you. so what should i do?

TechnicallyCoded commented 2 years ago

image Fail #1

TechnicallyCoded commented 2 years ago

image Fail #2

TechnicallyCoded commented 2 years ago

Exactly as I assumed

TechnicallyCoded commented 2 years ago

So I can confirm they are using the noob approach. I really don't recommend using that plugin.

xF3d33 commented 2 years ago

ok so for the first screen the priority need to be HIGHEST, and for the second?

TechnicallyCoded commented 2 years ago

For the first screenshot it needs to be highest but for the second it would require them to redesign how the plugin works. Data sync across servers is harder than it can seem, but what they are doing there is definitely not the way you should be working with bukkit. From what I understand, they could be using that method as a work-around for some issues in how proxies like bungee/velocity work. However, this also means that the plugin won't work with any other plugin that's using the API correctly (for inventory-related actions when the player joins).

xF3d33 commented 2 years ago

image

what needs to be changed here?

TechnicallyCoded commented 2 years ago

That was just showing that my assumption was correct. There is nothing to change about that line. The whole way this works is broken, it would need a complete re-write, and as I said, they probs chose against doing it the right way due to data sync issues.