NichtStudioCode / InvUI

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

Implement AsyncAutoUpdateItem #33

Closed BlackBaroness closed 12 months ago

BlackBaroness commented 1 year ago

Hi.

I noticed that there are AsyncItem and AutoUpdateItem, but not both. So I added this.

I have tested it on my side and no issues found.

Example:

AtomicInteger atomicInteger = new AtomicInteger();
new AutoUpdateAsyncItem(
    new ItemBuilder(Material.WEB).setDisplayName("Loading..."),
    Duration.ofSeconds(1),
    () -> new ItemBuilder(Material.WEB).setDisplayName(String.valueOf(atomicInteger.incrementAndGet()))
);
BlackBaroness commented 1 year ago

Hello? Any suggestions about this?

NichtStudioCode commented 1 year ago

I don't think that Bukkit's scheduler system is well suited for this use-case. Tasks started with runTaskTimerAsynchronously will run every period ticks, even when the previous task hasn't finished yet.

This is an issue for multiple reasons:

I also don't really see a use-case for such an item type. AsyncItem exists because retrieving the texture value for player heads requires web requests, but I don't really see a reason to put anything like database requests or similar in InvUI's item logic.

BlackBaroness commented 1 year ago

I don't think that Bukkit's scheduler system is well suited for this use-case. Tasks started with runTaskTimerAsynchronously will run every period ticks, even when the previous task hasn't finished yet.

This is an issue for multiple reasons:

  • Task B, which started after task A might finish before A, but then A would replace it with outdated data
  • This could lead to performance issues, as the operation might be run more often than necessary
  • This could lead to memory issues, because Bukkit's scheduler creates a new thread for every async task. Tasks such as web- or database requests might take a very long time, which would then lead to a huge amount of threads (and thus memory) being allocated.

I also don't really see a use-case for such an item type. AsyncItem exists because retrieving the texture value for player heads requires web requests, but I don't really see a reason to put anything like database requests or similar in InvUI's item logic.

I need it because I make gui which updates values from the database. With such solution, we can make good-looking, interactive and zero-tps-loss GUIs.

For example, we can create auction GUI which will update in real-time.

I can improve this implementation, by using thread pools or Timer, if you will accept it.

NichtStudioCode commented 1 year ago

I need it because I make gui which updates values from the database.

That's exactly something that I don't want to encourage. Database requests should generally not be on a timer, and if they really need to be for some reason, they should not be run from within InvUI's item logic. If two people have the same Gui opened, why should the database be queried twice? Adding to that, in most cases you'd have more than one of those items in a Gui, so there'd be an additional overhead because you're sending multiple individual queries instead of one big one.

BlackBaroness commented 1 year ago

I need it because I make gui which updates values from the database.

That's exactly something that I don't want to encourage. Database requests should generally not be on a timer, and if they really need to be for some reason, they should not be run from within InvUI's item logic. If two people have the same Gui opened, why should the database be queried twice? Adding to that, in most cases you'd have more than one of those items in a Gui, so there'd be an additional overhead because you're sending multiple individual queries instead of one big one.

This can be not the 2 requests but 2 possible expensive cacheable methods. We are talking about minimizing tps overhead. Is users doesnt need it, they will not use it. But currently there is no such ability without ugly boilerplate.

I think it is possible to create beatiful and flexible implementation for this.

Or another example - minigames system. What about concurrently updating minigame icon information? Isn't it cool enough?