-
Notifications
You must be signed in to change notification settings - Fork 219
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
Add cached roll to direct rollInitiative call #2530
Conversation
It appears that rollInitiativeDialog creates a _cachedInitiativeRoll before calling rollInitiative which then assumes that a _cachedInitiativeRoll exists for the hook. This would return undefined to the dnd5e.preRollInitiative hook. (this doesn't cause errors in the initiative roll as foundry code calls getInitiativeRoll and it returns the cached roll if it exists or gets a new roll) To fix this issue: - I added a check to see if there is a _cachedInitiativeRoll in rollInitatiative and if not to call getInitiativeRoll to get a new roll and cache it. - I added a rollOptions to rollInitiative to allow for providing RollOptions to the getInitiativeRoll if the _cachedInitiativeRoll does not exist. - Moved the delete this._cachedInitiativeRoll from rollInitiativeDialog to rollInitiative as we will now always have a cached roll to delete and eliminates extra checks needed to determine if we need to delete _cachedInitiativeRoll depending on if it is set inside of rollInitiative or not. Closes foundryvtt#2468
module/documents/actor/actor.mjs
Outdated
// Set a _cachedInitiativeRoll if not called from rollInitiativeDialog. | ||
if ((this._cachedInitiativeRoll ?? undefined) === undefined) { | ||
this._cachedInitiativeRoll = this.getInitiativeRoll(rollOptions); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would read better if we create a local variable for it. The idea is that we used a cached roll if one exists, otherwise we create our own in this method.
// Set a _cachedInitiativeRoll if not called from rollInitiativeDialog. | |
if ((this._cachedInitiativeRoll ?? undefined) === undefined) { | |
this._cachedInitiativeRoll = this.getInitiativeRoll(rollOptions); | |
} | |
const roll = this._cachedInitiativeRoll ?? this.getInitiativeRoll(rollOptions); |
Then we need to pass roll
into the hook on line 1602 (1605 in this PR). We still delete this._cachedInitiativeRoll
at the end of the method. That looks right to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, Part of the issue with making a roll variable here and passing it to the hook and not setting this._cachedInitiativeRoll
is that await super.rollInitiative(options);
eventually calls getInitiativeRoll()
from foundry code, and we pass back the _cachedInitiativeRoll
if it exists, otherwise we create a new roll to pass to foundry. If we make this roll variable we don't have a way to pass it to the foundry code, and the roll that will be used for the initiative roll will be different from the one in the hook. This wouldn't be too big an issue as I could remove the rolloptions I added and call the getInitiativeRoll()
in such a way to make the roll the same way foundry does, however the hook would allow _cachedInitiativeRoll
to be modified before it is rolled, but not if we ended up making the roll in rollInitiative()
.
For example we could have a hook that always gives advantage on initiative rolls such as:
Hooks.on("dnd5e.preRollInitiative", function(actor, roll) {
roll.options.advantageMode = 1;
roll.configureModifiers()
});
This would roll _cachedInitiativeRoll
with advantage, but not the getInitiativeRoll()
from inside rollInitiative()
Two other ways this could be handled is moving the hook to getInitiativeRoll()
, however this could trigger the hook without necessarily rolling initiative. (Plus it looks like there was hesitancy about that in the PR adding this hook #2009)
Or creating a different property on actor that we pass back in getInitiativeRoll()
, but this feels like reinventing the wheel since we have a _cachedInitiativeRoll
and would only set it when that value does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if the issue is with the check I can simplify it a bit (I realized I don't need to do the undefined check as ?? already does null and undefined) to:
this._cachedInitiativeRoll ??= this.getInitiativeRoll(rollOptions);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I had missed that detail, that makes sense.
this._cachedInitiativeRoll ??= this.getInitiativeRoll(rollOptions);
Please go ahead and make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change has been made.
Simplify _cachedInitiativeRoll check as ?? handles both null and undefined.
It appears that rollInitiativeDialog creates a _cachedInitiativeRoll before calling rollInitiative which then assumes that a _cachedInitiativeRoll exists for the hook. This would return undefined to the dnd5e.preRollInitiative hook if called directly. (this doesn't cause errors in the initiative roll as foundry code calls getInitiativeRoll and it returns the cached roll if it exists or gets a new roll)
To fix this issue:
Closes "dnd5e.preRollInitiative" hook bug when programmatically rolling initiative #2468