Chain

[CJCrafter]

Often, people want to add a callback for after a task is completed. This is done often enough that there are libs. Unfortunately, with all of the new methods for Folia support, all of these libraries are useless for folia. I noticed that you already have some support for this:

• https://github.com/TechnicallyCoded/FoliaLib/blob/main/platform/common/src/main/java/com/tcoded/folialib/impl/ServerImplementation.java#L29
• https://github.com/TechnicallyCoded/FoliaLib/blob/main/platform/common/src/main/java/com/tcoded/folialib/impl/ServerImplementation.java#L38
• https://github.com/TechnicallyCoded/FoliaLib/blob/main/platform/common/src/main/java/com/tcoded/folialib/impl/ServerImplementation.java#L223
• https://github.com/TechnicallyCoded/FoliaLib/blob/main/platform/common/src/main/java/com/tcoded/folialib/impl/ServerImplementation.java#L328
• https://github.com/TechnicallyCoded/FoliaLib/blob/main/platform/common/src/main/java/com/tcoded/folialib/impl/ServerImplementation.java#L338

But we don't have those future instances on the runLater methods. This PR adds those methods. Quick notes:

1. I did not add futures for runTimer methods, because I figured it didn't make much sense. But I am open to adding that to this PR if you'd like.
2. In my edits, I found a recursion error that must've slipped through... You have methods just calling eachother
3. I am requesting to merge this into the dev branch, since it seems you prefer PRs to go there.

Thanks very much!

Fixes #14

[TechnicallyCoded]

I updated the dev branch to include the commits I had pushed to main. Can you pull the dev branch in yours such that I can review changes easier please?

[CJCrafter]

Hey @TechnicallyCoded, I merged your dev branch onto my chain branch.

[TechnicallyCoded]

I don't have much against these changes given that upgrading to the new signature changes is an opt-in process.
But I do have a few questions related to the concept (or purpose) behind these changes:

1. Unless my understanding is wrong, from a purely functional POV (ignoring code-style preference), there isn't much of a difference between adding a call to the next desired method within your runnable/consumer VS using the thenRun() of the future returned. I assume that this is mostly desired to improve readability, simplified mental models, and de-obfuscation of code flow?

2. My only concern related to function here is that we already require wrapping every task due to the multi-platform nature, and within these changes, new object creation would double due to every new CompletableFuture created: once for the task, once again for the post-task execution. In the grand scheme of things, it's not huge but the additional object create/destroy cycle is not optional, even if you don't use the Futures. At the same time, the cost of creating a bunch of new objects that will likely stay within young gen is probably better than exponentially increasing the number of methods to split this behavior into 2 parts: (with future & without future). Unless you have any amazing ideas that doesn't require multiplying the number of methods by 2, we'll just take the overhead hit. Yes it's nitpicking but I am quite sensitive to bloat 🙃 - hence why I created this lib: to avoid all the other libs that added Folia schedulers + x + y + b + re-writing the command registration + ... you get the point.

3. My only requested change here is related to style / docs. I would prefer that it is made very clear to developers which thread the future's thenRun() would execute from. To more experienced developers, it's obvious that the thread wouldn't magically switch back to the one it was called from, but that may not be the case for new users (whether new to the Folia environment or new to development and multi-threading in general). As such, if you can expand upon @ return java-documentation to explain, for each method updated, the behavior of the CompletableFuture. Something like: "@ return CompletableFuture which will complete and execute any subsequent actions on the entity's ticking thread". Feel free to use any better wording but keep the general idea :) .

[CJCrafter]

isn't much of a difference between adding a call to the next desired method

True, but it is very useful syntactic sugar. You often run into cases where you might have a callback... based on an if statement later in code. This syntactic sugar is super useful for maintaining order and readability in those cases.

For example, in WeaponMechanics, I create a reload task for reloading the weapon. Then I might add firearm actions to it, depending on the weapon.

new object creation would double due to every new CompletableFuture created

Object creation in java is expensive when compared to something like a standard function call, or math... but it is one of the most optimized processes in the language. I'd argue that adding 1 more object here won't create a measurable difference in plugins.

BUT, you gave me an alternative idea... What if the WrappedTask class had a bunch of .then methods (e.g. thenRunNextTick, thenRunAtLocationLater, etc.) (or, for less code, just a .then() method which is run immediately after, where you can schedule a new task). Then, all methods in ScheduleImpl could return an instance of WrappedTask instead of CompleteableFuture<Void>.

I would prefer that it is made very clear to developers which thread the future's thenRun() would execute from

Will do! 👍

I'll wait on your comment on the new methods in WrappedTask to make any changes

[CJCrafter]

So I did make those doc changes... I don't really like the idea of adding to WrappedTask just because of all of that clutter... Let me know what you think

[TechnicallyCoded]

One interesting thing about adding to WrappedTask is that we do have more flexibility from an API POV. Should we use Futures, if people start using them, we'd have to fully break the API to change the available methods. Alternatively, we could have our own Futures with the most barebones thenRun(Runnable) for the moment while allowing for non-breaking expansion later.

[CJCrafter]

probably use thenCompose... but yeah. Returning your own future class, imo, is more confusing. I'd rather have all methods return a WrappedTask so that they can call thenRunAsync and whatnot (no confusion).

The issue with this method is that it is a big change and is missing some obvious things, like callbacks onto an object. (e.g. load file async, act on the contents sync)

[CJCrafter]

(and anyway, you already have a couple futures in your base code... that is a breaking change)

[TechnicallyCoded]

👍

[TechnicallyCoded]

I will merge this onto main once a few of my other changes from dev are well tested

[CJCrafter]

If you publish a -SNAPSHOT build, I can run some tests easier as well