Emulate Heap Fragmentation by inspectredc · Pull Request #3863 · HarbourMasters/Shipwright

inspectredc · 2024-01-15T22:01:05Z

????????????????????????????

This is a system for simulating how the heap would be allocated on original hardware. Actors are then checked against this heap to see if it would be possible to load the actor on hardware, before it gets loaded. Allocated memory to this heap is all zeroed and not used for any actual data used by the game.

The most common use case for this is the 'stairs' glitch where the Royal Tomb is not loaded after switching between the two 'rooms' on the stairs in the graveyard scene, allowing you to enter the Tomb without Zelda's Lullaby.

Would love any feedback on the system and how I can improve the stairs.cpp in particular (or any better renames).

There are a few things which can be done differently to get a better alignment to console, including a better method for calculating the original heap size to start with. I'm not quite sure how much the skybox, for example, should be allocating. The other shortcoming is that actor struct sizes may differ in SoH than on console, which should only lead to minor differences itself.

I've also added some watches in the Value Viewer since it seemed appropriate and helped me debug a lot.

Build Artifacts

jbodner09 · 2024-01-18T02:52:11Z

@@ -0,0 +1,675 @@
+#include "stairs.h"


Given that this file is basically simulating the entire heap, which could be used for more than just this graveyard use case, I would recommend renaming this file and all associated heap functions that aren't specific to just this use case to something more general, like heap_sim or simulated_heap.

Yes makes sense!

jbodner09 · 2024-01-18T02:53:46Z

+#undef DEFINE_ACTOR
+
+// Original size subtracted by various constant allocations which are always made on init, so those allocations don't need to be made there
+size_t stairsAllocSize = 0x1D4790 - 0x26960 - 0x2200 - (0x55 * 0x60) - (3 * (48 * 16) / 2) - (4 * (32 * 32 * 4)) - 0x1000 - (20 * sizeof(MtxF));


It would be nice to mention what all these magic numbers correspond to so that future uses will know if and how they need to do something similar.

Alright, this section is already something which isnt the most accurate so may replace it with a more correct system anyway

jbodner09 · 2024-01-18T03:03:39Z

        actorCtx->absoluteSpace = NULL;
    }

+    if (CVarGetInteger("gStairs", 0)) {


It would make more sense to me to have all these simulated heap functions controlled by the gSimulateHeap cvar, and then use your gStairs one ONLY for the graveyard use case.

so the "gStairs" cvar is just a silent variable (i.e. the user has no control over it) and then gSimulateHeap cvar will set this on/off at the start of play init. this is so that the heap is not allocated memory when it shouldnt, and that the memory always gets freed properly at the end of the play loop

I understand the reasoning here, however, if this is going to be useful for emulating more than the Royal Tomb Grave unloading glitch, this variable should have a better name.

jbodner09 · 2024-01-18T03:09:46Z


+    // #Region [SoH] Stairs
+    if (CVarGetInteger("gSimulateHeap", 0)) {
+        CVarSetInteger("gStairs", 1);


By placing this in Play_Init, changes to the checkbox in the UI won't apply until the next time you change areas, since that's when Play_Init is run, right? It might be nice to mention that in the tooltip that that checking or unchecking the box won't apply until the next area load.

Yes will add that to the tooltip!

leggettc18 · 2024-01-22T02:00:34Z

Would this cause any issues with how we've been handling (i.e. removing) Object Dependency? I'll need to deep dive before I can properly review this because I don't fully understand it yet, but that question just popped into my head upon reading your description. I'm not sure if it's really connected in anyway at all so if you're not sure the answer is probably no. I'll do a full review at some point soon.

inspectredc · 2024-01-22T02:08:45Z

Nah fortunately this is unrelated to object dependency, this is in regards to the heap rather than the object list that has been loaded. The actors don't actually use this memory anyway, it's all done so that a condition which checks 'do i have space to load this actor' returns as false

inspectredc · 2024-01-22T02:09:17Z

Also thank you @jbodner09 ! I will try and find some time this week to address your review comments

leggettc18 · 2024-01-22T04:56:38Z

Nah fortunately this is unrelated to object dependency, this is in regards to the heap rather than the object list that has been loaded. The actors don't actually use this memory anyway, it's all done so that a condition which checks 'do i have space to load this actor' returns as false

OK cool, and this is behind a toggle right? It looks like it is just making sure. Otherwise I would worry about this potentially limiting modding potential.

inspectredc · 2024-01-22T08:23:17Z

Yeah it has a kind of locking system, so you can toggle it, but the toggle only takes effect when play init happens (i.e. new scene load). otherwise you would need to worry about freeing the heap or issues allocating to a heap with no initialised size

leggettc18 · 2024-01-22T17:03:29Z

Yeah it has a kind of locking system, so you can toggle it, but the toggle only takes effect when play init happens (i.e. new scene load). otherwise you would need to worry about freeing the heap or issues allocating to a heap with no initialised size

OK, as long as that's mentioned somewhere I think that's OK. It wouldn't be the first enhancement that has issues toggling without a scene reload. I'm about to start an actual review but it might take a bit for me to parse it all.

leggettc18

I think this looks reasonable to the best of my understanding, although I have asked for some clarification on naming, a few commented out chunks that need either an explanation for why they are commented or to just be removed, and some magic numbers that I would like some explanation for.

As a followup, not necessarily needed for this PR, would it be possible to enable the heap emulation and visualization, without it actually affecting gameplay? I could see that being useful for research purposes (although I suppose it could still be useful for research without decoupling the visualization from the gameplay). That may not be reasonable or all that useful, just a thought that popped into my head.

leggettc18 · 2024-01-22T17:10:18Z

+Arena sStairsArena;
+
+// using sizes from the debug rom
+std::unordered_map<u16, size_t> actorOverlaySizes = {


To the best of my understanding, this should probably be static const. So static const std::unordered_map.... Assuming you aren't changing this at runtime any. Shouldn't impact functionality, more of a correctness factor for storage/type specification.

leggettc18 · 2024-01-22T17:17:41Z

+    return true;
+}
+
+void* StairsArena_Malloc(size_t size) {


This might just be lack of knowledge on my part, but I don't really understand what the difference is between Malloc, MallocR, MallocGeneral, MallocRGeneral, etc. are. If that's the kind of thing that can be briefly explained in a comment somewhere in here I think that would be good, if not I'll ask you at some point later because it's probably just some deep memory management knowledge that I'm missing.

leggettc18 · 2024-01-22T17:19:28Z

+            UIWidgets::PaddedEnhancementCheckbox("Simulate N64 Heap", "gSimulateHeap", true, false);
+            UIWidgets::Tooltip("This restores heap fragmentation glitches, such as causing the Royal Tomb to not load when reloading its graveyard room.");


I would add a note about requiring a scene reload for this to take effect.

leggettc18 · 2024-01-22T17:20:34Z

+        UIWidgets::EnhancementCheckbox("Stairs heap display", "gStairsDisplay");
+        UIWidgets::Tooltip("Visualises the stair heap");


IIRC there was some talk about this being used for more than just the stairs outside shadow temple, so genericizing this language would be good (or do the other places where this is relevant require a separate visualizer?)

leggettc18 · 2024-01-22T17:23:40Z

        actorCtx->absoluteSpace = NULL;
    }

+    if (CVarGetInteger("gStairs", 0)) {


I understand the reasoning here, however, if this is going to be useful for emulating more than the Royal Tomb Grave unloading glitch, this variable should have a better name.

leggettc18 · 2024-01-22T17:26:09Z


 #include <time.h>
 #include <assert.h>
+// #include <stdlib.h>


If this is truly not needed, should probably delete it instead of commenting it out.

leggettc18 · 2024-01-22T17:27:56Z

+
+    // todo: figure out correct 'size' rather than 0x10000 (seems quite large?)
+    // if (CVarGetInteger("gStairs", 0)) {
+    //     if (skyboxId == SKYBOX_NORMAL_SKY || skyboxId == SKYBOX_OVERCAST_SUNSET || skyboxId == SKYBOX_CUTSCENE_MAP) {
+    //         Stairs_DecreaseSize(0x10000 * 4);
+    //     } else {
+    //         Stairs_DecreaseSize(0x10000 * 2);
+    //     }
+    // }


Delete or explain why this is commented out instead of being deleted

leggettc18 · 2024-01-22T17:28:29Z

+            if (CVarGetInteger("gStairs", 0)) {
+                Stairs_DecreaseSize(8 * 150 * sizeof(Gfx));
+            }
+


Would appreciate some explanation of these magic numbers.

briaguya0 · 2024-02-11T07:59:57Z

i'm not fully following why gStairs checks can't just use gSimulateHeap, but assuming that is required

since gStairs is completely controlled by gSimulateHeap, i'm not sure it makes sense for it to be a cvar (and therefore saved to shipofharkinian.json)

i can't remember where off the top of my head but i'm pretty sure we have some ship specific global vars somewhere, it probably makes sense to follow that pattern for gStairs

Pepper0ni · 2024-11-14T16:37:14Z

Am i right in remembering that this is being put on hold to implement it in a better way? If so it should be closed or marked do not merge.

aMannus · 2025-02-04T10:22:14Z

Going to close this for now. This generally feels like a LOT just to accomodate one or 2 glitches, but if there's no other way, let's revisit this some other point when it's updated again.

serprex · 2026-07-22T21:20:43Z

inspectredc#4

inspectredc added 8 commits January 9, 2024 12:33

STAIRS

cc83811

add missing cvar check

00ff6ae

create more realistic stairAllocSize

b62231a

bugfix and add to value viewer

0afb36d

actually return if overlay cant load

6aa3e3b

cvarri!!

0091b3a

fix windows

0dec8be

actually save the file before i commit

06763b7

jbodner09 suggested changes Jan 18, 2024

View reviewed changes

leggettc18 requested changes Jan 22, 2024

View reviewed changes

aMannus closed this Feb 4, 2025

serprex mentioned this pull request Jul 22, 2026

Stairs #6964

Draft

		UIWidgets::PaddedEnhancementCheckbox("Simulate N64 Heap", "gSimulateHeap", true, false);
		UIWidgets::Tooltip("This restores heap fragmentation glitches, such as causing the Royal Tomb to not load when reloading its graveyard room.");

		UIWidgets::EnhancementCheckbox("Stairs heap display", "gStairsDisplay");
		UIWidgets::Tooltip("Visualises the stair heap");

Uh oh!

Conversation

inspectredc commented Jan 15, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Build Artifacts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leggettc18 commented Jan 22, 2024

Uh oh!

inspectredc commented Jan 22, 2024

Uh oh!

inspectredc commented Jan 22, 2024

Uh oh!

leggettc18 commented Jan 22, 2024

Uh oh!

inspectredc commented Jan 22, 2024

Uh oh!

leggettc18 commented Jan 22, 2024

Uh oh!

leggettc18 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briaguya0 commented Feb 11, 2024

Uh oh!

Pepper0ni commented Nov 14, 2024

Uh oh!

aMannus commented Feb 4, 2025

Uh oh!

serprex commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

inspectredc commented Jan 15, 2024 •

edited by github-actions Bot

Loading