Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Roll formula display #140

Merged
merged 11 commits into from
Jan 11, 2022
Merged

Conversation

draconas1
Copy link
Collaborator

This ended up a lot bigger than I originally intended. Sorry

Overall goal: The ability to show the original formula expression that went into a roll. To answer the oft asked question at my game table "where the hell did those numbers come from?" and "are you /sure/ it works like that"?

Secondary goal: refactor the d20Rolland rollDamage scripts to make them easier to maintain.

Big, file, level changes that are hard to diff:

  1. MultiAttackRoll has been moved into its own .js file
  2. A new Roll call: RollWithOriginalExpression has been created, with its own .js file
  3. The inner functions inside d20Rolland damageRoll in dice.js have been moved out to full functions for readability, now d20Roll and damageRoll are entirely about setting up the dialog boxes that pop up, and the dialogs call out to separate methods that handle doing all the work.

As part of the work of refactoring the 2 inner expressions I was able to simplify a lot of the shared code between them into helper methods to cut down on duplication. Most significantly for d20Roll, I was able to remove all the branching code about whether it was a single or multi-target roll which cut out a huge amount of near-duplicate logic. Everything is now a multi-target roll, it's just that it may only have 1 target and they are treated the same until it gets to the end where it picks whether to stick witht he multi-roll class or extract the single inner roll depending on how many rolls there were.

In order to be able to display the formula expression and make individual term highlighting work, I needed to pass some additional information through from roll attack, roll damage and roll healing in entity.js.
This ended up in 2 parts, to work with the parts of the formula that are already replaced by helper.CommonReplace before being passed to dice.d20Roll() as the first element of the parts[] array.

The first is known as partsExpressionReplacement and is an array of what the original formula was for a given element in the parts[] - so it converts back from the attack formula to the formula of "@wepAttack + @abilityMod + @lvlhf", this was essential to even display a meaningful formula as this information has already been lost from the parts array[] and with this I can operate highlighting in the chat formula on the granularity of the parts[] that drives the formula.

The second is another data object that has the values of those @variables that were filled in by commonReplace. I get this by pulling a new mod on commonReplace that returns the data object of what it would have done rather than a replaced string. This allows me to get the highlighting to the level of granularity of the individual @variables in the formula.

Also included some small changes, like adding the selected weapon to the popout title and chat flavour when makin a weapon roll. Including the damage types of secondary damage.

This ended up being pretty big, so always happy to talk about it on discord or in comments here.

…the roll (e.g. to answer the "where did that value come from") for attacks and damages

As part of doing this:
-Needed a new Roll object, overriding several methods, because adjusting data that goes to the Roll display is unnecessarily complicated in Foundry.

-Rewrote the inner _roll closure and moved it to be its own method - it had 2 very similar codepaths and the same variable being altered at several points (@bonus).  I consolidated this down to 1 codepath - everything is a multiroll, it's just that it might be a multiroll of 1 roll.  At the end, if it's a single roll we just get that single roll out of the multiroll.
Net result is it's about 150 lines shorter with several fewer branches.

-also removed some 5th edition holdovers that were not being used (reliable only edited flavour text):
--Halfling luck
--elven accuracy
--Reliable talent (it's a property on powers in 4E that does not work the same as 5th)
--Advantage and disadvantage

Similar for power damage, though that was made more complicated by hit, miss and crit, but was broadly the same.

I have not touched healing rolls as they confuse me, see issue EndlesNights#129
… helper methods, but things only done in 1 place now.

Also put a setting in around formula display
rollAttack and rollDamage now send more parts data down for their formula

Roll expression can unwind the expression to highlight individual parts.

Added "using <weapon>" as chat flavour for weapon rolls

Moved the 2 dice into their own files, as they were getting big.
@drywallmud
Copy link

This sounds like a ton of work, but this will be extremely useful, especially when teaching new players or when a PC is sure they're shorted a +1, which is always. I can't wait to see this rolled out.

Thanks so much for this.

This is is a bit of a hack solution, but it fixes a messy interaction between how class MultiAttackRoll Class relates back to the Roll Class, where how it is set up an extra hidden roll is taking place and that is being placed MultiAttackRoll.terms. The Dice So Nice module then only picks up this extra roll, and displays that with the 3d dice, so users are left with only this singular unrelated dice roll instead of all the other dice they were expecting to have rolled. 
This also fixes part of an issue with chat.js highlightCriticalSuccessFailure, which again would only look at this extra roll when determining to highlight for critical success or failure. So the entire multi roll log would be highlighted red or green at random internals.
@EndlesNights
Copy link
Owner

Just put in a small commit that fixes some wonky interactions.
It is a bit of a hack solution, but it fixes a messy interaction between how class MultiAttackRoll Class relates back to the Roll Class, where how it is set up an extra hidden roll is taking place and that is being placed MultiAttackRoll.terms. The Dice So Nice module then only picks up this extra roll, and displays that with the 3d dice, so users are left with only this singular unrelated dice roll instead of all the other dice they were expecting to have rolled.
This also fixes part of an issue with chat.js highlightCriticalSuccessFailure, which again would only look at this extra roll when determining to highlight for critical success or failure. So the entire multi roll log would be highlighted red or green at random internals.

Anyway thanks for reworking all of this! WOW what a great feature to have! And something I've been putting off because of the spaghetti mess that I had made.

@draconas1
Copy link
Collaborator Author

Awesome, sorry, should have considered 3rd party interactions which would only go off the fake roll data.

As I said to Albions-Angel, it's easy to fix the code when it's already there and you can see the whole picture. Want to see what mine is like first time - go look at my exporter code that was written as I went along as I learned how masterplan worked and stored its data and how Roll20/foundry wanted to receive its data.

Anyway I am going to take a break for a few days and will probably pick up a look over tool and item rolls while I have all the d20 roll stuff fresh in my head.

@EndlesNights
Copy link
Owner

Don't worry about not knowing about an interaction with another module. It's been a minor issue for a while that I've just never gotten around to fixing.
Enjoy your break! And don't forget to enjoy just playing the game!

@EndlesNights EndlesNights merged commit 734da0a into EndlesNights:dev Jan 11, 2022
@EndlesNights
Copy link
Owner

Bit late to this pull, but I'll add it in as it's own commit, I did a quick rework to how you where assigning the critical / fumble tags. And turns out some times we over complicate things, while missing the simple answers.

dice.js Line 221

		if (subroll.dice.some(obj => obj.results.some(obj => obj.result >= critical)) && !subroll.dice.some(obj => obj.results.some(obj => obj.result <= fumble))) {
			critStateArray.push(" critical");
		} else if (!subroll.dice.some(obj => obj.results.some(obj => obj.result >= critical)) && subroll.dice.some(obj => obj.results.some(obj => obj.result <= fumble))) {
			critStateArray.push(" fumble");
		} else {
			critStateArray.push("");
		}

was simplified to

		if (subroll.terms[0].total >= critical) {
			critStateArray.push(" critical");
		} else if (subroll.terms[0].total <= fumble) {
			critStateArray.push(" fumble");
		} else {
			critStateArray.push("");
		}

Since we only want to check every die being rolled, but only the d20 that is being counted. If you are rolling a 2d20kh, we only need to know the results of the higher of the two, and if one die rolls a 1, while the other does not, we don't want to accidently count that as a critical failure. And similarly, if in the situational field, if you where add in another die roll such as "1d4", we do not need to be checking this die to be determining if the roll is a critical success or failure.
Lucky for use the Foundry API does most of the heavy lifting here with Roll.terms.total.

Anyway, I want to say thanks again for doing ALLLL of the heavy lifting for this merge. It's super helpful!

@draconas1
Copy link
Collaborator Author

@EndlesNights Sweet!
That's useful stuff - to tell the truth those lines about criticals aren't mine - I copied them from the original. I read the comment and didn't have the strength to go down another rabbit hole of criticals and fumbles.

Thanks for showing me that, it's a lot simpler and more elegant :) I need to do a bit more reading up and experimenting with the dice and terms api and how it works.

@draconas1 draconas1 deleted the roll-formula-display branch January 25, 2022 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants