Add "Ai Item Improvements" and "Ai Item Optimizations" Skypatches to SkyTemple - #568
Add "Ai Item Improvements" and "Ai Item Optimizations" Skypatches to SkyTemple#568HappyLappy1 wants to merge 7 commits into
Conversation
Integrate Ai_Item_Optimizations and AI_Item_Improvements Skypatches into Skemple.
This is on your hands, chesyon!
Test Results 14 files ±0 14 suites ±0 45m 39s ⏱️ - 2m 57s For more details on these failures, see this check. Results for commit 8e66f64. ± Comparison against base commit 0aefaaa. ♻️ This comment has been updated with latest results. |
|
Maybe you can specify where the free space begins in the description? |
Frostbyte0x70
left a comment
There was a problem hiding this comment.
I have a couple of gripes with the idea of adding these patches by default tbh
- First, there's no way to ensure that AiItemOptimizations doesn't break anything. It's basically impossible to test that all previous AI behavior always works the same way, and if there's a bug, it's going to be basically impossible to spot.
- Second, AiItemImprovements seems like a very specific patch that I don't see many people wanting to apply. I think it makes more sense as a community skypatch.
- Third, you can't properly list everything AiItemImprovements does in the description, since it's too long. People won't want to apply a patch without knowing what it does, which requires checking out the patch on the community patches channel anyway, so why not keep it there?
I still pointed out a few things you need to fix, just in case.
(Note to self / other reviewers: I haven't tested if the patch applies correctly yet)
True. I've tested every item the Ai can use at least once, with an understanding of what the behavior of each item was based on the original code and the red rescue team c code. I have checked the 3 places the function is actually called to ensure I understand what the original function was intended to do. That said, it's not enough to ensure there won't be bugs.
Understandable. If others agree, I'll take this out and just include AiItemOptimizations. Am going to fix the things you suggested anyway, just in case. |
Addressed the comments left on my PR
Changes label names to be more intuitive, and ensure that IsAdjacentToEnemy is only used for the function. AreaEnd is identical to IsAdjacentToEnemy, but refers to the end of the function, rather than the next function.
Co-authored-by: Chesyon <55322011+Chesyon@users.noreply.github.com>
Enemies will not attempt to use items while embargoed! Thanks Mond for the feature request!
Frostbyte0x70
left a comment
There was a problem hiding this comment.
I'd say the code looks good now (save for the formatting / linting errors). Nice job!
Athough I still don't think it makes much sense to have the patches in SkyTemple.
Ai Item Optimizations reduces the size of GetAiUseItemProbability without changing anything else, creating 1080 bytes of free space in overlay29.
Ai Item Improvements replaces the same function with one that tweaks team and enemy AI to use more types of items, and use them more effectively. Here's a full list of all the things it adds/modifies: