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 speed ties #4780

Merged
merged 7 commits into from
Aug 3, 2024
Merged

Fix speed ties #4780

merged 7 commits into from
Aug 3, 2024

Conversation

mrgriffin
Copy link
Collaborator

@mrgriffin
Copy link
Collaborator Author

The doubles test fails, and I think this is because each battler shouldn't have a 50-50 chance of being faster than another. As an example, if battler1's bit says it's faster than battler2 and slower than battler3, but battler2's bit says it's faster than batlter3 then we have 3 > 1 > 2 from the first and 2 > 3 from the second, and that's obviously inconsistent.

I think the correct approach would be to generate a number 0..23 which corresponds to the number of permutations of 4 (MAX_BATTLERS_COUNT) elements.

@mrgriffin
Copy link
Collaborator Author

The doubles test fails

Pushed a fix for this, which is exactly as described in my message—I generate a number 0..23 and interpret that as the relative order of all four battlers.

@mrgriffin mrgriffin marked this pull request as ready for review July 15, 2024 14:49
@mrgriffin
Copy link
Collaborator Author

mrgriffin commented Jul 15, 2024

I've un-drafted this. I would like to make the doubles test faster, but I don't think it's possible to do in the test system as it exists rn.

My idea is to replace the 24 PARAMETRIZEs with just counting the order that each battler goes in, and then in a FINALLY (does FINALLY even work for PASSES_RANDOMLY?) we can check that turn order "slot" contains the same count for each battler. But as I say, I'm not sure how to do that. If anybody has suggestions then I'm all ears :)

@hedara90
Copy link
Collaborator

Could 3 different fixed damage moves and a healing move work?
Nature's Madness deals 50%, Crush Grip BP depends on HP of target, Dragon Rage 40 and Recover heals 50% of total.
I haven't done the math, but all the orders should result in a different final HP for the target if its max HP is less than 80, I think.

@mrgriffin
Copy link
Collaborator Author

Oh, that's clever! My gut feeling is that should work 🤔

@hedara90
Copy link
Collaborator

Attackers
A level 22 Wobbuffet with Crush Grip
Something using Nature's Madness or equivalent
Something at level 25 using Seismic Toss
Target
A level 13 Wobbuffet using Life Dew

I think this could work, but I didn't calculate all permutations.

@tertu-m tertu-m self-assigned this Jul 19, 2024
@hedara90 hedara90 added the bugfix Bugfixes label Jul 26, 2024
@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Aug 2, 2024

Any reason we can't merge this? This solves the issue.

Also conflicts. I made a new function for ai speed calcs and removed the randomness during a tie but now that there is consistency across the board we should bring it back. Not the function but the speed tie check. I think having the separate function has other benefits.

@hedara90
Copy link
Collaborator

hedara90 commented Aug 2, 2024

Oh, I was working on trying to set up a test that didn't have the massive array of turn orders.
But I hit some problems with it, I might have a setup that works though.
Life Dew, Crush Grip, Endeavor and Super Fang might result in unique final HP values depending on the speed tie.

@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Aug 2, 2024

Life Dew, Crush Grip, Endeavor and Super Fang might result in unique final HP values depending on the speed tie.

hm, I feel like we shouldn't hold up a merge because of this since it solves a crucial problem.

@hedara90
Copy link
Collaborator

hedara90 commented Aug 2, 2024

Yea, the test can wait.
I'll try to calculate it soon, but Crush Grip makes everything so much more complicated.

@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Aug 2, 2024

Any reason we can't merge this? This solves the issue.

Also conflicts. I made a new function for ai speed calcs and removed the randomness during a tie but now that there is consistency across the board we should bring it back. Not the function but the speed tie check. I think having the separate function has other benefits.

@mrgriffin conflicts 👀

@tertu-m I'll merge it once mgriffin solves conflicts if that's okay

@mrgriffin
Copy link
Collaborator Author

conflicts

Resolved! (Hopefully—I only ran the "Turn order" tests locally because it's bedtime 😅)

@AlexOn1ine
Copy link
Collaborator

conflicts

Resolved! (Hopefully—I only ran the "Turn order" tests locally because it's bedtime 😅)

For some reason test/battle/ability/comatose.c:48: Unmatched ANIMATION fails. Which is weird.

@mrgriffin
Copy link
Collaborator Author

For some reason test/battle/ability/comatose.c:48: Unmatched ANIMATION fails. Which is weird.

I watched the replay and it's very weird—the Ditto uses Toxic on itself (that's probably a difference in the speed ties in the old and new code)... But as far as Toxic goes I would guess what's happening is that Ditto tries to go for Celebrate which would be the first move in its moveset... but the first move is now Toxic due to it transforming.

@AlexOn1ine AlexOn1ine merged commit 9d97537 into rh-hideout:master Aug 3, 2024
1 check passed
@hedara90
Copy link
Collaborator

hedara90 commented Aug 3, 2024

Test setup:
Player
Wobbuffet, 480 MaxHP, 360 HP, 100 Def
Anything
Opponent
Wobbuffet, 490 HP (default), 100 Atk
Anything

Turn
Player Wobbuffet, Endeavor -> Opponent Wobbuffet
Player Anything, Life Dew
Opponent Wobbuffet, Crush Grip -> Player Wobbuffet
Opponent Anything, Super Fang -> Player Wobbuffet

Possible Outcomes

Player Wobb HP Foe Wobb HP
188 360
189 360
261 360
235 360
262 360
202 360
189 378
189 189
189 480
188 480
188 240
188 188
262 262
262 142
202 403
202 202
262 283
202 283
235 180
261 180
235 235
235 300
261 141
261 261

@Bassoonian Bassoonian added the category: battle-mechanic Pertains to battle mechanics label Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bugfixes category: battle-mechanic Pertains to battle mechanics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants