GC2D/GCConsole2: decompile the HUD update and draw paths - #152
GC2D/GCConsole2: decompile the HUD update and draw paths#152KakarottoCake wants to merge 7 commits into
Conversation
Implements the fifteen per-frame process methods, drawWater, drawJuice, drawWaterBack, loadAfter and startAppearBalloon, which were previously stubbed empty in the header, plus the Dolpic news tables and the pane helper routines they share. Byte-identical functions go from 14/41 to 32/60; the compared set grows because the formerly empty methods now emit. The remaining data gap is 40 bytes in .data, which is the precompiled-header footprint addressed by doldecomp#130 and deliberately untouched here. Field mapping: several opaque char[] blobs are replaced with typed fields at identical offsets, evidenced from the target disassembly (unk70/72/73/74/76/77, unk2B8, unk424, and unk154 as a TBoundPane*[3]). Also fixes an existing typo in the second star loop, which assigned to unk134 rather than unk154. Three constructs here are fitted to the compiler and worth calling out rather than leaving for a reviewer to find: - The process methods are reached through one accepted helper layer below perform() so that MWCC offers them at a pass with a statement budget and refuses to expand them, emitting each with the weak linkage the map records. Called directly from perform() they are offered at the unlimited first pass and absorbed regardless of size. The helpers are grouped by HUD subsystem to stay readable rather than being opaque forwarding wrappers. - Four pane state helpers take their pane parameter as a reference to a pointer. The reference is never rebound; it is there because it produces the member reload sequence retail emits. Each was tested with a plain pointer and each regressed: updateDownPaneState -2, updateUpPaneState -4, updateUpPaneStateAfter -1, updateUpBlendPaneState -1. - setBlendDigit and the placement of updateCounterState() after the water-gauge interpolation are positioned so that TVec3<f32>::set falls past the inliner's statement budget and is emitted out of line, as the map requires, in the same five call sites retail has. The rogue include of M3DUtil/InfectiousStrings.hpp follows the existing placement in src/Enemy/graph.cpp and restores the MActorMtxCalcType_* table and the dummy string pair, closing .rodata entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Report for GMSJ01 (9b62e09 - c1d62b7)📈 Matched code: 37.30% (+0.15%, +5412 bytes) ✅ 16 new matches
🥀 3 broken matches
📈 25 improvements in unmatched items
📉 2 regressions in unmatched items
|
`perform` was 4032 bytes short of retail. The gap was not missing logic but logic factored out of it: setCoinCounterDigits, setJetCounterDigits and getBalloonTextColor existed as separate functions in our object and not in retail's, which has that code inline. Each sat two call layers below `perform`, so the inliner offered it at a pass with a statement budget of 10 and refused it -- 48, 27 and 35 statements respectively. Folding each into its only caller puts the combined helper back at the unlimited first pass, where it is absorbed. All three had exactly one call site, so this moves code rather than duplicating it. `perform` goes from 13.4% to 17.2%, and 0x36ec to 0x3e28 against retail's 0x46ac. startLifeMeterDisappear was tried the same way and reverted. It has four call sites, so folding it duplicated an eleven-line block four times for 1.9 percentage points on a function that stays unmatched either way. Not worth the source. No other function changes state; still 32/60 in the unit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All seven preprocessor macros in this file are gone, converted to static inline functions or folded into their single call site. Four of them had exactly one use, and one of those -- UPDATE_LIFE_SEGMENT_COUNT -- contained another one-use macro, UPDATE_LIFE_METER_COLORS. A 27-line macro used once is a function that was afraid to be a function. SET_THREE_DIGITS turned out to duplicate a helper that already existed, so it was removed rather than converted. No function changes match state: still 32/60 in the unit, data still 40 bytes (the PCH footprint from doldecomp#130), linkage still matches the map. perform() moves from 17.2% to 19.5% as a side effect -- the macros were being pasted into their callers, so removing them changes which bodies the inliner is offered and at what statement budget. Some conversions cost a little percentage and later ones gave it back; none cost a byte-identical function. Reported by troy and theAzack9 on the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
None of this file's helpers existed in the original source -- the retail build wrote that code inline at each call site, and they are here to keep the decompilation readable. Marking them says so, matching the convention used about 335 times elsewhere in the tree. 72 comments added, covering every fabricated helper including the ones that came out of the macro conversion in the previous commit. The group note above the pane-offset pair is replaced by per-function markers. The static data tables are real and stay unmarked. Comments only: the unit still builds byte-for-byte the same, 32/60, perform() 19.5%. Reported by troy and theAzack9 on the PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TheAzack9
left a comment
TheAzack9
left a comment
There was a problem hiding this comment.
This is what i had the time for before a meeting... 😅 :)
| static int getOffsetForBelowScreen(TExPane* pane) | ||
| { | ||
| // setPaneOffset moves this to y1 = 465 | ||
| return 465 - pane->mInitialBounds.y1; | ||
| } | ||
|
|
||
| // fabricated | ||
| static int getOffsetForAboveScreen(TExPane* pane) | ||
| { | ||
| // setPaneOffset moves this to y2 = -1 | ||
| return -(pane->mInitialBounds.y2 + 1); |
There was a problem hiding this comment.
This should use the mInitialBounds getter. I also feel the entire function feels wrong to me. It smells way more of some inlined function inside TExPane instead of being defined here. It is probably best to avoid this inline until it can be proven / disproven later on 🤔
I usually mark them with // Possibly inline to help the next person see my thought that it might be a boundary instead of assuming :)
There was a problem hiding this comment.
You were right on both counts. It now uses TExPane::getInitialBounds(), and the helper is
gone — the expression is folded into all 12 call sites.
On your TExPane hunch: I think the evidence backs you up, so I want to put it on the record
rather than just assert it. After folding, every single call site turns out to be an
argument to a setPaneOffset/updatePaneOffset call on the same object whose bounds it
reads:
unk2F8->setPaneOffset(unk98, 0, 0, 0, 465 - unk2F8->getInitialBounds().y1);
unk44C->updatePaneOffset(20, 0, 525 - unk44C->getInitialBounds().y1);
unk140->setPaneOffset(40, 0, 0, 0, -(unk140->getInitialBounds().y2 + 1));22 sites, no exceptions. And the codegen agrees: folding cost eight exact matches, and six of
the eight are off by exactly one instruction too many (startAppearCoin 79→80,
startAppearRedCoin 105→106, startAppearStar 243→244, startDisappearStar 200→201, plus
startInsertJetBalloon and startDownLeftBot); the other two are one instruction short
(pauseOut 229→228, startDisappearTimer 76→75). That is what you would expect if retail
computed this inside TExPane, where mInitialBounds.y1 comes off a this that is already in
a register, instead of re-fetching it through a getter from outside.
I have deliberately not added the TExPane member — that is your call and Mrkol's, not
something this PR should decide, and I would rather ship the honest folded version than guess
at a second API. If you want me to try TExPane::setPaneOffsetBelowScreen(s32 time) or
similar, say the word and I will test it; the eight functions above are the measurement.
The regression is retained on purpose under the readability-over-matching rule. Happy to be
told that trade was wrong.
| JUTPoint TGCConsole2::cCoinMidPoint(0, 45); | ||
| JUTPoint TGCConsole2::cCoinBotPoint(0, 0); | ||
|
|
||
| // fabricated | ||
| static int getOffsetForBelowScreen(TExPane* pane) | ||
| { | ||
| // setPaneOffset moves this to y1 = 465 | ||
| return 465 - pane->mInitialBounds.y1; |
There was a problem hiding this comment.
This would likely be the same as getOffsetForAboveScreen
There was a problem hiding this comment.
Folded for the same reason, 10 call sites. See the note above — the two are indeed the same
shape, and both fold into the same setPaneOffset argument position.
| GXSetNumTevStages(texGenCount); | ||
| GXSetVtxAttrFmt(GX_VTXFMT0, GX_VA_POS, GX_POS_XY, GX_F32, 0); | ||
| GXSetVtxAttrFmt(GX_VTXFMT0, GX_VA_TEX0, GX_TEX_ST, GX_S8, 0); | ||
| GXClearVtxDesc(); |
There was a problem hiding this comment.
This is also an inline i am a bit sceptical about. Not because i think it should be somewhere else, but rather because it is very specific 🤔 It might be true, but i don't feel confident it is yet, so not making it inline might be better for now
There was a problem hiding this comment.
This is the one place I pushed back, so I want to be explicit about it rather than quietly
keep it. It is a single coherent 18-line GX state block — matrix load, cull mode, tex-gen and
TEV stage counts, vertex attribute format, vertex descriptors, channel control — used by both
gauge draw paths. Duplicating it inline would put 18 lines of GX setup in the middle of two
functions that are already long, and the reader would have to diff them by eye to see they are
the same state.
Kept, and marked // Possibly inline using your convention so the next person knows it is an
assumption and not a claim. If you would still rather see it folded I will do it.
| { | ||
| GXSetTevColorIn(GX_TEVSTAGE0, GX_CC_C0, GX_CC_C1, GX_CC_TEXC, GX_CC_ZERO); | ||
| GXSetTevAlphaIn(GX_TEVSTAGE0, GX_CA_A1, GX_CA_A0, GX_CA_TEXA, GX_CA_ZERO); | ||
| GXSetTevColorOp(GX_TEVSTAGE0, GX_TEV_ADD, GX_TB_ZERO, GX_CS_SCALE_1, |
There was a problem hiding this comment.
Kept and marked // Possibly inline, same reasoning. It is a repeated stage-specific TEV
configuration rather than an incidental one-use wrapper.
| GX_CC_ZERO); | ||
| GXSetTevAlphaIn(GX_TEVSTAGE1, GX_CA_ZERO, GX_CA_APREV, GX_CA_TEXA, | ||
| GX_CA_ZERO); | ||
| GXSetTevColorOp(GX_TEVSTAGE1, GX_TEV_ADD, GX_TB_ZERO, GX_CS_SCALE_1, |
There was a problem hiding this comment.
Kept and marked // Possibly inline, same as stage 0.
| // fabricated | ||
| static inline bool isBossPakkunCameraDemo(void* boss) | ||
| { | ||
| return *(u8*)((u8*)boss + 0x29A) != 0; |
There was a problem hiding this comment.
This is VERY fake matching...
There was a problem hiding this comment.
Removed. On top of being a raw +0x29A cast, it had zero callers — it was dead code that
the compiler never saw.
| // fabricated | ||
| static inline void setPictureColor(J2DPane* pane, u32 white, u32 black) | ||
| { | ||
| ((J2DPicture*)pane)->mWhite = white; |
There was a problem hiding this comment.
Doubt this is a real function, more realistic to be an inline in J2DPane. Remove this inline
There was a problem hiding this comment.
Covered above — it turned out to be J2DWindow rather than J2DPane, and it is now a named field
plus accessor there instead of a fabricated free function here.
| static inline void setupLifeSegments(TGCConsole2* console, int firstIndex, | ||
| int count, int value, u32 white, u32 black) | ||
| { | ||
| for (int i = 0; i < count; ++i) { |
There was a problem hiding this comment.
I could see this being an inline, but i wouldn't bet on it. I doubt they would send in white and black as variables. So i doubt the bounds are correct
There was a problem hiding this comment.
Removed, and setPictureColor with it. The mWhite/mBlack assignments are now explicit at
their 9 call sites, and the life-segment initialisation loop lives in loadAfter() where the
rest of the life-meter setup is visible. loadAfter() went 57.0% → 58.0%.
| // fabricated | ||
| static inline void drawDetachedPane(J2DPane* pane, J2DOrthoGraph& graph) | ||
| { | ||
| if (pane != nullptr && pane->isVisible()) |
There was a problem hiding this comment.
This would also make more sense inside J2DPane than here. I would remove this inline for now
There was a problem hiding this comment.
Removed. Its only caller was drawDetachedBoundPane, which was itself dead — see below.
| static inline void drawDetachedBoundPane(TBoundPane* pane, J2DOrthoGraph& graph) | ||
| { | ||
| if (pane != nullptr) | ||
| drawDetachedPane(pane->getPane(), graph); |
There was a problem hiding this comment.
Removed. While acting on your note I found this one had zero callers — you had not flagged
it as dead, but it was. Deleted along with drawDetachedPane above.
|
Thanks for the review — the fabricated-inline objection was the right call and I have acted on Headline numbers, stated plainly:
Three helpers are kept rather than folded — the GX/TEV setup blocks — and are argued Two things worth flagging that you did not ask about:
|
2 participants
TGCConsole2is the in-game HUD. Fifteen of its per-frameprocess*methods, plusdrawWater,drawJuice,drawWaterBack,loadAfterandstartAppearBalloon, werestubbed empty in the header. This implements them, along with the Dolpic news tables and
the pane helpers they share.
Byte-identical functions in the unit go from 14/41 to 32/60. The compared set grows
because the formerly empty methods now emit. Project total 8068 -> 8086.
Data: the missing-constant gap goes from 665 bytes to 40.
.rodatacloses entirely,including the uniform +204 head offset, and
.sdatanow matches. The remaining 40 bytesin
.dataare the precompiled-header footprint;GCConsole2is already aPCHObjectin#130, so that closes when #130 lands. Nothing here fabricates those bytes.
validate-symbol-order.pyreports[ OK ]on linkage, and map symbols missing from theobject go from 19 to 0.
Field mapping
Several opaque
char[]blobs become typed fields at identical offsets, evidenced from thetarget disassembly:
unk70/72/73/74/76/77,unk2B8,unk424, andunk154as aTBoundPane*[3]. This also fixes an existing typo in the second star loop, which assignedto
unk134rather thanunk154.Fitted to the compiler — flagging these rather than leaving them to be found
The
process*methods are reached through one accepted helper layer belowperform().Called directly from
perform()they are offered to the inliner at the unlimited firstpass and absorbed regardless of size; one layer down they are offered at a pass with a
statement budget, refused, and emitted out of line with the weak linkage the map records.
The helpers are grouped by HUD subsystem so they read as real code rather than as opaque
forwarding wrappers.
Four pane state helpers take their pane parameter as
T*&. The reference is neverrebound; it is there because it reproduces the member reload sequence retail emits. Each
was tested with a plain pointer and each regressed:
updateDownPaneState-2,updateUpPaneState-4,updateUpPaneStateAfter-1,updateUpBlendPaneState-1.setBlendDigitand the placement ofupdateCounterState()after the water-gaugeinterpolation put
TVec3<f32>::setpast the inliner's statement budget so it is emittedout of line, as the map requires, at the same five call sites retail has.
The rogue include of
M3DUtil/InfectiousStrings.hppfollows the existing placement insrc/Enemy/graph.cpp.Not attempted
perform()is still structurally short (ours 0x3700, target 0x46ac), and two functions areframe-only misses:
processDrawTelopshort by 24 bytes andstartDisappearCoinby 16.Left unmatched rather than padded.