Allow for searching nested dice #31
Conversation
fixes damage rolls in PF2e not being triggered. foundryvtt#28 Adds an optional configuration to search for nested dice. With it is turned on, for each `roll` we’ll not only look if the term is a `Die` but also see if the non-die term answers to `term.dice`, in which case we’ll recursively look for the dice in there. This solves issues in the cases where we have special Roll types that don’t conform usual pattern of a straight formula. An example is a damage roll in Pathfinder 2e (or spell damage, or crit…) where without this option enabled, `unfulfilled-rolls` will not be able to act on them. Also adding `.history` to gitignore to avoid issues with VS Code history plugin.
eturino
force-pushed
the
nested-dice
branch
from
d2be613
to
5fe2f27
Compare
scripts/fulfillable-roll.mjs
Outdated
| * @param {RollTerm[]} terms Terms of the Roll instance | ||
| * @param {object} config The unfulfilled-rolls.diceSettings configuration | ||
| * @returns {FulfillableTerm[]} An array of identified terms | ||
| * @private | ||
| */ | ||
| function _identifyFulfillableTerms(terms, config) { | ||
| const toFulfill = []; | ||
| for ( const [i, term] of terms.entries() ) { | ||
| const diceTerms = terms.flatMap(t => _diceFromRollTerm(t, !!config.searchNestedDice)); |
There was a problem hiding this comment.
The idea here is right, we need to iterate over dice terms and recursively handle terms which contain inner dice.
This implementation fully iterates first, however, to create const diceTerms rather than recursively evaluating terms as we go. This results in us iterating over all terms twice, instead of just once.
A better way to do this will be something like this:
function function _identifyFulfillableTerms(terms, config, toFulfill=[]) {
for ( const term of diceTerms ) {
if ( !(term instanceof Die) ) continue;
if ( this._fulfilled?.length > 0 ) continue; // already fulfilled
const method = config[`d${term.faces}`];
if ( !method || (method === "fvtt") ) continue;
toFulfill.push({term, method, terms, index: toFullfill.length-1});
// Recursively identify inner terms
if ( "dice" in term ) {
_identifyFulfillableTerms(term.dice, config, toFulfill);
}
}
return toFulfill;
}There was a problem hiding this comment.
happy to change that.
There was a problem hiding this comment.
I'd like to avoid modifying the interface of this function though, I think it'll probably be better to make the extraction inside, avoiding exposing toFulfill as an argument. I'll change the PR.
There was a problem hiding this comment.
It's a private function, change whatever you need to change to have the cleanest implementation.
templates/dice-config.hbs
Outdated
| @@ -18,6 +18,14 @@ | |||
| {{/each}} | |||
| </section> | |||
|
|
|||
| <section> | |||
| <div class="form-group"> | |||
There was a problem hiding this comment.
I'm not sure this should be a setting. Feels to me like this should just be the default behavior. Do you have use cases in mind where the user would not want to identify inner terms as fulfillable?
There was a problem hiding this comment.
To be honest, I added this as an opt-in so that the exceptional behaviour would be executed only in pf2e and potentially other cases, to prevent from potential issues in other systems that I had not encountered. If you are happy to make this the default behaviour, I'll gladly make it so.
|
Thanks for the contribution @eturino. I have a few comments for you to consider! |
- avoid opt-in behaviour, make this the default behaviour all the time - avoid double iterations.
|
@aaclayton went with your suggestions, fixing some issues. Please have a look. By making the 2 internal private functions for identifyTerm() and for running the extraction, we could extend this in the future with new behaviour as needed. |
scripts/fulfillable-roll.mjs
Outdated
| if ( !method || (method === "fvtt") ) continue; | ||
| toFulfill.push({term, method, index: i}); | ||
| if ( method && (method !== "fvtt") ) { | ||
| fulfillableTerms.push({term, method, index}); |
There was a problem hiding this comment.
Code generally looks good, but ...
I think it's a problem that we would end up with multiple elements in fulfillableTerms with the same index. We need to create a unique name for the HTML element which allows us to identify which user input result belongs to which term. As currently designed we could end up with, for example, two different d6 terms each with index: 0 which would result in the same name.
I have not tested this, but I think you would replicate the issue with something like:
/roll 1d6 + {1d6, 1d6}kh + (1d6 + 3)
You would end up with 3 different d6 terms all with index: 0
Probably we should just use the actual index position in fulfillableTerms as the simplest solution. Do you understand what I am highlighting with this concern?"
There was a problem hiding this comment.
yes indeed. This wasn't an issue in my original approach because of the double loop. I'll fix in a bit.
There was a problem hiding this comment.
went with using as index the length of the array before pushing. That way we keep the index property of the fulfillable terms objects, and avoid depending on the order of the array for identification. I can see a future where we would want to allow the user to opt-in to sort the dice by type when exposed to the user for example (all d10s together, then all d12s…), which would mean reordering the elements of the array. Better to not depend on that order for identification.
less changes than removing the index altogether and relying on the order of the array
fixes damage rolls in PF2e not being triggered. #28
For each
rollwe’ll not only look if the term is aDiebut also see if the non-die term answers toterm.dice, in which case we’ll recursively look for the dice in there.This solves issues in the cases where we have special Roll types that don’t conform usual pattern of a straight formula.
An example is a damage roll in Pathfinder 2e (or spell damage, or crit…) where without this option enabled,
unfulfilled-rollswill not be able to act on them.Also adding
.historyto gitignore to avoid issues with VS Code history plugin.