Fix heap corruption due to skybox code using GameState as PlayState (#7045)

Also corrects clearing skybox context before Skybox_Setup

soh/soh/Enhancements/Graphics/Disable2DBackgrounds.cpp

@@ -85,8 +85,12 @@ std::set<SkyboxId> skyboxIdControlList = {
 };
 
 void Register3DPreRenderedScenes() {
+    // Runs after the scene commands have set play->skyboxId, but before Play_InitEnvironment builds the
+    // skybox. Overriding the id here means Skybox_Setup loads a real sky; overriding it any later would
+    // leave the display lists pointing at the pre-rendered skybox's texture slots.
     COND_HOOK(AfterSceneCommands, CVAR_VALUE, [](int16_t sceneNum) {
-        if (!skyboxSceneControlList.contains(static_cast<SceneID>(sceneNum))) {
+        if (!skyboxSceneControlList.contains(static_cast<SceneID>(sceneNum)) &&
+            !skyboxIdControlList.contains(static_cast<SkyboxId>(gPlayState->skyboxId))) {
             return;
         }
 
@@ -120,15 +124,6 @@ void Register3DPreRenderedScenes() {
     });
 
     COND_VB_SHOULD(VB_DRAW_2D_BACKGROUND, CVAR_VALUE, { *should = false; });
-
-    COND_VB_SHOULD(VB_LOAD_SKYBOX, CVAR_VALUE, {
-        if (!gPlayState || !skyboxIdControlList.contains(static_cast<SkyboxId>(gPlayState->skyboxCtx.skyboxId))) {
-            return;
-        }
-
-        gPlayState->skyboxCtx.unk_140 = 0;
-        *should = false;
-    });
 }
 
 static RegisterShipInitFunc initFunc(Register3DPreRenderedScenes, { CVAR_NAME });

soh/soh/Enhancements/game-interactor/vanilla-behavior/GIVanillaBehavior.h

@@ -3193,14 +3193,6 @@ typedef enum {
     // - None
     VB_DRAW_2D_BACKGROUND,
 
-    // #### `result`
-    // ```c
-    // CVarGetInteger(CVAR_ENHANCEMENT("3DSceneRender"), 0)
-    // ```
-    // #### `args`
-    // - None
-    VB_LOAD_SKYBOX,
-
     // true
     // ```
     // #### `args`

soh/src/code/graph.c

@@ -28,7 +28,6 @@ FaultClient sGraphFaultClient;
 CfbInfo sGraphCfbInfos[3];
 FaultClient sGraphUcodeFaultClient;
 
-void Skybox_Setup(PlayState* play, SkyboxContext* skyboxCtx, s16 skyboxId);
 void PadMgr_ThreadEntry(PadMgr* padMgr);
 
 // clang-format off
@@ -438,7 +437,6 @@ extern void ProcessSaveStateRequests(void);
 static void RunFrame() {
     u32 size;
     char faultMsg[0x50];
-    static bool hasSetupSkybox = false;
 
     switch (runFrameContext.state) {
         case 0:
@@ -469,14 +467,6 @@ static void RunFrame() {
         }
         GameState_Init(gGameState, runFrameContext.ovl->init, &runFrameContext.gfxCtx);
 
-        // Setup the normal skybox once before entering any game states to avoid the 0xabababab crash.
-        // The crash is due to certain skyboxes not loading all the data they need from Skybox_Setup.
-        if (!hasSetupSkybox) {
-            PlayState* play = (PlayState*)gGameState;
-            Skybox_Setup(play, &play->skyboxCtx, SKYBOX_NORMAL_SKY);
-            hasSetupSkybox = true;
-        }
-
         uint64_t freq = GetFrequency();
 
         while (GameState_IsRunning(gGameState)) {

soh/src/code/z_vr_box.c

@@ -2,6 +2,7 @@
 #include "vt.h"
 
 #include <stdlib.h>
+#include <string.h>
 #include <assert.h>
 
 #include "z64environment.h"
@@ -452,23 +453,17 @@ void func_800AF178(SkyboxContext* skyboxCtx, s32 arg1) {
 
 void LoadSkyboxTex(SkyboxContext* skyboxCtx, int segmentIndex, int imageIndex, char* tex, int width, int height,
                    int offsetW, int offsetH) {
-    if (GameInteractor_Should(VB_LOAD_SKYBOX, true)) {
+    skyboxCtx->textures[segmentIndex][imageIndex] = tex;
-        skyboxCtx->textures[segmentIndex][imageIndex] = tex;
-    }
 }
 
 void LoadSkyboxTexAtOffset(SkyboxContext* skyboxCtx, int segmentIndex, int imageIndex, char* tex, int width, int height,
                            int offset) {
-    if (GameInteractor_Should(VB_LOAD_SKYBOX, true)) {
+    skyboxCtx->textures[segmentIndex][imageIndex] = tex;
-        skyboxCtx->textures[segmentIndex][imageIndex] = tex;
-    }
 }
 
 void LoadSkyboxPalette(SkyboxContext* skyboxCtx, int paletteIndex, char* palTex, int width, int height) {
-    if (GameInteractor_Should(VB_LOAD_SKYBOX, true)) {
+    skyboxCtx->palettes[paletteIndex] = palTex;
-        skyboxCtx->palettes[paletteIndex] = palTex;
+    skyboxCtx->palette_size = width * height;
-        skyboxCtx->palette_size = width * height;
-    }
 }
 
 static const char* sSBVRFine0Tex[] = { gSunriseSkybox1Tex, gSunriseSkybox2Tex, gSunriseSkybox3Tex, gSunriseSkybox4Tex,
@@ -628,6 +623,18 @@ void Skybox_Setup(PlayState* play, SkyboxContext* skyboxCtx, s16 skyboxId) {
             LoadSkyboxTexAtOffset(skyboxCtx, 0, 4, gSunsetOvercastSkybox5Tex, 128, 128, 0x8000);
 
             LoadSkyboxPalette(skyboxCtx, 0, gSunsetOvercastSkyboxTLUT, 16, 8);
+
+            // SOH [Port] This skybox is drawn by the two-segment sky path, so its display lists reference
+            // segment 1 and SkyboxDraw_Draw loads palettes[1], but vanilla only ever DMAs segment 0 here.
+            // The blend is always 0 for this skybox, so mirroring segment 0 keeps the vanilla look while
+            // giving the second segment real data to point at.
+            LoadSkyboxTexAtOffset(skyboxCtx, 1, 0, gSunsetOvercastSkybox1Tex, 128, 64, 0x0);
+            LoadSkyboxTexAtOffset(skyboxCtx, 1, 1, gSunsetOvercastSkybox2Tex, 128, 64, 0x2000);
+            LoadSkyboxTexAtOffset(skyboxCtx, 1, 2, gSunsetOvercastSkybox3Tex, 128, 64, 0x4000);
+            LoadSkyboxTexAtOffset(skyboxCtx, 1, 3, gSunsetOvercastSkybox4Tex, 128, 64, 0x6000);
+            LoadSkyboxTexAtOffset(skyboxCtx, 1, 4, gSunsetOvercastSkybox5Tex, 128, 128, 0x8000);
+
+            LoadSkyboxPalette(skyboxCtx, 1, gSunsetOvercastSkyboxTLUT, 16, 8);
             break;
         case SKYBOX_MARKET_ADULT:
             skyboxCtx->unk_140 = 1;
@@ -896,6 +903,14 @@ void Skybox_Init(GameState* state, SkyboxContext* skyboxCtx, s16 skyboxId) {
     skyboxCtx->unk_140 = 0;
     skyboxCtx->rot.x = skyboxCtx->rot.y = skyboxCtx->rot.z = 0.0f;
 
+    // SOH [Port] On N64 these are segment bases that are always mapped, but here they are raw texture
+    // pointers living in the PlayState. That memory is freed and re-allocated on every scene load, so any
+    // slot Skybox_Setup does not fill would otherwise be drawn from the previous scene's pointers (or from
+    // uninitialized arena memory on the first load).
+    memset(skyboxCtx->textures, 0, sizeof(skyboxCtx->textures));
+    memset(skyboxCtx->palettes, 0, sizeof(skyboxCtx->palettes));
+    skyboxCtx->palette_size = 0;
+
     Skybox_Setup(play, skyboxCtx, skyboxId);
     osSyncPrintf("\n\n\n＊＊＊＊＊＊＊＊＊＊＊＊＊＊＊＊＊＊＊＊\n\n\n"
                  "ＴＹＰＥ＝%d"