Add an asynchronous version of the dice and rolling APIs #4135

Closed aaclayton closed 3 years ago

Originally in GitLab by @zeel02

Implementation Notes

Pass an async parameter to Roll#evaluate to determine which flavor of roll you want.
If the roll is evaluated with minimize or maximize options, it is synchronous by default.

Roll evaluation argument

const r1 = new Roll("2d4");
r1.evaluate(); // Sync by default, displays a console warning
const r2 = new Roll("2d4");
r2.evaluate({async: false}); // Synchronous, no warning
const r3 = new Roll("2d4");
r3.evaluate({async: true}); // Asynchronous, no warning

Module implementation example

In order for a module to actually implement an asynchronous roll, the starting place would be to define/override the DiceTerm.prototype._evaluate (async) method, for example:

DiceTerm.prototype._evaluate = async function() {
  const rolls = await fetch(MY_ROLL_API, {
    method: "POST",
    headers: {
      "Content-Type": "application/json",
    },
    body: JSON.stringify(this.toJSON())
  });
  for ( let n=1; n <= this.number; n++ ) {
    this.results.push({result: rolls[n], active: true});
  }
  this._evaluateModifiers();
  return this;
};

Out of Scope (for now)

The following features were mentioned in the original request, but have been deemed out of scope for 0.8.1. The plan should be to evaluate the functionality added by 0.8.1, determine whether these features are still necessary, and generate a sequence of requests for 0.8.2+ to fill remaining gaps:

A flag to designate that a Roll is intended for "internal" use and should not call an external API. This is left to modules to handle however they see fit (at least for now).
Asynchronous evaluation of dice roll and dice pool modifiers which result in generating follow-up rolls. Things like exploding dice and automatic rerolls are only handled synchronously for now. The code becomes far too complex to manage both sync and async use cases for these modifiers so this will need to wait until async becomes standard in 0.10.x
Additional hooks (or async hooks) to interject at various phases of the dice rolling workflow

Original Request (Below)

Feature Summary

Add additional methods to the Roll, DiceTerm, and DicePool classes that are async allowing for modules to implement features which delay the return of rolls, without needing to block execution.

In addition, the evaluate() methods should all contain appropriate hooks, allowing for modules to more easily intercept and modify rolls without needing to perform heavy modification. To support this, a new type of Hook which functions asynchronously should be introduced, as discussed later.

Finally, a distinction should be made between rolls made for "internal" or API purposes, and those made by the user directly. Allowing modules to selectively manipulate only rolls that fit into one category, or handle the two categories differently in some way.

User Experience

From the user standpoint, this change allows for modules that currently could not exist without severely impairing the user experience. Anything that delays the current, synchronous, rolling system would lead to freeze ups, socket disconnects, etc. For this reason, such modules are rarely developed, and those that have been tested have not seen wide release.

With this change, such modules can function quite effectively, and can improve the user experience greatly.

Examples

The following are module ideas, some already partially developed, but all of great interest for future development, which would benefit from this addition but are currently not feasible:

Dice terms that require additional user input, similar to rolls on competing platforms like 1d20 + 3 + ?{Bless|Yes,1d4|No,0}.
Requesting randomness from an external randomness API (random.org, etc)
Requesting randomness from a hardware device
Obtain results from another application working in tandem with Foundry
Requesting that the user provide the results of a roll manually (either by rolling the dice themselves, or maybe the GM uses some other method to determine the results)
Obtain results from a computer vision API which reads real life dice
Roll 3D dice in a physics simulation
Delay the result of a roll for effect, without needing to "hack" the system by visually hiding the result.

Some of these may have "work arounds" but as a collection they can all be most easily solved through this addition to the rolling API. An async way to roll dice solves them all elegantly, as the async/Promise APIs exist for this very purpose.

API Additions

The following is a (possibly non-exhaustive) list of API changes that would be needed:

Roll#evaluateAsync & Roll#rollAsync

Methods of the Roll class which evaluate the roll asynchronously, these are the core pieces of the puzzle.

DiceTerm#evaluateAsync & DiceTerm#rollAsync

Methods of the DiceTerm class which evaluate the dice term asynchronously for use in the above.

DicePool#evaluateAsync

The new async rolls can be consumed by async Dice Pools.

Internal/API Roll Flag

The above (as well as standard rolls) should all gain an additional parameter "api" or "internal" or something that implies this purpose: When true, it is assumed that the roll should not require user input, and should return relatively quickly, but when false it is assumed that the roll may require user input, and may not return for an indefinite time.

An example would be a module that automates many things all at once, such as "mook" movement, environmental hazards, etc. These sort of modules may need to make many rolls all at once. It would therefore being impractical for the GM to actually roll those dice, so this module would use the internal flag to denote that its rolls should not be user-facing. Then if there is a module that prompts the GM for input, it will skip this roll. But a module that simply sends rolls to an external API would not skip these rolls.

Another example would be a rollable table with a non-standard die type, which would be impossible for the user to roll, but would pose no issue for alternative RNG/PRNGs.

A Question of Default Value

The default value for this flag is somewhat debatable. On one hand, if the default is "true" then this may lead to some module authors being a bit "lazy" and leaving the value as default, resulting in rolls that should not be internal being hidden from the user. On the other hand, "internal" rolls are often more common. As such it may be preferable that the more common use case be the default, to avoid situations where a module spams virtual 3D dice rolls, or requests an unending string of rolls from the GM.

Hooks#callAsync

A Hook method that calls the registered hooks, but awaits them all. The implementation should be very similar to the current call method, with the slight variation that it awaits either each call one at a time, or perhaps await Promise.all().

Additional Hooks

preEvaluateRoll: Hook to Roll#evaluate which passes the roll formula, terms, etc. to allow modification. The hook should be able to either return a value, or make modification to one of the parameters to feed a result back to the calling function. In this case, the returned result should be used, and the standard RNG behaviour should be skipped.
evaluateRoll: Hook to Roll#evaluate which passes the parameters of the roll as above, along with the result generated by the RNG.
preEvaluateRollAsync: Hook to Roll#evaluateAsync with an identical signature to the above except that it expects a function that returns a Promise.
evaluateRollAsync: As above.

Deprecation

In order to encourage developers to convert to async rolls, it would be advisable to include deprecation notice warnings in the standard Roll methods. All uses of Roll within Core code should be converted to the async variant. Major systems should be encouraged to convert as well, so that eventually the synchronous methods could be phased out. This deprecation process likely should span multiple major versions to allow sufficient time for updates. For example, if this feature were added in 0.8.x, older methods should remain until at least 0.9.x, or simply remain for legacy purposes indefinitely with an appropriate warning so new developers can be directed to prefer the async option.

Alternative Approach

Adding to the API is one possible option, it leads to the fewest breaking changes, but allows for a number of new possibilities. However, there are some downsides to this approach. Since this becomes an "optional" feature, its adoption relies heavily on its use in Systems and existing modules.

An alternative option which would lead to far more breaks, but ultimately ensure adoption across the board, is to replace the existing Roll APIs with async methods. This would break... all systems, and many modules all at once. Though with enough advanced notice and time to update, the actual changes they would need to make are fairly minimal.

Priority/Importance

This addition would be extremely useful for realizing certain workflows and features that many developers and users would like to see. If this proposal is to be considered at all, I would rank the priority fairly high - the sooner a change like this is made, the fewer side effects. The more modules there are, the more systems, the more collective work there is to adapt to changes. Especially the alternative option discussed above, changes of this magnitude are better sooner than later.

Originally in GitLab by @kayhos

mentioned in issue fvtt-modiphius/foundryvtt-conan2d20#216

Originally in GitLab by @zeel02

That was fast.

It's a good comment.

Request granted: https://gitlab.com/foundrynet/foundryvtt/-/issues/4779

Originally in GitLab by @Norc

My (ignorant, secondhand) understanding was that even wrapped properly it wasn't true asynchronous handling.

If this is not the case, yes, please do carry on.

Originally in GitLab by @zeel02

@Norc You can already use async in macros with a bit of extra syntax:

(async () => {
  // Macro code with awaits
})()

But it would be really nice if macros were simply always invoke as async. Since they don't have a return value, there really isn't a reason I can think of to keep them synchronous.

That's probably something a separate issue should be opened for though.

Originally in GitLab by @Norc

My only feedback is that async support for macros should ideally be put in place at the same time the breaking change is deployed, whenever that is. This would preserve the ability to call Roll Tables from a macro.

(Please forgive any technical naivete if this is not actually a concern).

marked this issue as related to #4778

I need some feedback on async rolls as relate to Roll Tables. Currently RollTable#draw and RollTable#drawMany (which rolls on the roll table AND does other things like mark the results as drawn and display the result to chat) are both already async. However, RollTable#roll which is the internal mechanism by which the randomization occurs is synchronous.

My assessment is that the logical hoops needed to support both sync and async versions of Roll Table drawing is going to be infeasible for me to implement. With regards to roll tables specifically, that prompts a choice.

Move Roll Tables to async right away causing a breaking change for anyone who calls RollTable#roll directly (rather than RollTable#draw or RollTable#drawMany)
Keep RollTable#roll synchronous for now until async rolling becomes the default behavior at which point make a breaking change (0.9.x)
Keep RollTable#roll synchronous for now until async rolling becomes the mandatory behavior at which point make a breaking change (0.10.x)

My own inclination (as with almost every decision of this type) is just go ahead and break it immediately and don't bother with complicated and drawn out migration paths. I don't know who would be impacted by this - I know some game systems like SWADE or FATE might be.

Implementation Notes

Roll evaluation argument

const r1 = new Roll("2d4");
r1.evaluate(); // Sync by default, displays a console warning
const r2 = new Roll("2d4");
r2.evaluate({async: false}); // Synchronous, no warning
const r3 = new Roll("2d4");
r3.evaluate({async: true}); // Asynchronous, no warning

Module implementation example

In order for a module to actually implement an asynchronous roll, the starting place would be to define/override the DiceTerm.prototype._evaluate (async) method, for example:

DiceTerm.prototype._evaluate = async function() {
  const rolls = await fetch(MY_ROLL_API, {
    method: "POST",
    headers: {
      "Content-Type": "application/json",
    },
    body: JSON.stringify(this.toJSON())
  });
  for ( let n=1; n <= this.number; n++ ) {
    this.results.push({result: rolls[n], active: true});
  }
  this._evaluateModifiers();
  return this;
};

Out of Scope (for now)

The following features were mentioned in the original request, but have been deemed out of scope for 0.8.1. The plan should be to evaluate the functionality added by 0.8.1, determine whether these features are still necessary, and generate a sequence of requests for 0.8.2+ to fill remaining gaps:

A flag to designate that a Roll is intended for "internal" use and should not call an external API. This is left to modules to handle however they see fit (at least for now).
Additional hooks to interject at various phases of the dice rolling workflow
Async hooks

Bumping to 0.8.1 to reduce the overhead in initial 0.8.0 update.

Originally in GitLab by @gatesvp

Another +1 vote here for just paying this down. It's only going to get worse as more people build on top of it and it's an essential service that needs to be done right. It's only going to get harder if we wait more. And spreading it out over 3 major versions will give people ample time to update their systems, modules and macros.

It would have been a smart choice, in hindsight to make roll evaluation async. The only concern/resistance I have had to the idea of making changes there is the scope of impact that further API changes to the Roll system will have for systems and modules, especially given that Roll API changes aren't intended to be a major theme for 0.8.x (they were for 0.7.x, which would have been a better time to do this).

The dev community has proposed an approach that - while somewhat unsavory to me - will provide a path to get us to async rolls without immediately breaking what anyone is doing and allowing for a period of transition. My hesitation isn't over the difficulty of making the change, only over the potential ramifications of it for existing modules. Amusingly, this conversation is usually reversed where I am refactoring systems for the purpose of enabling them to achieve a better underlying design against the wishes of the dev community for stability.

All that said - there is clearly a strong desire for this change - at least within a set of vocal developers so I will be implementing an option for async roll evaluation in 0.8.0. I'll be following the recommendation of adding an optional argument which returns a promise from Roll#evaluate with a deprecation/migration plan and set of warnings to get users to shift over towards expecting promises from this function by the time we get to 0.9.x release with support for the old synchronous evaluation removed in 0.10.x.

Hopefully this clarification around what the plan is will avoid some unnecessary argument about what should be done and help the community focus on other areas where constructive discussion is more useful.

Originally in GitLab by @zeel02

That's one strategy, yes.

However we actually came up with another option that's similar, but I think a bit smoother in execution. Here we leverage Javascipt's lack of strong typing as a strength:

roll({async=false}={}) {
  if (async) return this._rollAsync(); // Private or inlined, per preference
  // old sync code
}

In this strategy, the method returns a Roll when the optional parameter async is false, or it returns a Promise<Roll> if async: true.

0.8.x: The default value of this parameter is false, so any existing code which omits this parameter gets the standard synchronous version. Using this however prompts a warning. Developers that need additional time upgrading can set async: false explicitly. It's easy to do, and ensures they have more time for...
0.9.x: The default is changed to true. Anyone still using async: false is good to go for another full version. And anyone already upgraded to async: true can remove the parameter, or leave it since it hurts nothing. The warning should now also be a toast notification.
0.10.x: Deprecate the parameter. Explicitly setting async: true still has no ill effect, since the code simply ignores the parameter now. However async: false is also ignored, but should no longer exist in any active code base.

The beauty of this approach, is that for a full version, upgrading is optional. For two full versions, complete upgrading can be postponed if necessary. However anyone who does upgrade only needs to do so once. Even if they leave async: true in their code for the rest of time, it won't hurt anything, it would just be clutter.

Additionally, at the 0.10.x point the sync code can also simply be removed.

This option was met with pretty enthusiastic approval among those I discussed it with.

If we went down the path of adding Roll#rollAsync(), I imagine the timeline would look something like this:

In 0.8.x we would have async rollAsync() and roll(). The roll() method would throw a warning that rolls will become async in 0.9.x and encourage you to temporarily migrate to call async rollAsync().

In 0.9.x, we would make async roll() the default behavior and rollAsync() would begin to throw a deprecation warning that you should move back to roll() because rollAsync() was a temporary function that will be deprecated.

In 0.10.x we would deprecate rollAsync() and we would only have async roll().

Is that what people have in mind? Or am I misunderstanding the proposal?

Originally in GitLab by @zeel02

I should probably preface this with something I maybe should have included in the main post: This isn't something I just wrote up by myself, we workshopped this between half a dozen developers in order to present a proposal we all felt comfortable with, that would provide the needed functionality, but cause the least trouble. This is something that as a group we concluded meets the needs of the developer community as best as possible.

If we make this change, I think [alternative option] is the right way to do it. Adding a Roll#rollAsync() method that doesn't actually get used because systems like dnd5e still call Roll#roll() is pretty useless and would prevent actually doing the things that devs want to do with asynchronous rolls.

The way I see it is this: There is going to be a lot of work to do if async rolls are added. One way or another, systems including 5e (especially 5e, as it stands as an example that other systems and modules emulate) are going to have to make changes. The question then is whether or not those changes are mandatory or just recommended. Whether those changes mean that systems and modules simply stop working, or if there is a (preferably nice and long) transitional period.

I fully understand wanting to just break it and remake it. I like the idea of not duplicating methods, not being stuck with xAsync named methods, and ensuring that all systems and modules support new features. In fact, a few days ago in the #developers-chat Discord channel I was very much a proponent of just overhauling it and dealing with the fallout. But in listening to the many other developers expressing their concerns, talking it over, and working on this proposal... I've changed my mind. In a perfect world, it would be cool to just change it all, let everyone know, and it all get's patched up before the first public release. But I don't think that's likely to work out.

For me, it comes down to a question of whether the developer community is in agreement to band together and help each other to migrate all rolls to deal with a significant and immediately breaking change...

Honestly, I think that's something we are capable of. But that means we are also capable of converting systems and modules for a non-breaking change, and we can do that more effectively. The biggest risk in the augmentation approach vs. the replacement approach is the risk of people not using the new feature. But I really don't think that's a horribly large risk. As the most popular systems (5e) and modules (DSN) adapt to this new feature, other modules will want to ensure compatibility with those systems, and systems will want to be compatible with those modules. Migration will happen, I think as a community we can do a lot to make it happen smoothly.

We have a lot of very dedicated devs here, and I think we are all devoted to making our modules and systems the best they can be. We all want new features, we want to keep up with the latest. So I'm not really worried about "Adding a Roll#rollAsync() method that doesn't actually get used" because it will. It might take a little time, it might take some coaching, but it will happen, I'm sure of that.

Originally in GitLab by @troygoode

Macro scripts can use async functions, they just currently need to be wrapped in an IIFE. This is how mine work:

(async () => {
  await doTheThing();
})();

It'd be great to see Foundry VTT automatically wrap the macros in an IIFE under-the-hood, though macro owners would still need to manually go in and prepend await to the roll calls - so it isn't zero work for sure.

Edit: Forgot to add that I am "+1" to the break-all-the-things path here. FVTT is already fast moving and the module ecosystem is (a) quite new and (b) modules already go out of date quickly given the many platform changes. This is a foundational change. Rip the band-aid off earlier rather than later. For me the most exciting thing about FVTT is how quickly it has evolved/improved compared to the notoriously glacial pace of its two primary competitors; there will be short-term pain, but it is worth it to keep the momentum going.

Originally in GitLab by @JiDW

One thing came to the discussion while debating this change in the League: Roll macros.

Currently, macros can't use async function as they use eval.

Making a breaking change here would mean that every macro break (horrible for users).
And we also need to make macros able to use async anyway (Furnace does it, but we need this in the Core).

Originally in GitLab by @kakaroto

For me this is the important part :

If this feature were added in 0.8.x, older methods should remain until at least 0.9.x, or simply remain for legacy purposes indefinitely with an appropriate warning so new developers can be directed to prefer the async option.

And I also disagree with the alternative approach which would break everything. Ideally, there should never be API breakage, but IF a breaking change is being planned today, it would have to be for 0.9.x, not for 0.8.x. And the deprecations and documentations about the break would be released in 0.8.x. That's the only way to ensure things aren't breaking for everyone with no proper advance notice.

Originally in GitLab by @rughalt

I am also in favor of having both as this seems a good way to not break everything while providing functionality for systems or modules that need it and allowing a smooth transition.

In my particular case, as my system does hundreds of rolls that will never need async, this change would definitely force a rewrite or at least huge refactor.

Originally in GitLab by @s17b1_rabinovich

If we want to make rolls async but don't want to break everything, why not introduce rollAsync alongside the normal roll, but add a message to roll, saying saying that it's deprecated and will be removed in the next major update (0.9.0 for example). This way nothing would break for now but people will know to use the async roll

Originally in GitLab by @JiDW

I do not think it would be useless to have both at the same time. We've seen a lot of systems chose to make their code compatible with different modules already. Adding async (instead of replacing) would let devs decide if and when to support modules that need this feature to work.

If we think devs would not bother to change their code to support new and fancy modules, I do not think that making it mandatory would be a wise choice.

I'm strongly in favor of doing everything possible to lessen the burden on volunteer devs by reducing the number of breaking changes when we can. This can only improve the overall quality of the FVTT scene and facilitate the adoption of the newer FVTT core releases.

Thanks for the through write-up @zeel02. My 2 cents is this:

An alternative option which would lead to far more breaks, but ultimately ensure adoption across the board, is to replace the existing Roll APIs with async methods. This would break... all systems, and many modules all at once. Though with enough advanced notice and time to update, the actual changes they would need to make are fairly minimal.

If we make this change, I think this is the right way to do it. Adding a Roll#rollAsync() method that doesn't actually get used because systems like dnd5e still call Roll#roll() is pretty useless and would prevent actually doing the things that devs want to do with asynchronous rolls.

For me, it comes down to a question of whether the developer community is in agreement to band together and help each other to migrate all rolls to deal with a significant and immediately breaking change that would arrive in 0.8.x (or whenever else we do this). As you mention, changing roll() to return a Promise would break pretty much all game systems and many modules. It would reflect really badly on me as the core developer if this happened, so I'm only willing to consider this issue if the dev community is almost unanimously in agreement and behind this change.

It has finally arrived! The issue that was foretold!

foundryvtt / foundryvtt