GC2D/GCConsole2: inline boundaries, bounds accessors, and the water gauges (27 -> 33)

[KakarottoCake]

Follow-up to #152, continuing GCConsole2 as asked.

mario/GC2D/GCConsole2 27/60 -> 33/60. Project 8098 -> 8104. Six functions now
match byte for byte: startDisappearTimer, startAppearCoin, startAppearRedCoin,
startInsertJetBalloon, startDisappearStar, startDisappearCoin.

After that I went back over the ones you called out as poorly matched. Those did not
land byte matches, but they moved a long way and the reasons are concrete:

function	before	after
drawWater	83.82%	99.12%
drawWaterBack	74.67%	87.20%
processAppearStar	92.09%	95.99%
startAppearBalloon	92.46%	94.17%
processDownCoin	95.82%	98.88%
processAppearCoin	95.80%	98.86%
processDisappearBalloon	99.72%	99.89%

Full disclosure below, including the things I got wrong and the things I am still
stuck on, so you do not have to re-derive any of it.

---

Four mechanisms did most of the work

1. Retail doing arithmetic in more steps than us means an inline boundary

Where we wrote 525 - y1, retail emits two instructions:

subfic r3, r0, 0x1d1   ; 465 - y1
addi   r0, r3, 0x3c    ; + 60

not the folded subfic r0, r0, 0x20d. Writing 465 - y1 + 60 by hand does not
reproduce it -- that folds straight back. The constant can only arrive still waiting
for its + 60 if it crossed an inline boundary, because folding happens before
inlining and there is no re-fold afterwards.

That is getOffsetForBelowScreen, which @TheAzack9 asked me to hold off on during the
#152 review as unproven. The codegen now argues for it, so I have restored it under his
name with his // Possibly inline marker. getOffsetForAboveScreen is the same
argument: retail emits neg then add where we folded to a single subf.

Both are used only where an operation sits outside the boundary -- + 60,
+ unk26A, - getHeight(). That is the only place the boundary is observable.
Routing the plain 465 - y1 sites through the same helper costs matches; I measured
it and did not do it. So retail genuinely has both spellings, which is a bit
unsatisfying but is what the object says.

He also thought this belonged on TExPane rather than as a file-local. I have not done
that -- it is his call, and this PR should not grow a new API on his behalf.

2. Each inlined scalar accessor call costs exactly 8 bytes of stack

Measured across six functions before changing anything -- frame deficit against number
of direct ->mInitialBounds reads:

function	short by	uses
startAppearCoin	8	1
startAppearTank	8	1
startAppearRedCoin	16	2
startInsertJetBalloon	16	2
startAppearStar	16	2
startDisappearStar	16	2

Six for six, and those functions were otherwise byte-identical -- pad the frame by the
delta and the instruction diff goes completely empty.

The catch was the return type. The existing const JUTRect& getInitialBounds() gets
every frame exactly right but emits an extra address computation:

target:  lwz  r3, 8(r28)
ours:    addi r3, r28, 4 ; lwz r3, 4(r3)

A per-field int getInitialY1() const gives both the reserved slot and the direct
load. JUTRect already exposes getWidth()/getHeight() that way, so this follows
what is already there rather than inventing a shape. Returning JUTRect by value is
decisively wrong here -- 8099 -> 8091 project, 28 -> 21 unit -- which fits, since
JUTRect is 16 bytes and every delta is 8.

This landed four matches at once and also corrected frames I was not aiming at:
startDisappearCoin +24 -> +8, endCameraDemo +32 -> +24, startAppearTelop
+48 -> +40, and startAppearTank/startAppearStar to exact.

3. The register save mask says which values were floats

This is what cracked drawWater, and it is the one I would reach for first next time.
The target saves f28-f31 and only r20-r31; we were saving two float registers
and three more integer ones. Retail was holding values in float registers that we
held in integer registers, which for a function whose only floating-point work is
GXPosition2f32 points straight at the quad corners.

Three separate things fell out of that, and all three were needed:

• top and bottom are f32 locals. The target converts each exactly once,
  before GXBegin, and keeps them in f29/f28 across all four vertices.
• left and right are not locals at all. The target re-loads
  unk2BC[layer].x1/.x2 from memory and re-converts them at every vertex -- four
  loads and four xoris conversions for two values. Hoisting them, as either int or
  f32, is exactly what was costing the two extra float registers. Inline
  (f32)unk2BC[layer].x1 at each call site is what the target does.
• The height is the picture's, not the cached rect's. The target reads +0x18 and
  +0x20 off unk2A0[layer], which is J2DPane::mBounds.y1/.y2 -- i.e.
  J2DPane::getHeight() -- where we were subtracting inside unk2BC[layer]. The two
  rects hold the same numbers here, so this is not a behaviour change, but it is the
  one retail actually reads.

83.82% -> 99.12%. This does not transfer to drawWaterBack, and I checked before
assuming it would: there the target converts all eight position components separately
at each vertex, which is what drawGaugeQuadF32's int top, int bottom parameters
already produce. The two functions genuinely differ.

4. An assigned JUTRect inlines; a constructed one calls JUTRect::copy

Copy-construction goes through the out-of-line JUTRect::copy. Assigning to a rect
that already exists inlines member-wise as four lwz/stw pairs. That asymmetry is
visible in the object and it identifies which of the two the source used.

processAppearStar ended with JUTRect bounds(...) and JUTRect bounds2(...) for its
two emitter-centring blocks. The target has only one rect: it calls JUTRect::copy for
the first and inlines the second copy into the same stack slot. Reusing the one
variable took it 92.09% -> 95.99%. drawWater already shows both forms side by side
for the same reason, so this is the shape the file was written in.

Read the other way round, the same rule settles the getContentsBounds TODO that was
already sitting in startAppearBalloon. The target copies the contents rect twice
there -- once out of mContentsBounds at +0xec into a stack temporary, then again from
that temporary into the named local. A const JUTRect& return cannot produce that; a
by-value return does. So J2DWindow::getContentsBounds() returns JUTRect by value.
92.46% -> 94.17%.

processDisappearBalloon looked like it contradicted that, because it copies only
once. It does not: the target copies straight into a slot it reads getHeight() out of
two instructions later and never touches again -- an unnamed temporary, not a local.
The rect was only ever there to be measured, so the local goes and the call reads as
one expression. 99.72% -> 99.89%.

---

The rest of the commits

loadAfter was calling the wrong virtual. The target dispatches through the pane's
vtable at +0x14 -- J2DPane::resize, the fourth virtual after the destructor, move
and add, and the one J2DTextBox overrides. We called setFontSize, which is not
virtual at all, and read gpSystemFont's +0x24 (getHeight) where the target reads
+0x28, annotated in JUTFont.hpp as getWidth(), shifted left by 10. The bounds also
come from a stack copy via JUTRect::copy, and one height serves both boxes.
92.33% -> 95.24%.

I left the << 10 as the shift the code performs. I do not know what unit that width
is in and would rather leave it plain than name it wrongly -- flagging it in case you do.

load was filling only half the life-pane array. unk17C is J2DPane*[18]
holding nine pairs and unk1D0 is JUTRect[9], one per pair, but the loop indexed
unk17C[i] and unk17C[i + 1] for i in 0..8. Each iteration overwrote the previous
pair's second pane, indices 9 through 17 were never written, and unk1D0[i] took its
bounds from whichever pane landed at [i]. Everywhere else already indexes [n * 2]
and [n * 2 + 1]. This is match-neutral -- load is dominated by a 344-byte frame
difference -- and is in here because the code is wrong as written, not because it moves
a number.

drawWaterBack was drawing the full gauge twice. The else if (unk48) and else
arms both ended in the same drawGaugeQuadF32(bounds, bounds.y1, bounds.y2, 0.0f, 1.0f)
call. The target's unk48 == 0 and unk30C == 0 tests both branch to the same
GXBegin, which is a single guarded block followed by an unconditional draw.

The pressure flash resets its own counter. if (unk30C >= 25) unk30C = 0; sat in
the caller ahead of the colour computation. It is the final else of the colour chain
itself: the target's cmpwi r4, 0x19 / bge lands on li r0, 0 / stb r0, 0x30c(r29),
which then falls into the shared color + 0xc8. The counter is cleared instead of
picking a fade colour, not before picking one. The two are equivalent -- resetting
first meant the frame-0 branch ran with frame == 0 and both of its terms are
(f32)0 * k, so the colour came out unmodified either way -- which is why it was easy
to miss. The helper now takes the counter by reference so the whole cycle reads in one
place. Its comparisons are also signed in the target (cmpwi, not cmplwi) and the
fade's int-to-float conversion is xoris rather than a clrlwi zero-extend, so the
frame index is an int there, not a u8.

drawWaterBack picks its texture through getTexture. The lookup was written out
longhand as an if/else over mTextureNum into a local. That is exactly
J2DPicture::getTexture(0), which already exists. Using it also explains a mnemonic
that had been bothering me in two functions: the target emits ble after
cmplwi r0, 0 where we emitted beq. mTextureNum > 0 canonicalises to != 0 and
gives beq; the accessor's 0 < mTextureNum, with the constant on the left, does not
canonicalise and gives ble. Same test, just which side the zero is on.

waterGun is read before the rect copy, not after -- the target's
lwz r31, 0x3e4(r5) sits between the copy's argument setup and the bl, and a load
cannot be scheduled across a call.

processAppearStar tests the shine count first. I originally rejected
(shines > 100 && unk50) as a fakematch and was wrong; the target's branch layout
tests the count before the flag. Short-circuit order is semantically meaningful and
directly visible in the object, so it is not the same kind of change as swapping
commutative operands.

The coin emitters go through the existing helper. processDownCoin and
processAppearCoin had the centring written longhand next to a
setEmitterToPaneCenter that does exactly that. processAppearStar keeps its longhand
because all four variants measured worse there (90.27 / 90.27 / 90.75 / 90.53 against
91.20), which I cannot explain and am flagging rather than papering over.

setTimer had two logic holes: the non-sentinel path never assigned timerValue,
so the argument was silently dropped, and the field was then written back from the raw
argument rather than the clamped value. checkChangeTelopArray selected the wrong
two Dolpic news tables -- the 5:0001 && 5:0002 branch takes 5_4 and the neither-flag
branch takes 5_1. That fix is at the call sites deliberately: permuting the table
definitions corrects the code offsets but breaks .data symbol ordering, so it fixes
one thing and breaks another.

startDisappearCoin hid its two panes with two different spellings of the same
arithmetic; retail added the height before the + 1 and we added the + 1 first.

---

Measured and rejected

Stating these so nobody repeats them:

• JUTRect getInitialBounds() by value -- 8099 -> 8091, 28 -> 21.
• Applying getOffsetForAboveScreen to all ten -(y2 + 1) sites -- startAppearCoin
  falls 100% -> 81%.
• Routing plain 465 - y1 through getOffsetForBelowScreen -- costs a match.
• A getTextureNum() accessor returning int, to try to get drawWater's ble the
  same way getTexture does -- 99.12% -> 98.85%.
• Clamping drawWater's y as a ternary at the use site rather than an in-place if.
  The target keeps y and the passed value in separate registers, which is what a
  ternary produces, but it measured 99.12% -> 95.83%. The if is right and the extra
  mr comes from somewhere else.
• Hoisting drawWater's left/right to locals of any type -- that is what the two
  spurious float register saves were.
• Moving drawWater's height/topDiff statics below the GXSetChanAmbColor call.
  This also zeroes the .sdata2 offset and looks like a tidy declaration-order fix.
  It is not -- it just displaces the section head by 4 bytes, the same 4 that the
  SMS_NO_MEMORY_MESSAGE const below accounts for. I committed it, found the real
  cause, and dropped it. Neither is in this PR, and this TU's .sdata2 is therefore
  still off by 4.
• Nesting endCameraDemo's body inside the unk50 test, which is what the TODO in that
  function predicts. The compiler collapses it to the same single beq, so it buys
  nothing and costs a 40-line body indented two levels. The TODO stands.
• Const-qualifying SMS_NO_MEMORY_MESSAGE in System/DummyStrings.hpp. I had this in
  the PR and have removed it -- flagging it because it looked right and was not. That
  symbol appears in mario.MAP 293 times and is in .sdata2, the small const section,
  in every one; we declare static const char*, a mutable pointer, which lands in
  .sdata. Adding the second const does put it where the map says, and it cleans up this
  TU's data sections. But DummyStrings.hpp is included very widely, and across the tree
  it costs 4,416 bytes of matched data for zero functions -- 315,187 -> 310,771 --
  breaking .rodata in MapStaticObject, MapObjFloat and MapObjSirena and .sdata in
  NpcInitPrg and MapMirror, all of which were at 100%. So the map is right about where the
  symbol ends up and the one-word change is still the wrong way to get there; something
  else in that header is carrying the difference. I only checked matched functions
  before pushing, which is how it got in.
• Removing the duplicate <System/DummyStrings.hpp> include (no match change, .rodata
  goes from matching to a uniform +32) and removing <M3DUtil/InfectiousStrings.hpp>
  (four sections differ instead of two, 185 bytes of compiler-generated constants
  vanish). Both includes are correct.

---

What I am still stuck on

I classified all remaining failures by padding out each frame delta so the diff was
readable, then splitting what was left into three kinds: a real difference (different
opcode, different immediate, an instruction on one side only), register-allocation
permutation, and stack-slot displacement. The pad was removed before committing;
nothing like it survives in this branch.

Frame size only, no real instruction differences -- checkChangeTelopArray,
processAppearLife, startAppearLife, startDownLeftBot, startInsertLife,
processAppearBalloon, processDisappearBalloon, pauseOut, processDrawTelop,
entryHelpActor, processMoveNozzle, startAppearTelop, startAppearTank,
startAppearStar.

checkChangeTelopArray is the frustrating one: 99.94%, every instruction identical,
and 48 bytes of stack unaccounted for with no mInitialBounds use to explain it.
processAppearLife's frame is already the right size -- its three JUTPoint
temporaries just sit 4 bytes higher than retail's, so retail has one more 4-byte local
than we do and I cannot work out what it is. startDownLeftBot and entryHelpActor
are the other direction, 16 and 8 bytes too big.

I deliberately stopped rather than guess at these. processDrawTelop needs +24 and
already carries an unused textBounds local; getting there means inventing two more
locals, which is the line I am not crossing.

Real code differences left, in the ones I worked this round:

• drawWater 99.12% -- frame 72 short. The two texture-count tests still emit beq
  where the target emits ble after the same cmplwi r0, 0. Both calls there are
  guarded rather than ternaries, so getTexture does not fit and I have not found the
  spelling that does.
• drawWaterBack 87.20% -- frame 32 short with one extra saved GPR; the target
  re-reads bounds.y1 from the stack at each use where we cache it. It also compares
  < 15 and < 25 signed but < 10 unsigned, on the same register in the
  same chain. Mixing the spellings in source to reproduce that would make the chain
  read arbitrarily, so I left the one instruction wrong rather than write it that way.
• processAppearStar 95.99% -- frame 56 short, and blueCoinValue gets an extra
  mr r25, r0 because the target allocates the subtraction straight into the register
  already holding blueCoins. Collapsing the two into one variable produces that, but
  the name would then be wrong for what it holds.
• startAppearBalloon 94.17% -- the unk3E0 == unk3E0 term (already commented as a
  probable copy-paste slip in the original) is folded away by our compiler but survives
  in the target as cmplw r4, r4, so retail's two operands must have been textually
  different expressions that CSE'd to one load. I could not find a spelling that keeps
  the compare without inventing something.
• loadAfter 95.24% -- frame 352 short. The target re-evaluates
  (int)(value * 0.01f) where we common-subexpression it, and re-loads the member from
  memory in between, which says its operand is a memory lvalue there rather than
  something held in a register. I do not have the shape yet.
• perform 27.94% -- 3634 real instruction differences. Nowhere near aligned, so its
  percentage is not measuring anything useful. It went down 0.15% in this PR from the
  getContentsBounds change, which affects a helper inlined into it; I would rather not
  make that line read worse to chase noise in a function that is this far off.

Three things I could not resolve at all:

1. The four UNUSED functions in this TU -- changeNum (312 bytes),
   startDisappearLife (240), resetMoveTank (224), startUpLeftBot (148) -- are
   still empty stubs. You said UNUSED functions have usually been inlined into other
   functions rather than being genuinely dead, so I went looking. For these four it does
   not hold: they are absent from the extracted retail object entirely, their MAP
   addresses are ........ so they were never linked, and no surviving function in the
   unit calls anything we do not already call. There is no disassembly to read and no
   call site to read them from, so anything I write is invention. If you know a build
   where these are linked, that would unblock them.

2. This TU's .sdata is untouched by this PR and still wrong: two symbols retail
   does not have, dummyMactorStringValue1 and SMS_NO_MEMORY_MESSAGE, and every shared
   symbol at a uniform -8. dummyMactorStringValue1 appears in mario.MAP zero times
   -- it is ours, not retail's, added to force a 12-byte null literal into .rodata.
   Removing it fixes this TU and breaks .rodata in about twenty others. As above, the
   const fix for the other symbol is also a net loss tree-wide. Both need solving in
   DummyStrings.hpp itself, by someone who knows what that header originally was -- the
   comment in it still says nobody does.

3. Two raw offset casts remain in this file: + 0x68 off unkC4 and + 0xCC off
   the current nozzle. unkC4 searches for the Peach actor, and unkBC/unkC0 next to
   it are properly typed TBathtub*/TBossEel* -- but Peach's class is not decompiled
   anywhere in the tree, so I cannot type it without guessing. Leaving them ugly and
   visible rather than dressing them up.

---

Verification

mario.dol: OK. Whole-tree clang-format clean. Symbol order clean on the changed file.
CRLF preserved.

Checked project-wide, not just this unit: 8098 -> 8104 functions, and matched data
unchanged at 315,187 bytes. The +6 is exactly this unit's +6. I am calling out the
data figure specifically because I did not check it the first time round, which is how
the SMS_NO_MEMORY_MESSAGE regression got in.

J2DWindow.hpp is the one header outside GC2D that this touches. It has three callers,
all in GCConsole2.cpp.

main was merged in rather than rebased, so the changed-file set is just this file plus
the two headers.

Correction to the first version of this PR: it also contained the
SMS_NO_MEMORY_MESSAGE const change described under "measured and rejected" above. The
progress report caught five broken data matches in other units; I have measured it,
confirmed the cause and dropped the commit.

[decomp-dev]

Report for GMSJ01 (39ad624 - 172258c)

📈 Matched code: 37.53% (+0.08%, +2980 bytes)

✅ 6 new matches

Unit	Item	Bytes	Before	After
mario/GC2D/GCConsole2	TGCConsole2::startDisappearCoin()	+14	98.19%	100.00%
mario/GC2D/GCConsole2	TGCConsole2::startDisappearTimer()	+5	98.61%	100.00%
mario/GC2D/GCConsole2	TGCConsole2::startDisappearStar()	+3	99.70%	100.00%
mario/GC2D/GCConsole2	TGCConsole2::startAppearCoin()	+1	99.86%	100.00%
mario/GC2D/GCConsole2	TGCConsole2::startInsertJetBalloon()	+1	99.86%	100.00%
mario/GC2D/GCConsole2	TGCConsole2::startAppearRedCoin()	+1	99.84%	100.00%

📈 15 improvements in unmatched items

Unit	Item	Bytes	Before	After
mario/GC2D/GCConsole2	TGCConsole2::drawWaterBack()	+268	74.67%	87.20%
mario/GC2D/GCConsole2	TGCConsole2::drawWater(J2DOrthoGraph&)	+250	83.82%	99.12%
mario/GC2D/GCConsole2	TGCConsole2::loadAfter()	+147	92.33%	95.24%
mario/GC2D/GCConsole2	TGCConsole2::processAppearStar(int)	+87	91.20%	95.99%
mario/GC2D/GCConsole2	TGCConsole2::processDownCoin(int)	+44	93.91%	98.88%
mario/GC2D/GCConsole2	TGCConsole2::setTimer(long)	+41	96.12%	99.80%
mario/GC2D/GCConsole2	TGCConsole2::processAppearCoin(int)	+27	95.80%	98.86%
mario/GC2D/GCConsole2	TGCConsole2::startCameraDemo()	+12	97.78%	98.16%
mario/GC2D/GCConsole2	TGCConsole2::startAppearBalloon(unsigned long, bool)	+12	92.46%	94.17%
mario/GC2D/GCConsole2	TGCConsole2::startDownLeftBot()	+10	98.68%	99.78%
mario/GC2D/GCConsole2	TGCConsole2::pauseOut()	+2	99.49%	99.72%
mario/GC2D/GCConsole2	TGCConsole2::startAppearTank()	+1	99.08%	99.32%
mario/GC2D/GCConsole2	TGCConsole2::load(JSUMemoryInputStream&)	0	99.66%	99.66%
mario/GC2D/GCConsole2	TGCConsole2::checkChangeTelopArray()	0	99.93%	99.94%
mario/GC2D/GCConsole2	TGCConsole2::processDisappearBalloon()	0	99.72%	99.89%

📉 3 regressions in unmatched items

Unit	Item	Bytes	Before	After
mario/GC2D/GCConsole2	TGCConsole2::perform(unsigned long, JDrama::TGraphics*)	-54	28.24%	27.94%
mario/GC2D/GCConsole2	TGCConsole2::endCameraDemo()	-8	99.14%	97.84%
mario/GC2D/GCConsole2	TGCConsole2::startAppearStar()	-7	99.76%	99.01%

[Mrkol]

I don't feel like these could ever be real so lets leave them out of the PR for now