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

[#4628] Replace roll parts rather than rebuilding config entirely #4699

Open
wants to merge 1 commit into
base: 4.1.x
Choose a base branch
from

Conversation

arbron
Copy link
Collaborator

@arbron arbron commented Nov 11, 2024

To remove an issue with programmatically altering roll config for skill/tool checks & attack rolls, this modifies the buildConfig method to swap part data in-place, rather than completely rebuild the parts & data objects. To do this easily this introduces BasicRoll#swapParts which is designed to swap part data and part keys in place without losing the original ordering of the parts.

Todo

  • Also apply to attack rolls

Closes #4628

To remove an issue with programmatically altering roll config
for skill/tool checks & attack rolls, this modifies the
`buildConfig` method to swap part data in-place, rather than
completely rebuild the parts & data objects. To do this easily
this introduces `BasicRoll#swapParts` which is designed to swap
part data and part keys in place without losing the original
ordering of the parts.

Closes #4628
@arbron arbron added bug Functionality which is not working as intended api system: dice Dice rolling functionality labels Nov 11, 2024
@arbron arbron added this to the D&D5E 4.1.1 milestone Nov 11, 2024
@arbron arbron requested a review from Fyorl November 11, 2024 20:03
@arbron arbron self-assigned this Nov 11, 2024
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 might be the only solution but I'd like to try to keep our auto-computed and arbitrarily added parts separate so that they can always be re-computed from scratch, rather than merging parts eagerly and then having to swap them in-place. I feel like if we could incorporate the initialRoll concept into this framework then we'd be even better off than when we started.

I think this needs a bit more thought, so we might have to pull this PR from the milestone for now so that it doesn't hold up the rest of the other fixes.

@arbron arbron removed this from the D&D5E 4.1.1 milestone Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api bug Functionality which is not working as intended system: dice Dice rolling functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4.1: d20 roll ConfigurationDialogs do not make use of any added parts added programmatically
2 participants