search

HarbourMasters / Shipwright

3.29k stars 492 forks source link

Add freeze timer hook for Gold Skulltula collection #4445

Closed JordanLongstaff closed 1 month ago

JordanLongstaff commented 1 month ago

Fixes #4409

Transferred the freeze timer handling for Gold Skulltula tokens to the VB hook, which now reads the "No Skulltula Freeze" enhancement option.

https://github.com/user-attachments/assets/b7fe942f-abfc-42cc-bd62-dc29a99c1f9e

Build Artifacts

JordanLongstaff commented 1 month ago

I don't know where to discuss this (I'd do so on Discord if I knew which thread to use), but could we rename the func_** functions in z_en_si.c for clarity? One in particular, func_80AFB950, appears to specifically be the handler for removing the target reticle from a Gold Skulltula token when it's collected.

Malkierian commented 1 month ago

You can discuss anything to do with development in the soh-development channel. As for function names, I'd look at decomp first to see if they have new names, there's a lot of documentation we haven't pulled over. If they do, then port those changes over. If not, I think I'd be OK with renaming them, just leave the decomp names in a comment next to the definitions so we can reference future changes if they update docs on their end. However, that probably should be in a separate PR.

JordanLongstaff commented 1 month ago

You can discuss anything to do with development in the soh-development channel.

Ohhh. I didn't see that channel in my menu. I had to go to "Channels & Roles" to find it.

However, that probably should be in a separate PR.

I agree with that. 👍

Malkierian commented 1 month ago

I think we need to invert that hook. We want to avoid moving any vanilla behavior out of its vanilla location, and instead handle the should that's passed to the hooks as a way of telling vanilla what to do and when.

JordanLongstaff commented 1 month ago

Should we have a separate VB flag for the freeze timer versus giving the Gold Skulltula Token? Because I can make just the freeze timer dependent on that flag, but everything else (the fanfare, the message, the actual acquiring of the token) should happen regardless.

Malkierian commented 1 month ago

What you have seems good, but I think we used to have an enhancement for skipping the message and fanfare. Those being included in the SHOULD_GIVE I believe was an attempt to make that automatic with randomizer.