-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add Gen 1 Crit Chance #5439
Add Gen 1 Crit Chance #5439
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preproc ifs should be regular if statements.
Many of the comments seem redundant.
Goodness my bad don't PR sleepy lol I've slashed down the comments pretty thoroughly let me know if this seems cleaner :) |
I don't know how many of the peculiarities of Gen 1 crits should be implemented. |
I intentionally didn't include the bug as discussion lately has been trending towards not supporting bugs in general, forget adding configs for new ones, but I can add it back in with out usual BUGFIX config if we like the idea of having it better :) I agree with you that both the multiplier scaling and the different calc would be in expansion's scope, but I didn't intend for them to be in scope for this PR just because I don't already have a working implementation of them. This is specifically gen 1 crit chance, not gen 1 crit damage. What d'you think about just adding chance for now, and I'll make a feature request issue to also add gen 1 crit damage to be added later? I'd prefer not to focus on adding that functionality right now, and was more PR'ing this because I already happened to have a working implementaiton. |
When running tests with |
Let me eat breakfast and make some strong tea and I'll begin double checking the list lol |
Progress update: Going through all of the assumption fails, most of them were fine and are being handled appropriately for the gen 1 system. Lansat Berry and G-Max Chi Strike weren't being considered and now are, though their assumptions still fail and their crit rates are still different because the math is entirely different, but I checked to make sure they were being considered properly by breaking the function to force gen 6 crit rates if they were being considered, which made those tests pass if the assumption was removed. Will push this update and then go through the explicit fails :) |
Okay this has taken me a while and it's always possible I've missed stuff while checking this over, but as far as I can tell all of the 69 fails are caused by the significantly higher crit rates messing up tests that require moves to do damage within a certain range by critting when they wouldn't normally. I don't think there's anything else going wrong here, as far as I can tell. I did also find one typo'd test name which I'll PR separately, otherwise I think this investigation is complete :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good (not that I would use it, Gen 1 Crit is cursed)
Description
Happened to be scrolling the branches on TAAR today, realized some of mine were outdated, got to this one and thought "hang on this is totally in scope for expansion isn't it"
This is basically an exact update of my branch to current expansion, that implementation has been working (or at least no one has ever told me it's not working lol) since 1.7.X and I know at least a handful of folks have used it, so I didn't want to do any unnecessary overhauls unless it was determined to be a good idea in review.
I've left in the more liberal comments I included in the branch as it was originally requested by someone who wasn't a programmer, though in this case I think they help guide customization as gen 1 crit mechanics behave so radically differently from the rest of the series. Feel free to suggest their removal.
Discord contact info
@Pawkkie