Enemy/bosstelesa: decompile Boss Telesa behavior - #134
Conversation
Report for GMSJ01 (9b62e09 - fe7bed2)📈 Matched code: 37.39% (+0.24%, +8672 bytes) ✅ 68 new matches
...and 38 more new matches 📈 38 improvements in unmatched items
...and 8 more improvements in unmatched items |
KakarottoCake
changed the title
|
Follow-up on the latest commit ( Header
Source changes
Effect
TU fuzzy match: 90.38% → 93.18%. Remaining residuals are MWCC stack-frame padding, register-allocation shifts, and inlining depth (e.g. |
|
Addressed most of the review in Done
Pushed back on
Still TODO (not attempted / couldn't resolve cleanly)
One thing to flag: routing the sounds through the inline |
| } | ||
| } | ||
|
|
||
| BOOL TNerveBubbleLive::execute(TSpineBase<TLiveActor>* spine) const |
There was a problem hiding this comment.
Nerve definitions/declarations should use the macros
There was a problem hiding this comment.
Not done — I think this one has to stay hand-written, and I would rather flag it than quietly skip it.
DEFINE_NERVE emits theNerve() and execute() adjacent. The map has them separated by two other functions:
002d2c38 0003e8 execute__16TNerveBubbleLiveCFP24TSpineBase<10TLiveActor>
002d3020 000214 appendEnemy__7TBubbleFv
UNUSED 0000d8 appendItem__7TBubbleFv
UNUSED 000064 theNerve__16TNerveBubbleLiveFv
002d3234 00005c __dt__16TNerveBubbleLiveFv
Since .text emission is reverse definition order, that means the retail source defined theNerve, then appendItem, then appendEnemy, then execute — so the two halves cannot be adjacent.
I tried it anyway to be sure: converting to DEFINE_NERVE makes check-changed-symbol-order.py fail with Non-weak symbol ORDER differs from the map. Reverted. Every other nerve in the file does use the macro — this is the only one that cannot. Happy to add a comment above it explaining why if you would like.
🤖 Addressed by Claude Code
|
Pushed two more commits addressing feedback:
Still open from the review: the 6 remaining |
|
Edited: this predated the rebase onto Status update — five commits. match
The extra layer is what fixes the nerve singletons. Reaching reconstruct reconstruct
The open question in the original version of this comment is now resolved, in
My first attempt routed it from the Spit nerve instead, which is one layer too shallow, and Four sizes now land exactly on the map — For current figures see the description; the ones originally quoted in this comment were against a base the rebase has since superseded. |
eeb747a to
574381a
Compare
- route sounds through SMSGetMSound()->startSoundActor + SoundEffects enum - add boss telesa particle enum entries, use them in emit/load - use MsMtxSetXYZRPH f32 overload in TBubble::calcRootMatrix - drop fabricated isActorTypeOf, use direct actor-type comparisons - rename MActor::getAnmBck -> getCurBckAnmPtr (real map name) - unk198/unk1A8 -> bool[3] arrays (remove pointer fakematch) - rolling/allStopped -> bool, MsWrap-free 360 wraps restored - fabsf instead of __fabsf, CAM_SHAKE_MODE enum, SMSGetMarDirector
Replace 42 hand-expanded copies of the bck motion-blend boilerplate
with the MActor wrappers that already exist for them, matching the
idiom Mrkol used when cleaning up bosseel:
actor->setBckOldMotionBlendAnmPtr(actor->getBckAnm());
actor->setBckFromIndex(n);
actor->setMotionBlendRatioForBck(ratio);
No raw ->unkC accesses are left in the TU. Net -183 lines, and the
match improves 92.242% -> 92.414% (TNerveBossTelesaAppear 99.5 -> 99.9,
TNerveBossTelesaHide 96.3 -> 99.0, setSpicy 95.7 -> 97.7,
moveObject 78.2 -> 80.1).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TBossTelesaBody::checkHit and TBossTelesaTongue::checkHit are UNUSED in mario.MAP -- they are real functions that the compiler fully inlined and the linker then stripped. Their bodies were open-coded inside TBossTelesa::moveObject instead of living in the methods themselves. Move each loop into its own method and let moveObject just call them, the same way TBGCork::perform was pulled out of TBossGesso::perform. Body's out-of-line copy now matches the map size exactly (0xa0); Tongue is at 0xec of 0x23c, so it still has code left to recover. Also use the actor-type inlines rather than open-coded comparisons: checkActorType(ACTOR_TYPE_ENEMY) for the "is an enemy" test Mrkol pointed at, and isActorType(0x80000001) for the Mario checks -- the boss's own collision loop was comparing against ACTOR_TYPE_PLAYER (0x80000000) rather than the 0x80000001 the game actually uses. unk16C/unk170/unk174 now carry their real types instead of THitActor*. moveObject 78.2% -> 85.3%; TU 92.414% -> 92.712%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 0x20000 live flag is set inside the nerve check, not after it. The ROM's early-exit branch lands on the 0x40 flag block and skips the 0x20000 one, so when the current nerve is already TNerveSmallEnemyDie only 0x40 gets set. TBubble::kill() now matches 100%.
…ppers Both collision-actor receiveMessage overrides forward to a TBossTelesa method rather than touching the spine directly. Both of those methods are UNUSED in mario.MAP, so they are fully inlined back into the caller and the emitted code is unchanged in size. The extra layer is what makes the nerve singletons match. Reaching pushNerve directly put TNerveBase<TLiveActor>::TNerveBase() at inline pass 3, where its 2 statements fit the budget of 3 and it was inlined. Going through the wrapper pushes it to pass 4, which never runs, so it stays an out-of-line call exactly as the ROM has it. TBossTelesaTongue::receiveMessage and TBossTelesaBody::receiveMessage both now match 100%.
The tail of the SlotStart nerve is a bool-returning TBossTelesa method, not an inline early return. checkSlot is UNUSED in mario.MAP at 0xd4, which is large enough for the isRollDrum test, the unk18C reset and the whole SpitSlotItem nerve singleton block. Routing through it fixes two things at once: the inlined bool result is materialised and then tested, which is the li/b/li/clrlwi./beq sequence the ROM emits, and the extra call layer pushes TNerveBase<TLiveActor>::TNerveBase() out to inline pass 4 so it stays an out-of-line call. TNerveBossTelesaSlotStart::execute now differs only in stack frame size.
moveStart is 19 statements, so a call to it at inline pass 0 (which has no limit) was being expanded; the pragma existed only to stop that. The SlotStart nerve reaches it through TBossTelesa::slotStart(), which is UNUSED in mario.MAP at 0x40 -- exactly the three statements it needs. Going through the wrapper puts moveStart at pass 1, where 19 statements exceed the budget of 10 and it stays an out-of-line call on its own. The pragmas around isRollDrum, getSlotResult and forceAllItemKill turn out to do nothing at all: those three are byte-identical with or without them. Verified by diffing the full nonmatching symbol set before and after: identical, no regressions. Two pragmas remain, around randomReset (its caller TNerveBossTelesaAppear::execute drops to 80% without it) and forceStopSlot.
mario.MAP records getCurBckAnmPtr__6MActorFv at 0x1c (28 bytes), but the accessor we had under that name was `return unkC;`, which compiles to 8. 28 bytes is the shape of the null-checked `unkC->unk24` body we had named getBckAnm, so the two were swapped. Per Mrkol: getCurBckAnmPtr is the null-checked one returning J3DAnmTransformKey*, and the 8-byte `return unkC;` getter is ours to name, so it becomes getUnkC() alongside the existing fabricated getUnk28(). They are not interchangeable -- different return types and different callers -- so this is a rename of both, not a merge. 8 references to the old getCurBckAnmPtr (gatekeeper, hinokuri2, NpcInitPrg) and 27 to getBckAnm (CameraBck, bosseel, bosstelesa) updated. No change to the nonmatching set in any of the six affected translation units, CameraBck included, which is at 0. This does not yet clear the validator's MISSING report for the symbol: the map wants bosstelesa.o to define a weak out-of-line copy, and that only appears once some call to it fails to inline.
main removed the base-class LIVE_FLAG_UNK10000 because 0x10000 is overloaded between derived classes, so declare a class-scoped one on TBossTelesa and qualify the use, following TBossEel. Also reflow the six setBckOldMotionBlendAnmPtr calls that went over the 80-column limit after the getCurBckAnmPtr rename. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
setBckAnm was a two-statement stub with no callers; the map wants 0xd4. Its real body is the animation-switch block that was open-coded in setSpicy, so move it there and have setSpicy call setBckAnm(1). That also fixes the missing getCurBckAnmPtr__6MActorFv weak copy the map expects from this TU. getCurBckAnmPtr is 4 statements, so it is only refused at inline pass 3 (budget 3). Writing the block in setSpicy put it at pass 2; routing through setBckAnm and letting setSpicy inline into TBossTelesaTongue::checkHit puts it at pass 3, where MWCC gives up and emits the out-of-line copy. setBckAnm has to stay under the pass-1 budget of 10 statements for that to happen, hence the ternary for the bas name. All four sizes now land exactly on the map: setBckAnm 0xd4 (212) setSpicy 0x19c (412) TBossTelesaTongue::checkHit 0x23c (572), was 0xec getCurBckAnmPtr 0x1c (28), was absent check-changed-symbol-order.py goes from FAIL (1 missing symbol, 20 UNUSED size mismatches) to PASS with 18. No change to the nonmatching set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- reconstruct TBubble::appendItem from the block open-coded at the bubble generation site, symmetric with appendEnemy; lands on the map's 0xd8 - offAllCollision/onAllCollision had a fourth collision actor the map does not account for. Dropping unk174 puts both at the map's 0x30, and the three open-coded sites now call offAllCollision() - use MActor/J3DModel accessors instead of mMActor->unk4->unk20 - use the MTX* wrapper macros instead of PSMTX* throughout - use checkLiveFlag() instead of testing mLiveFlag directly - hardcode the composed actor types rather than composing them from the ACTOR_TYPE_ macros - give unk2A8 and unk2F8 their real TMapObjBase* type and drop the twelve casts at the use sites; likewise hoist the TItem* cast in slotFall UNUSED size mismatches 18 -> 15. No change to the matched set: 61/100 functions and 8672 bytes before and after, per-function percentages identical. mario.dol: OK. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c045766 to
0aa5dbd
Compare
unk178/unk17C/unk180 were three adjacent TRoulette* fields that four
different sites already treated as an array, three of them by casting:
((TRoulette**)&unk178)[i]->setRollSp(unk184->unk1E4[i]);
(&unk178)[found] = (TRoulette*)actor;
Declaring `TRoulette* unk178[3]` says what those sites are doing and
removes all three casts. The original almost certainly declared it this
way, since indexing three separately-named pointers is not something the
code would do by choice.
No effect on the match: 61 of 100 functions byte-identical and 8672 bytes
before and after, per-function percentages unchanged. mario.dol: OK.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Summary
UNUSEDbodies the map requires, rather than fencing them off with#pragma dont_inlineMatching
Current figures for the BossTelesa TU:
.data,.rodata,.sdata,.sdata2,.sbssNote that
fuzzy_match_percentis lower than it was earlier in the PR's history despite the TU being further along. Reconstructing an inlinedUNUSEDbody adds a function with no counterpart on the target side, so it counts as an extra symbol and drags the average down. This TU is better judged on exact functions and matched code.Not everything is close yet —
TBossTelesaKillSmallEnemy::checkHitandTBossTelesa::forceHideare the weakest, and are genuinely unreconstructed rather than near-misses.Inline reconstruction
Six pragma-fenced blocks have been reduced to two (
randomResetandforceStopSlot). The rest were replaced with the real call structure, using the compiler's own inlining rules rather than suppression:This is what makes wrapper methods load-bearing.
TBossTelesa::tongueHitWater,checkMessage,checkSlot,slotStartandsetBckAnmall exist in the map asUNUSED, and routing calls through them puts the nerve-singleton constructors andMActoraccessors at the pass depth where retail refuses to inline them.setBckAnmis the clearest case. Its real body is the animation-switch block that had been open-coded insetSpicy; moving it there and keeping it under the pass-1 budget of 10 statements is what makesMActor::getCurBckAnmPtr(4 statements) land at pass 3, where MWCC gives up and emits the weak out-of-line copy the map expects from this TU. Four sizes land exactly:setBckAnmsetSpicyTBossTelesaTongue::checkHitgetCurBckAnmPtr(weak)TU structure
UNUSEDsymbolsUNUSEDsize warnings, i.e. inlined bodies not yet reconstructed to their retail sizeKnown gaps
UNUSEDbodies are still the wrong size, the largest beingfanfale(map 496, ours 4),prepareGenerate(472 vs 12) andcalcObjCollision(412 vs 4)#pragma dont_inlineblocks remain (randomReset,forceStopSlot); the wrapper that would replacerandomReset's has not been identifiedVerification
build/GMSJ01/mario.dol: OKpython tools/check-changed-symbol-order.py src/Enemy/bosstelesa.cpp— PASS