Miscellaneous matches #2143
Miscellaneous matches #2143
Conversation
|
I have some Kirby matches stashed locally while #2138 is not merged, I'll rebase, pop the stash and push them if they're not obsolete later. |
src/melee/gr/grbigblue.c
Outdated
| { | ||
| Ground* gp = GET_GROUND(gobj); | ||
|
|
||
| HSD_Free((void*) gp->gv.corneria.xC8); |
There was a problem hiding this comment.
this is a very common failure mode for m2c+agent-based decomp. m2c will just choose the first enum variant that "fits"; it has no way to infer that this one should be gv.bigblue. You can, however, manually specify the variant you want using the --union-field flag.
Personally, I addressed by adding a separate Claude Skill for item-related decomps; it basically says, "when you're decomping functions related to item X, make sure there is an enum variant for it, and pass the right --union-field flag." Easy to imagine doing the same for gr/, ft/, etc.
More info here: #2076
There was a problem hiding this comment.
Fixed: changed to gv.bigblue and added missing xC8/xCC fields to grBigBlue_GroundVars. Also added guardrails to the vacuum system to detect this pattern going forward.
As for the skills, I'm checking them out now
|
|
||
| ip = gobj->user_data; | ||
| Item_80268E5C(gobj, 0, ITEM_ANIM_UPDATE); | ||
| ip->on_accessory = (HSD_GObjEvent) fn_802BB428; |
There was a problem hiding this comment.
Kept the cast - it's actually required because fn_802BB428 returns s32 but on_accessory expects void (*)(). Removing it causes a type error.
There was a problem hiding this comment.
Can the fn_802BB428 declaration be changed without breaking anything?
There was a problem hiding this comment.
Claude:
No, the declaration cannot be changed.
fn_802BB428returnss32because it wrapsit_802BCA30(Item*)which is declared as returnings32initseakchain.h:32. The function genuinely returns a value - the callback mechanism simply ignores it. The cast toHSD_GObjEvent(which expectsvoidreturn) is the correct approach for assigning an s32-returning function to a void-returning function pointer field.
There was a problem hiding this comment.
it_802BCA30 isn't decompiled yet and (almost certainly) has an inaccurate declaration; it should return void too.
| it_802874F0(gobj); | ||
| it_80287690(gobj); | ||
| if (ip->xCC_item_attr) { | ||
| } |
There was a problem hiding this comment.
fascinating: removing this empty if block increases the stack size, breaking the match. I've never seen that before. Very curious if anyone more experienced knows why this happens! Personally when I've had too much stack usage, I replace GET_ITEM with a direct access (but I always feel a little guilty about it...)
There was a problem hiding this comment.
Can't say I remember seeing something like that either.
There was a problem hiding this comment.
one would think that the compiler with the current optimization flags would optimize that away, no?
There was a problem hiding this comment.
looks like the if is irrelevant; a plain ip->xCC_item_attr; also matches (albeit with an "Expression result unused" warning). It does not match if you do attrs->x10_fall_speed; though, or any other attrs access; seems like it has to be ip. Interestingly, ip->x0 (a void* pointer) also does not match, but all the other fields I tried do.
My best guess is that accessing ip after the other statements keeps ip "live" wrt some phase of compiler analysis, but I don't know why that would decrease stack. ‾\_(ツ)_/‾
There was a problem hiding this comment.
Should we leave it as is or change it and accept the warning?
There was a problem hiding this comment.
cleanest fix here is probably to replace GET_ITEM with gobj->user_data (assuming that works)
|
Overall: I think this is promising, but needs a few more guardrails (particularly around enum variants and |
- Fix union variants (gv.corneria → gv.bigblue, add struct fields) - Revert grRCruise_80200540 fake match to stub - Move local structs to itCommonItems.h (itKyasarin, itWstar) - Add kyasarin variant to xDD4_itemVar union - Fix itFreeze_ItemVars.x0 type (s32 → f32) - Add itLikelike_ItemVars.x50 field - Use chain assignments and cleaner patterns - Remove type punning and raw offset access
- Move itHououAttr to itCommonItems.h - Use local variable instead of inline void* cast
I think the fact that these functions are so short and "easy" makes Claude be super lazy and exit early without checking for quality. I'll try to iterate a bit more on that. EDIT: Turns out the Sandbox that was created for every agent was completely oblivious to my docs so this was basically all done by 'raw' Claude. Fixed that, we'll see how it improves things. I'm going to pick a random unit and have it vacuumed (less functions this time to keep reviews more manageable) |
|
After iterating a bit more on the 'vacuum' I find it works better if directed towards an unit and with another claude instance overseeing the process to reduce friction (integration failures, shitty code, etc). So continuing with the analogy this works much better with someone directing it rather than have a roomba go wild all over the codebase. |
got a script running quality checks on the code (not AI, static code analysis) so that I can follow up before PR on the actionable items without having you guys point them out in review as more feedback comes in from the PRs with fundamental errors I'll add them to the static checks if possible, and to the prompting if not (less effective) |
|
What are the static checks? |
https://github.com/malvarezcastillo/melee-decomp-agent/blob/master/melee-ai/quality_check.py suggestions welcome |
|
Oh, I see. Not bad. |
Add grRCruise_GroundVars struct with proper float/s32 field types instead of using Map_GroundVars which has pointers at these offsets.
|
@lukechampine ping me again if the comments haven't been addressed via commit or reply so I can finish whatever is blocking the PR |
3 participants
Testing letting Claude be more autonomous with the 'vacuum system'.