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

Fix min/max setting. #306

Merged
merged 5 commits into from
Apr 15, 2024
Merged

Fix min/max setting. #306

merged 5 commits into from
Apr 15, 2024

Conversation

AustinYQM
Copy link
Contributor

@AustinYQM AustinYQM commented Apr 2, 2024

Pull Request Description

Addresses Issue #304

Changes Proposed

  • Create static Array.range() method
  • Fix the possibilties from min/max to create a range from the min max instead of limiting the posibilities to just the min/max.

Related Issues

Fixes #304

Checklist

  • I have read the contribution guidelines and code of conduct.
  • I have tested the changes locally and they are working as expected.
  • I have added appropriate comments and documentation for the code changes.
  • My code follows the coding style and standards of this project.
  • I have rebased my branch on the latest main (or master) branch.
  • All tests (if applicable) have passed successfully.
  • I have run linters and fixed any issues.
  • I have checked for any potential security issues or vulnerabilities.

Screenshots (if applicable)

image

Additional Notes

BEGIN_COMMIT_OVERRIDE
feat: New min/max syntax: dice: 1d[5-10]
END_COMMIT_OVERRIDE

@valentine195
Copy link
Member

I understand this was trying to fix an issue with min, max rollers, but it would break a feature to allow you to specify an arbitrary list of numbers.

I have been thinking about how to fix this and I think I prefer changing the min/max specification to 1d[5-15]

@AustinYQM
Copy link
Contributor Author

Ah, I understand, that makes a lot of sense.

5-15 makes a lot of sense (this is actually what I tried originally before realizing I was trying the wrong thing.

I could also see 1r[5, 15] but I think I like [5-15] more.

Am I editing in the correct place? I can change my PR later tonight to implement the - change.

@valentine195
Copy link
Member

This looks great, I will pull it down tomorrow and check it out in depth, thank you!🙏

@AustinYQM
Copy link
Contributor Author

I spent a lot of time at work thinking about how I would handle this kind of project (instead of working on banking software lol) and I think I settled on.

XdYm[values]±Z

Where X is the number of dice.
Y is the number of faces on those dice (and might be optional).
m is some modifying option (for example r for range, a for arbitrary, ! for explode, !! for exploding explodes, etc)
[values] is an array unique to the modifying option. For example for r and a it defines the faces but for ! it could define what values explode.
± divides our equation and works as a mathematical operator.
Z could be another dice equation (2d6+1d4) or it could be a simple number (2d6+3)

To parse it we split on any ± operators giving us basic math to do or dice equations to parse.
Then you can easily parse the string by looking for d and checking the suff after it. it will either be a single character and an array or a single character. check for those until you reach the end of your string.

so if you wanted to roll 2d6 plus 1d8 that only explodes on a 7 you'd do 2d6+1d8![7] which would be parsed as

Dice 1: 2d6 (no need to dive into this, its easy to handle)
Dice 2: We know the times and sides easily; detect the first non-number character after the D. We see it's explode. Call handleExplode(die, [options]) which can set our explode options.

Our die class could look like
{
sides: number[],
times: number,
explode: { isExplode: bool, isExplodeExplode: bool, explodesOn: number[] }
fudge: { isFudge: bool }
}

Anyways, all that to say that I think I like the idea of letter[options] as it makes parsing easier and allows for the addition of other things in the future easier.

@valentine195 valentine195 merged commit fc7ee80 into javalent:main Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐞 Min/Max rolls result in only min or max Value
2 participants