search

NichtStudioCode / InvUI

A spigot library for creating custom inventory-based GUIs.
MIT License
242 stars 19 forks source link

feat: use inventory holders for window management #67

Closed bridgelol closed 5 months ago

bridgelol commented 5 months ago

Use bukkit's inventory holders for more reliable window management

NichtStudioCode commented 5 months ago

What is the benefit of this? Can you provide a case where the current approach would be unreliable?

bridgelol commented 5 months ago

What is the benefit of this? Can you provide a case where the current approach would be unreliable?

This can prevent potential exploits where clients tamper with packets to cause weird behaviour, inventory holders are a lot more robust than storing the players whom have the inventory opened.

Not only that, can be extremely useful for external listeners.

bridgelol commented 5 months ago

image Storing windows like this is also very memory inefficient...

NichtStudioCode commented 5 months ago

This can prevent potential exploits where clients tamper with packets to cause weird behaviour, inventory holders are a lot more robust than storing the players whom have the inventory opened.

Can you provide an actual example, i.e. what packets would need to be sent to access an inventory that was not opened by that player? I cannot think of a way on how this would be possible. The only inventory-related exploit I am aware of is abusing the fact that some plugins check for the inventory title, and then renaming a chest in an anvil to match that title.

Not only that, can be extremely useful for external listeners.

What would your use-case for this be? I generally wouldn't want to encourage this because it kind of defeats the purpose of using a gui library.

Storing windows like this is also very memory inefficient...

That overhead is negligible, probably only a few bytes per player that has a window open. Contrarily, one could also argue that iterating over all online-players to find all open windows, like you're proposing in this pull request, is inefficient, even though it is probably equally irrelevant.

I'm generally not against switching to inventory holders, and they should've probably been used from the beginning, but the current approach is stable (at least to my knowledge) and changing it would require me to re-test everything on all supported versions, which is very tedious. I also think you forgot about AnvilWindow and CartographyWindow in this pull request, but I haven't tested it.

bridgelol commented 5 months ago

I'm generally not against switching to inventory holders, and they should've probably been used from the beginning, but the current approach is stable (at least to my knowledge) and changing it would require me to re-test everything on all supported versions, which is very tedious. I also think you forgot about AnvilWindow and CartographyWindow in this pull request, but I haven't tested it.

You're right, didn't realize anvil & cartography windows use straight nms upon creation, will have to figure something out for this.

Contrarily, one could also argue that iterating over all online-players to find all open windows, like you're proposing in this pull request, is inefficient, even though it is probably equally irrelevant.

As you said it is irrelevant for its use case at the moment, given it's only accessed upon disabling the plugin, was originally going to propose removing this method altogether as it doesn't seem very useful given spigos api should be plenty to handle this if you were to need it.

Overall, using inventory holders is best practice to store any inventory-related data as the API recommends.

NichtStudioCode commented 5 months ago

As I already said, I don't see a reason to move forward with this PR unless you can provide an actual example of how the current approach is not stable, even if it is not following best practices.

portlek commented 5 months ago

inventory holders are not meant to be used for custom inventories, according to choco who's one of the main spigot maintainers.

NichtStudioCode commented 5 months ago

inventory holders are not meant to be used for custom inventories, according to choco who's one of the main spigot maintainers.

Interesting, but that thread is from 2018 and nothing has been deprecated yet. The paper team also indeed encourages the use of custom inventory holders.