-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Dynamic Switch AI + Switch Utility Functions #6208
base: upcoming
Are you sure you want to change the base?
Dynamic Switch AI + Switch Utility Functions #6208
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.
Initial review and a few questions, the cleanup changes look great! :D
for (j = 0; j < abilityCount; j++) { | ||
if (GetMonAbility(&party[i]) == abilities[j]) { | ||
// mon has ability | ||
if (percentChance && (Random() % 100 > percentChance)) |
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.
I'm not sure I love the idea of embedding a non-testable Random call in a function like this. Is the goal of the randomization to have a failure rate for switching, ie. have percentChance
to not return a valid mon even if one exists? If so, I think that should be handled on the switching function side, not in the helper function.
I'm on board for the idea of randomizing between acceptable candidate mons (though again I'd use RandomPercentage
with a tag instead so we can write a test for it, which I'd also want included) if you that's something you want to do here, but I'm not on board for having a failure rate in the helper function rather than the switch decision function.
|
||
// custom switching logic | ||
// NOTE: needs to always end with `return SetSwitchinAndSwitch` or `return FALSE` | ||
if (gDynamicAiSwitchFunc != NULL && gDynamicAiSwitchFunc(battler)) |
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.
I'd move this below the // NOTE: The sequence of the below functions matter! Do not change unless you have carefully considered the outcome.
comments, because that also applies to this function
struct TerrainParams { | ||
u32 flag; | ||
u16 move; | ||
u16 abilities[2]; |
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 2
used throughout this sequence feels like a magic number, can we use a define instead?
return FALSE; | ||
} | ||
|
||
bool32 ShouldSwitchIntoElectricTerrain(u32 battler) |
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.
This isn't actually used by default, right? This set of functions is just an example of a function that could be used? If so, could you include a few lines (either in comment or in the PR description) for how to use this as the dynamic switching function the same as you did for the AI flag one?
Introduces
gDynamicAiSwitchFunc
, which is similar tosDynamicAiFunc
but is meant for switching logic rather than move scoring. This function takes precedence over all other switching logic checks.An example dynamic switch logic function for prioritizing a specific terrain is also provided. (NOTE this was only tested at a base level so may be flawed)
This PR also introduces some utility functions to battle_ai_switch_items.c including: