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

[#2138] Add BasicRoll type as part of the roll refactor #3146

Merged
merged 6 commits into from
Jul 25, 2024
Merged

Conversation

arbron
Copy link
Collaborator

@arbron arbron commented Feb 27, 2024

First part of refactoring the rolling system. This adds a new BasicRoll type that replaces core's Roll 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 this BasicRoll will also be used for things like rolling hit points and item recharges. The changes to rollHitDie 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

@arbron arbron added epic api system: dice Dice rolling functionality labels Feb 27, 2024
@arbron arbron self-assigned this Feb 27, 2024
@arbron arbron force-pushed the rolls/base branch 4 times, most recently from 2768579 to 9f286fe Compare February 29, 2024 21:53
@arbron arbron added this to the D&D5E 3.2.0 milestone Mar 25, 2024
@arbron arbron requested a review from Fyorl March 25, 2024 19:37
@arbron arbron changed the base branch from 3.1.x to 3.2.x March 25, 2024 19:58
@krbz999
Copy link
Contributor

krbz999 commented Mar 26, 2024

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.

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.

@arbron
Copy link
Collaborator Author

arbron commented Mar 29, 2024

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 addRoll method, I've added a public rebuild() method that will rebuild the rolls and render the dialog after changes have been made to the config. So that would be:

dialog.config.rolls.push({ parts: ["1d6"], options: { type: "radiant" } });
dialog.rebuild();

@arbron arbron modified the milestones: D&D5E 3.2.0, D&D5E 3.3.0 May 20, 2024
@arbron arbron changed the base branch from 3.2.x to 3.3.x June 24, 2024 17:05
@arbron arbron modified the milestones: D&D5E 3.3.0, D&D5E 4.0.0 Jul 18, 2024
@arbron arbron changed the base branch from 3.3.x to 4.0.x July 22, 2024 19:51
Copy link
Contributor

@Fyorl Fyorl left a 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.

templates/dice/roll-buttons.hbs Outdated Show resolved Hide resolved
templates/dice/roll-formulas.hbs Outdated Show resolved Hide resolved
module/documents/actor/actor.mjs Outdated Show resolved Hide resolved
module/documents/actor/actor.mjs Outdated Show resolved Hide resolved
module/documents/actor/actor.mjs Outdated Show resolved Hide resolved
module/applications/dice/roll-configuration-dialog.mjs Outdated Show resolved Hide resolved
module/applications/dice/roll-configuration-dialog.mjs Outdated Show resolved Hide resolved
module/applications/dice/roll-configuration-dialog.mjs Outdated Show resolved Hide resolved
Comment on lines 256 to 257
const rolls = this._finalizeRolls(event.submitter?.dataset?.action);
await this.close({ dnd5e: { rolls } });
Copy link
Contributor

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.

Suggested change
const rolls = this._finalizeRolls(event.submitter?.dataset?.action);
await this.close({ dnd5e: { rolls } });
this._finalizeRolls(event.submitter?.dataset?.action);
this.close();

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

module/applications/dice/roll-configuration-dialog.mjs Outdated Show resolved Hide resolved
Copy link
Contributor

@Fyorl Fyorl left a 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.

@arbron arbron requested a review from Fyorl July 24, 2024 23:58
module/dice/basic-roll.mjs Outdated Show resolved Hide resolved
@arbron arbron merged commit 597086e into 4.0.x Jul 25, 2024
@arbron arbron deleted the rolls/base branch July 25, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api epic system: dice Dice rolling functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add optional hit dice roll configuration window
3 participants