search

TheCraiggers / Pathfinder-Discord-Bot

MIT License
24 stars 15 forks source link

Custom Properties Do Not Allow Multiplication or Division #64

Closed coreyabrown closed 3 years ago

coreyabrown commented 3 years ago

Consider changing the propertiesRegex to the line below to allow for multiplication and division when adding properties as well as parentheses. This would allow automatic rolling for critical hits i.e. criticalbowdamage:=2*({1d6}+{1d6})+{1d10}

In the example below, I added "|*|\/|(|)" to the existing regex to allow the other characters that math.js should be able to handle.

propertiesRegex = /(?!)?(?\w+):((?\d+)(\/|\)(?\d+)|(?(=?(\w|[|]|{|}|+|-|*|\/|(|))+)))/g;

TheCraiggers commented 3 years ago

Makes sense. I hate the way I did these Regexes. Ugh... these things were refactored so many times, this one probably fell through the cracks.

Anyway, just built a new computer, and my IDE isn't set up yet. It'll take me a couple days, but should be a very easy fix.

coreyabrown commented 3 years ago

Cool! I tried to fix it myself, but my computer is so crappy, I couldn't even install npm!!

TheCraiggers commented 3 years ago

I jinxed myself by calling this an easy fix.

Got it mostly working, but I'm currently stuck on PF2E's rule that you should always round down on fractions. The current way the math works is that all dice are rolled, and then the math is done. This will lead to very odd results for complex math.

I'll think on this some more. Worst case, I can always release with multiplication support as that's safe from this issue.

coreyabrown commented 3 years ago

That's true. Division would cause problems, but multiplication and parentheses should always result in whole numbers based on the possible inputs.

coreyabrown commented 3 years ago

Also, re-reading my proposed fix, it looks like the escape characters weren't saved in my copy paste! You'd likely add the following to the regex:

|\*|(|)

You probably already knew that, just figured I would clarify!

TheCraiggers commented 3 years ago

I think I have a solution, but it'll require some refactoring. On the plus side, I'll be able to remove an entire dependency while also heavily increasing the amount of stuff the roller can do.