Use the global references to the Roll classes instead of the imported ones

[aaclayton]

Originally in GitLab by @Moerill

While i do like the new DamageRoll and D20Rollclasses, they make it more difficult to modify rolling behaviour for modules without having to use prototype patching.

What i've been doing previously

I created my own Rollclass Mars5eRolland replaced the global reference to the original Rollclass with my own one and everything worked fine without resulting to prototype patching. While still being a bit hacky (by replacing the global reference to a class), i found this solution to be cleaner and more easily maintainable.

The new issue

The introduction of the DamageRoll and D20Roll classes in theory allow for better moddability and modification of the rolling behaviour. In reality though a module can't intercept the usage of those classes and inject a custom class, since all references i found in the code only reference the imported classes, not the global class references in game.dnd5e.dice. So currently one would have to rely on hacky prototype patching, since this is the only way of modifying the behaviour of these classes.

My proposed solution

Instead of using imported references to the DamageRoll and D20Rollclasses, use the global references in game.dnd5e.dice. These references do already exist, but are currently not used.

This way i (or other modders wanting to modify this) could do smth like this and be done:

Hooks.on('init', () => {
  class Mars5eDamageRoll extends game.dnd5e.dice.DamageRoll {
     // Do whatever customizations i desire
  }
  game.dnd5e.dice.DamageRoll = Mars5eDamageRoll;
});

If this proposal gets accepted i'd of course be willing to put this "Find and Replace" in a quick MR, with any changes proposed in a possible discussion here!

[aaclayton]

I think in general the proposal is a good one, but I don't want the process for modules to override the class to be changing what is exposed within game.dnd5e - which should always provide references to the original classes in case they are needed.

Why don't we use a similar approach that is used for other classes of assigning something to the global CONFIG object instead. Perhaps something like CONFIG.Dice.d20Roll = D20Roll and CONFIG.Dice.damageRoll = DamageRoll and then reference the configured implementations there instead?

[aaclayton]

Originally in GitLab by @Moerill

This is a good change in the proposal and does make sense!

Should i go and create a MR as soon as i find some time?

[aaclayton]

Please feel free!

[aaclayton]

Originally in GitLab by @Moerill

Thanks for the quick reactions before ! :)

[aaclayton]

mentioned in commit 065e9c9

[aaclayton]

mentioned in commit 37b404c