Fix ice trap chests lacking smoke by object loading

[djevangelia]

Ice trap chests load OBJECT_FZ on spawn, if unopened. Fixes #1912.

For ice traps to have smoke effect the object OBJECT_FZ (related to Freezard actor) must be loaded. Objects are normally loaded on room-to-room basis depending on the room's object list. Vanilla ice traps are thus in rooms that load this object. Randomized ice trap chests will not have the smoke effect unless they spawn in a room that has OBJECT_FZ loaded for some reason (because in vanilla it has Freezards, ice traps, or other reason).

Non-persistent objects are loaded on room load from this object list, and cleared on next room load. Max number is 19. The object context is updated every frame.

This fix makes the chest actor EnBox load the OBJECT_FZ in EnBox_Init if its new get item ID is GI_ICE_TRAP and it has not been opened before.

• Run new function Object_LoadExtra to find if OBJECT_FZ is already loaded, or else, an available object slot
• Use vanilla function func_800982FC to fill the slot with data
• On next object context update, the object will be loaded
• If already max objects, it simply won't be loaded (like now)
• When room unloads, the object is unloaded naturally

Object_LoadExtra is not restricted to chests, it can be called by anything that needs to load an extra object. It's surely possible to make unloading possible too.

Testing

Tested on Ares recent nightly build, Project64 3.0.1 and Mupen64plus 2.8.
Looks like this: https://www.youtube.com/watch?v=bXqP-y3l_TE
The testing number printing code can be found here
(I had a very impactful, sly bug related to object loading (due to a typo) that crashed hard on Ares and Mupen64, but ran fine on Project64 - in case that says anything about bug finding potential in this type of case.)

[flagrama]

Could you use the same seed for GTG and walk in to the maze room from the lava room? If this is done incorrectly the game will crash during the room transition. Also walk back and forth then try to go elsewhere in GTG to make sure it isn't filling up object slots permanently?

[djevangelia]

Could you use the same seed for GTG and walk in to the maze room from the lava room? If this is done incorrectly the game will crash during the room transition. Also walk back and forth then try to go elsewhere in GTG to make sure it isn't filling up object slots permanently?

https://www.youtube.com/watch?v=cevpxJmoG8I

[flagrama]

So that video shows that the object may still not get loaded due to the way en_holl works? That chest you opened in the maze didn't have the effect. Is that part still non-trivial to fix?

[djevangelia]

So that video shows that the object may still not get loaded due to the way en_holl works? That chest you opened in the maze didn't have the effect. Is that part still non-trivial to fix?

Exactly, if the EnBox actors with GI_ICE_TRAP aren't destroyed but objects are unloaded, there will be no fresh spawning ice trap EnBox actor to call Object_LoadExtra in EnBox_Init and reload the object.

It seems mostly like a vanilla quirk of esthetic consequences that probably no one will ever see in actual gameplay. It should be fully possible to load on-demand on chest opening (or ice trap item pickup) and then unload, which would also fix this and would be good for any future new traps that need an object (so they can co-exist in the same room).
But, I did this fix as part of another thing I'm working on, so I will probably not try to do it.

[djevangelia]

Just some further testing - chest ice traps and enemy spawns with object load (and unload) as needed.
The spawning and unloading code is of course not part of this PR, but the object load code is exactly the same.
https://www.youtube.com/watch?v=TwMQRL0m-Bg

[TreZc0]

We should definitely compare this solution to the original proposed solution in #2169 first and see if the occuring crashes are no longer gonna be an issue with the new approach.

[flagrama]

That version of the fix used the function to load the object using the "make this object and everything before it persistent between room loads" function. This creates a new function that only temporarily loads the object. I tried something similar in the on-console randomizer here: https://github.com/flagrama/N64-OoT-Randomizer-Patches/blob/ae72149cbad4bf00ce7081c40e1e9b0c820efa34/src/code/z_scene.c.patch#L9-L34 though somewhat clumsier that does work as long as the object is manually unloaded when the chest despawns. The fact that running back and forth in GTG was tested for this PR means that is handled properly already.

You can still see the problematic function Kirox used referenced in the linked patch before the linked section. That function should only ever be called on scene load, not randomly when opening a chest. The new function is fine.

See #1912 (comment) for my comments on the old PR, but on the bug instead of the PR itself.

[djevangelia]

I took a deeper look into the previous fix now and actually, I wonder if it was the synchronous DMA request causing crashing. (Non-persistent objects on room load, and objects in this fix, load asynchronously through Object_UpdateContext. func_800981B8 also uses synchronous request, when exiting the pause menu.)
The number of persistent objects didn't change, because object_index_or_spawn made sure to re-set the number of loaded persistent objects (n_spawned_objects) directly after loading a new object.
Also, the interrupt hung up that results when it crashes has register values of DMA character (see below, crash entering GTG maze with ice trap chest), but this doesn't show unless you modify Fault_AddHungupAndCrashImpl. I know nothing about DMA, timings, lock-ups, but I can imagine the extra synchronous transfer being less disruptive when transitioning through doors than through En_Holl. @flagrama - if you can still build your fix, maybe try async and see if it works?

I've had one crash recently due to objects, on unloading (because I treated all OBJECT_FZ as ice traps, which would cast Freezards into EnBox...), but loading and unloading has otherwise been stable.

--------- Room switch -> 8 ---------
Chest: Check override base item
Chest: Check override base item
Chest: Check override base item
--> Override base item 0x7c
**** Load object 0x114

*** Objects: 11 - Space free: 203648 (0x31b80)
Slot 00: id 001 size 0x5bce0, seg 80279450
Slot 01: id 003 size 0x17af0, seg 802d5130
Slot 02: id 015 size 0x2cf80, seg 802ecc20
Slot 03: id 04d size 0x08b10, seg 80319ba0
Slot 04: id 02b size 0x08760, seg 803226b0
Slot 05: id 05d size 0x01a40, seg 8032ae10
Slot 06: id 11c size 0x01760, seg 8032c850
Slot 07: id 0a4 size 0x03c90, seg 8032dfb0
Slot 08: id 00e size 0x06000, seg 80331c40
Slot 09: id 154 size 0x067c0, seg 80337c40
Slot 10: id 114 size 0x034d0, seg 8033e400
- total (n_obj): 11 persistent: 3

Chest: Check override base item
Chest: Check override base item
Chest: Check override base item
Chest: Check override base item
Chest: Check override base item

THREAD ID:46875000 (0:Interrupt)
PC:00000002H   SR:000e0204H   VA:803da800H
AT:00000000H   V0:00000000H   V1:80008a60H
A0:80009c10H   A1:80003a90H   A2:800fc0b0H      a0 = part of __Dom2SpeedParam
A3:800089e8H   T0:00000000H   T1:00000000H      a1 = __osPiRawStartDma
T2:00000000H   T3:00000000H   T4:00000000H      a2 = sSsSramContext
T5:0000311eH   T6:03e52239H   T7:00000c15H      a3 = __Dom2SpeedParam
S0:006c02ecH   S1:00000000H   S2:00000400H
S3:000e0204H   S4:00000280H   S5:002501ffH
S6:00000002H   S7:0000311eH   T8:04651e39H
T9:00040c11H   GP:006c02ecH   SP:00000000H
S8:00000400H   RA:000e0204H   LO:00000280H

FPCSR:00013016H  (Invalid operation)

F00:-------------     F02:0.0000000e+00     
F04:0.0000000e+00     F06:-------------     
F08:-------------     F10:-------------     
F12:-------------     F14:0.0000000e+00     
F16:0.0000000e+00     F18:-------------     
F20:-------------     F22:0.0000000e+00     
F24:-------------     F26:2.4934360e-36     
F28:-------------     F30:1.1756020e-38     

[shirosoluna]

@fenhl can you add the needs review status back please, or at least hold on this for now since 9.1 is coming soon.
I am trying to get this done in a timely fashion but life has not been cooperative the past few days. Please give this some time

There are some concerns which I will try here as well but I want to thoroughly test the following:

1. these changes are not tested on VC -- please start adding dolphin for ones that affect memory specifically. Preferably all of them but this one has been more impactful of the more recent PRs

2. object loading and unloading in soft transitions is an actual technique used in glitches. Im not confident this could cause crashing when the room is overloaded from trying to perform one of these glitches. So this is part of something I'd like to test.

Also I dont fully understand, does this actually still crash? or was that only due to the debugger menu?
referring to the crash in the last comment.

[fenhl]

This PR has seen the basic testing to ensure the fix works as intended, plus testing to ensure it doesn't cause the same side effect as the previous attempt. I consider that enough testing for inclusion in Dev. Any further testing can be done while it's on Dev. It will spend an entire release cycle there, which should be more than enough time.

[djevangelia]

I looked into the previous fix GTG crash a final time, confirming that there was no manipulation or extra loading of persistent objects in any way, and that was not an issue at all. The problem was the En_Holl transition, the SoT block and synchronous DMA (same-frame transfer).

The game did not crash with the SoT block not visible (i.e. not drawn), matching the initial (very detailed and helpful) report on Discord. The SoT object was slot 12* in Lava room. When entering Maze room, the object is unloaded before the SoT actor is fully deleted. This is no problem in vanilla because the final object 9 in Maze room does not reach the memory space of slot 12 in Lava room. However, adding OBJECT_FZ as an additional object does overwrite what was previously slot 12. If this is done with synchronous DMA, it is loaded the same frame as the last frame of the SoT block - causing SoT block to try to draw with invalid data.
* it's been a few days since I did this so numbers might be off

As the asynchronous DMA (normally used for non-persistent objects) has a frame delay from taking the object slot to actually DMA loading the object, it does not present any problems. This also explains why the previous fix does work for loading Like Like dropped objects. (Here to the contrary, async load would not work as it is currently set up, because the current fix object load is called from EnItem00 init and the actor's object dependency is initially set to 0 - which is always loaded - so the draw function is already enabled from init. So if loading the shield/tunic object is done async, there is a delay from the real dependency object having an object slot and actually being loaded, which will cause crash on draw.)

---

I did two changes to the function:

• It now returns slot number, in line with the previous fix. This makes the function usable as a combined Object_GetSlot and load object. (However, I decided not to change argument play into objectCtx in line with vanilla functions, because it's quicker for assembly to just pass play than build objectCtx)
• I added an argument uint8_t syncDma: true if the object should be synchronous DMA loaded this frame, else false, as well as code for calling DmaMgr_RequestSync. I'm not adding any actual use of this into the pull request as it is approved already, but I want the arguments to be there already for future use. The purpose is to be able to sync DMA for objects that need to be drawn on the same frame as object for some reason, such as Like Like dropped objects. I'm using this in my Like Like trap build with tunic/shield drops and it works fine. Tested in PR build too just to be sure - as expected, ice trap loading works fine as async anywhere and as sync - when not crashing in the GTG En_Holl with SoT block loaded. (So of course the trap loading still uses async DMA.)

[fenhl]

@rrealmuto still has concerns, I've asked them to talk to you directly.

[fenhl]

Concerns seem to have been resolved, merging.