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

Crashes on probability calculation with too many dice #91

Open
Samasaur1 opened this issue Dec 4, 2020 · 3 comments
Open

Crashes on probability calculation with too many dice #91

Samasaur1 opened this issue Dec 4, 2020 · 3 comments
Labels
bug Something isn't working high priority Something that needs to be fixed urgently

Comments

@Samasaur1
Copy link
Owner

Describe the bug:

Dice.probabilities crashes if there are too many dice.

To Reproduce:

let dice = Dice(.d100, count: 5)
dice.probabilities

Expected behavior:

This should actually return the probabilities.

Stack trace:

I don't have a stack trace, because I'm reporting this bug way after I encountered it, but I know that the problem is an overflow when multiplying Chances together (multiplying the two denominators overflows).

Implementation Details:

  • Development OS: macOS 11.0.1 (irrelevant)
  • Target OS: iOS 13.6 (irrelevant)
  • DiceKit Version: 0.24.1

Additional context:

I believe I had already started working on this problem in the bigint branch, but didn't make a PR because it made small calculations very slow. I'm not sure how to have a "dynamic" conversion between Swift.Int and BigInt, which would make this a lot easier.

@Samasaur1 Samasaur1 added bug Something isn't working high priority Something that needs to be fixed urgently labels Dec 4, 2020
@Samasaur1
Copy link
Owner Author

Ah! Int has a static method func multipliedReportingOverflow(by other: Int) -> (partialValue: Int, overflow: Bool) (link to Apple docs). This will let us try to do the multiplication, but catch an overflow and either fail in a controlled way or switch to BigInt.

@Samasaur1
Copy link
Owner Author

e05a269 does some work on this

@Samasaur1
Copy link
Owner Author

Alright, so apparently what e05a269 (now decfaae, since I rebased) did was prevent crashes by approximating whenever the previous version would crash. This also has the advantage of not slowing down when large calculations would otherwise be performed (as would be the case if it automatically switched to using BigInt). I'm not sure which option we want to go with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority Something that needs to be fixed urgently
Projects
None yet
Development

No branches or pull requests

1 participant