Make Max Base, Roll Crit and Max Base + Max crit more intuitive

[ChemDragon]

I noticed that two of the settings for crits don't calculate properly, and I honestly can't even figure out at all how they're calculating to hazard a guess as to what input is wrong.

Both the "Max Base Damage & Max Critical Damage" and "Max Base Damage, Roll Critical Damage" output numbers that don't make sense.

Max both does show its maximizing the crit dice, but then seems to add a random number (or sometimes none)

link to max base, roll critical https://imgur.com/Kh0Y2t8 link to max base & max critical https://imgur.com/a/Rw7SKkP

As you can see, I rolled 3 times under both settings and the numbers don't line up in either. the max base & max crit should be the same number every time but isn't (and even added 0 once)

Edit: While I forgot to put it in the screenshots, I also tested rolling 20s naturally instead of pressing the forced critical and it results in the same random numbers.

[CarlosFdez]

This will definitely need a rewrite of some sort. They aren't bugged, but they are unintuitive.

The reason is that the module was designed originally for the dual roll d20 mode, which is still the default roll mode. When that happens, you might get erroneous crits that might not be correct, and you need to be able to choose to apply either non crit (the first number) or both (the sum). So what's happening is that the maximization is being moved TO the crit number so that when you apply crit damage, the sum is correct.

EX: 1d8+3 rolled a 5. That's 3 off from an 8. So the crit has 3 extra to compensate for that.

It should instead say 5 + (3 + 8) Crit or something like that but that's a feature and would need a partial rewrite to the storage of crit roll data.

[CarlosFdez]

Updated the issue name since while there isn't a bug, it definitely is not intuitive and could use some TLC.

[laclark93]

I was looking into this and was wondering if you could move the assigning of the baseRoll in updateCritStatus to a new method for GetBaseRoll where you do a similar thing to getCritRoll where you set the base roll depending on the setting assigned. You would then need to reconfigure getCritRoll but it would be a lot easier and straight forward. Is there a reason why this hasn't been done or would not work?

[CarlosFdez]

Not particularly, crits are the way there are since it was an incremental refactor of the old codebase which used to do it inline. That said, updateCritStatus() is for upgrading to a crit for roll editing, the originally assignment of the crit is in fields.js (in constructDamageRoll())

Rather than a new method, the ideal would be a new method (or object like a crit handler of sorts) that returns both the new baseRoll and new critRoll. Each crit type would be a different method or registered handler or what have you that the method then delegates to. Then you'd handle it by doing const { baseRoll, critRoll } = Utils.applyCrit(roll, { settings, extraCritDice })