ShipInit & Conditional Hooks

[garrettjoecox]

ShipInit

This axes our need to have these big register funcs/headers everywhere for our modules that just need to be initialized on startup, along with an added benefit: When you register your InitFunc you can specific N paths (strings) associated with that function, and then at any time you can re-invoke initFuncs associated with that path.

// Before
// in enhancements.h & make sure it's in alphabetical order!
void RegisterMyModule();
// in enhancements.cpp & make sure it's in alphabetical order!
RegisterMyModule();
// in BenMenuBar
if (UIWidgets::CVarCheckbox("My Module", "MyThing")) {
  RegisterMyModule();
}

// After
// At the bottom of your MyModule.cpp
static RegisterShipInitFunc initFunc(RegisterMyModule, { "MyThing" });
// in BenMenuBar
UIWidgets::CVarCheckbox("My Module", "MyThing");

// If you want to re-run for any other reason than a UIWidget changing:
// ShipInit::Init("MyThing");

Most people won't need to manually call these InitFuncs anywhere, because it's now built right into our UIWidgets! Any time any CVar associated UIWidget is changed, we invoke all InitFuncs associated with that cvar string. This works extremely well with our next addition:

Conditional Hooks

These are already in use in Anchor, Let it Snow & Dice. They are shortcuts for the common pattern of unregistered your hook and re-registering it based on a condition. This is an important optimization for expensive hooks, but should basically be used for 90% of cases going forward.

// Before
static HOOK_ID hookId = 0;
GameInteractor::Instance->UnregisterGameHookForID<GameInteractor::ShouldActorUpdate>(hookId);
hookId = 0;
if (CVAR) {
  hookId = GameInteractor::Instance->RegisterGameHookForID<GameInteractor::ShouldActorUpdate>(ACTOR_ID, MyHandler);
}

// After
COND_ID_HOOK(ShouldActorUpdate, ACTOR_ID, CVAR, MyHandler);

This PR converts masks & minigames so far, will do the rest if we agree this is a good pattern.

Additionally, I began using a macro pattern for CVar names and gets, simply to reduce the friction when copy/pasting code from one enhancement file to another. This is entirely separate from the systems above and not a requirement of them. Can easily undo these if people don't like them.

#define CVAR_NAME "gEnhancements.Minigames.AlwaysWinDoggyRace"
#define CVAR CVarGetInteger(CVAR_NAME, ALWAYS_WIN_DOGGY_RACE_OFF)

Build Artifacts

• 2ship-linux.zip
• 2ship-mac.zip
• 2ship-windows.zip

[Pepe20129]

I don't think this is a good idea, it works due to "magic" (you just need to know that declaring a static RegisterShipInitFunc does stuff) and it feels way too abstracted from the game, this will probably be a barrier to newcomers adding hooks as they'd have to learn a complicated system (this also applies to VB but I think in that case it's worth it given the change in original code impact, whereas this is just a different "front-end" to hooks).

[garrettjoecox]

it works due to "magic"

I will agree with this point. I usually tend to have an aversion to magic like this, but feel it outweighs the current alternative (the big register blocks)

this will probably be a barrier to newcomers adding hooks as they'd have to learn a complicated system

If you're referring to RegisterShipInitFunc, it isn't really tied to hooks, nor does it enforce the usage of hooks, it's just a way to register init functions that will run at boot (and can be run later if desired). Newcomers will either copy this pattern at the bottom of their file (which I think is intuitively named) or they'll copy the pattern of adding their register function to a big block in enhancements/*.h/cpp. The former happens in the file they are already in, the latter touches 2 additional files.

If you are referring to the Conditional hooks pattern, my goal will be to slowly move all hook usage to this pattern regardless of this PR, because we don't want code (especially expensive code) running when the user hasn't even opted into a given option. The primary reason it's not already enforced is because it's tedious to write the code for it (see the before block in the description).

[garrettjoecox]

It might also be worth calling out, I'll add to the description, but here I also began using macros for cvar names and cvarGets, none of this is necessary for this PR or the new systems, just a pattern I've begun using because it's more copy/paste friendly (for people copying hooks or something from one file to another)

#define CVAR_NAME "gEnhancements.Minigames.AlwaysWinDoggyRace"
#define CVAR CVarGetInteger(CVAR_NAME, ALWAYS_WIN_DOGGY_RACE_OFF)

[Archez]

Ok so I think my thoughts on this are:

ShipInit

Pros

• Reduces footprint of an enhancement to one file plus the UIWidget element
  • Less changes for an enhancement PR to have to resolve merge conflicts from other enhancements merging before it
• Having the enhancement encapsulated in one file means renaming/moving around the enhancement is simpler/doesn't require touching a lot of files.
• Un/Re-registering enhancements due to various triggers is now easier/less likely to experience dev errors (if someone forgot to call a register func)
  • E.g. dragging a settings.json onto the game window to load new settings, modifying the cvar via debug console, or an eventual preset system.

Cons

• Relies on "magic", but arguably to me its not "really" that magical. Its just functions being tracked in a string keyed structure.
• The re-triggering of funcs is only against explicit strings. If we want to expand it to support a globbing or namespacing pattern, I imagine it would complicate the mapping/iterating structures.
  • At least any change would be isolated to ShipInit.hpp itself.
• There doesn't seem to be any control on "order" in which the init funcs will be executed. Presumably the order is determined by the linker/runtime execution of when each file is statically loaded.
  • Currently, no enhancement depends on another enhancement being init prior to it, so there isn't an issue today. We probably just need to maintain that moving forward, essentially init funcs should be idempotent (as needed) and order independent.

Hook Macros

Pros

• Reduces boiler plate
• Guides contributors to design un/re-register patterned hooks first without having to think about it
• Existing hook syntax is still available for more complex scenario needs (hooks unregistering other hooks, etc)

Cons

• Having the function body block "inside" the macro args prevents break-point debugging. This has generally annoyed me, and will be increased further with more macro helper options.
  • A workaround for this is defining the body logic in a separate function declaration, then have that function called within the hook macro. This way the separate function can trip break-points. Generally I try to do this with hooks I create if its more than a couple lines of code.
• Obscures available parameters in the function body.
  • This is more of a discoverability problem for new contributors, and with good documentation as well as intelli-sense this should hopefully not be a big deal.

---

Overall I think this is a good step forward for general dev experience. There is the slight tradeoff with discoverability, but I don't feel like things were really "discoverable" before hand, so it is more like a sideways shift in that regards. Documentation and example patterns will be key regardless.

[garrettjoecox]

Alright the majority of the enhancements have been migrated, only the saving/cycle stuff remaining