Skip to content

Comments

Allow modification of options.keep for pf2e.preReroll and pf2e.reroll#21495

Open
7H3LaughingMan wants to merge 1 commit intofoundryvtt:v13-devfrom
7H3LaughingMan:reroll-keep
Open

Allow modification of options.keep for pf2e.preReroll and pf2e.reroll#21495
7H3LaughingMan wants to merge 1 commit intofoundryvtt:v13-devfrom
7H3LaughingMan:reroll-keep

Conversation

@7H3LaughingMan
Copy link
Contributor

@stwlam
Copy link
Collaborator

stwlam commented Feb 11, 2026

Isn't this in the roll?

@7H3LaughingMan
Copy link
Contributor Author

Isn't this in the roll?

The system uses options.keep from the parameters of rerollFromMessage to determine which roll to keep, the problem is that since it just passes it as options.keep it's a primitive type so Javascript is just passing around a copy of the data instead of a reference which means it can't be modified.

@stwlam
Copy link
Collaborator

stwlam commented Feb 11, 2026

roll.options.keep

@7H3LaughingMan
Copy link
Contributor Author

The system doesn't use that to determine which roll to keep, it uses options.keep from the parameters.

// Check if we should keep the old roll instead.
if (
(options.keep === "higher" && oldRoll.total > newRoll.total) ||
(options.keep === "lower" && oldRoll.total < newRoll.total)
) {
// If so, switch the css classes and keep the old roll.
[oldRollClass, newRollClass] = [newRollClass, oldRollClass];
keptRoll = oldRoll;
}

@stwlam
Copy link
Collaborator

stwlam commented Feb 12, 2026

I don't think we can do this until system version 8. To the extent the system has an API at all, this is part of it.

@7H3LaughingMan
Copy link
Contributor Author

I mean, this works the same way as how it currently works with unevaluatedNewRoll and newRoll which can be modified at the moment since the object itself is being passed. Modules are already using these hooks to modify the new roll.

@CarlosFdez
Copy link
Collaborator

CarlosFdez commented Feb 19, 2026

Any module listening to the hook would be receiving an object instead of a string now, so it would be a breaking change. I think this is a good idea but I agree that it should wait until system version 8 (which is when everything breaks anyways).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pass keep as an object to the hooks pf2e.preReroll and pf2e.reroll so it can be modified

3 participants