-
Notifications
You must be signed in to change notification settings - Fork 232
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
[#2138] Add BasicRoll type as part of the roll refactor #3146
Conversation
2768579
to
9f286fe
Compare
Would it be possible to add a method on the application to programmatically push a new roll? For example: dialog.addRoll("2d8", "radiant"); which then causes it to rebuild. A way to achieve the same result as using the situational bonus field essentially. |
Rather than adding a specific dialog.config.rolls.push({ parts: ["1d6"], options: { type: "radiant" } });
dialog.rebuild(); |
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.
This looks really good, thank you Jeff. Please see my comments for consideration.
const rolls = this._finalizeRolls(event.submitter?.dataset?.action); | ||
await this.close({ dnd5e: { rolls } }); |
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.
We could avoid having to propagate these rolls via options
if we change the contract for _finalizeRolls
to expect subclasses to modify this.rolls
in-place, rather than returning new rolls. That would mean that after _finalizeRolls
is called, the dialog's this.rolls
is the final version of the rolls, which is perhaps useful.
const rolls = this._finalizeRolls(event.submitter?.dataset?.action); | |
await this.close({ dnd5e: { rolls } }); | |
this._finalizeRolls(event.submitter?.dataset?.action); | |
this.close(); |
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.
Yeah, I can update that. Those main reason I was using closing options here is to know that the closing occurred via the submission versus occurring through the close button so I can know what to send to the resolve
method.
Since the close
method already takes ApplicationClosingOptions
, it would be fantastic if core provided some more details indicating what triggered the closing.
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.
That is a good idea. Maybe we can add some properties to the close event.
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.
This looks really good, thank you Jeff. Please see my comments for consideration.
First part of refactoring the rolling system. This adds a new
BasicRoll
type that replaces core'sRoll
adding a number of methods to help with building one or more rolls, displaying a roll prompt, and posting a message to chat.Splits roll configuration into three configuration objects, one for the rolls, one for the prompt, and one for the chat message. This separation should make it clear what options control what part of the process and make it easy to introduce new options in the future or to subclasses without a single massive configuration object like is currently used for the
d20Roll
function.Adds a custom dialog application for roll prompts rather than just customizing the contents of a standard dialog to allow for more dynamic interface. Every time changes are made in the dialog it applies those changes to the roll configuration object and then re-creates the rolls from those configs. The default dialog class can be specified for each roll type and by individual calls to the
build
factory method, allowing very custom implementations for specific rolls (such as adding an ability dropdown only for skill rolls).Currently I've only converted the
Actor5e#rollHitDie
method to use this new system as an example, but thisBasicRoll
will also be used for things like rolling hit points and item recharges. The changes torollHitDie
include deprecation paths to ensure that existing use of that function or its hooks don't break, though this required adding a new version of the hooks so nothing breaks for people using the existing ones.Closes #1813