Add Code Guidelines

[Pepe20129]

Add docs/CODE_GUIDELINES.md, this is a draft PR as there's still a lot of discussion to be had.

Currently the line about using libultra types over std ones is commented out as there's no consensus on it.

Github slightly messed up the rendering in the PR view, here's the correct rendering.

Build Artifacts

• soh-windows.zip
• soh-mac.zip
• soh-linux.zip

[bassdr]

@Pepe20129 The Original Source rule covers most of soh/src/ but not all of it, and the gap is two patterns that keep merging.

Counts in soh/src/ today:

• 742 GameInteractor_Should across 184 files — the rule as written
• 385 CVAR_ENHANCEMENT gates across 94 files — inline, not hooks: MirroredWorld, DisableLOD, DisableBlackBars, FastFarores, DisableDrawDistance, FixCameraDrift, FixCameraSwing, FixHangingLedgeSwingRate, …
• ungated pure bugfixes: Fix Gold Skulltula tokens overwriting the Bow inventory slot #7021, Restore the realId assignment in AudioLoad_AsyncLoadInner #7105, fix: seqLoadStatus OOB reads/writes for custom SAF sequences (#5706) #6917, Drop the seqLoadStatus bounds checks made redundant by #6932 #7100 — all merged, all soh/src/ only

Under "aside from removing differences previously introduced", those 385 read as debt to be deleted. I don't think that's the intent.

Suggest naming the three cases instead:

1. Port bug the original doesn't have (compiler change, OOB, uninitialised read) — inline, no gate. Nobody wants the broken behaviour back.
2. Original-game bug — inline behind CVAR_ENHANCEMENT("Fix…") defaulting to vanilla, only when there is no VB point to hook. Speedrunners and purists want the shipped behaviour available.
3. Everything else — soh/soh/, GameInteractor_Should or a hook; decomp only fires it.

2 is the one worth being explicit about, since the right answer is usually 3 — FixVineFall, FixFloorSwitches, FixSawSoftlock and FixFlexDrops are all COND_VB_SHOULD handlers with no decomp edit at all.

[serprex]

Original-game bug — inline behind CVAR_ENHANCEMENT("Fix…") defaulting to vanilla, only when there is no VB point to hook. Speedrunners and purists want the shipped behaviour available.

this is outdated, see 2ship or LightHouse for where we'd like to be, where they avoid direct CVar usage in decomp entirely

[bassdr]

Agreed, dropping 2.

That leaves: port bug the original doesn't have → inline, no gate; everything else → VB point in soh/soh/. The 385 CVAR_ENHANCEMENT gates in soh/src/ are then legacy to migrate, not a case to document.

Worth stating that positively in the section — "no direct CVar usage in src/" is a stronger and clearer rule than the current wording.

[Pepe20129]

Port bug the original doesn't have (compiler change, OOB, uninitialised read) — inline, no gate. Nobody wants the broken behaviour back.

Stuff that is UB should ideally be documented upstream (they have an AVOID_UB define for stuff like this).
Other things should still be handled with vb hooks, look at the AlwaysOnFixes.cpp file for examples.

Worth stating that positively in the section — "no direct CVar usage in src/" is a stronger and clearer rule than the current wording.

I believe that

... these guidelines have not always been here so some code has not been updated to follow these guidelines yet. ...

and

... the only changes that should be done (aside from removing differences previously introduced) are the addition of GameInteractor_Should calls ...

is enough to convey that.

[serprex]

relevant: #7149

[Pepe20129]

relevant: #7149

thanks

[bassdr]

One case the Original Source section has no category for: streamed audio.

NoteSynthesisState.opusFile and the aOPUSFree call in Audio_NoteDisable have been in soh/src/ since #5457, the 2ship streamed-audio backport. They are not GameInteractor_Should calls and cannot be — there is no vanilla behaviour to gate, since vanilla has no OPUS. A per-note decoder handle has to live with the note.

As written the clause forbids something already merged and shipping. Is the intent that port-only features carrying per-note state are a third category, or that they should move out of src/ entirely?

[Pepe20129]

One case the Original Source section has no category for: streamed audio.

NoteSynthesisState.opusFile and the aOPUSFree call in Audio_NoteDisable have been in soh/src/ since https://github.com/>HarbourMasters/Shipwright/pull/5457, the 2ship streamed-audio backport. They are not GameInteractor_Should calls and cannot be — there is no vanilla behaviour to gate, since vanilla has no OPUS. A per-note decoder handle has to live with the note.

As written the clause forbids something already merged and shipping. Is the intent that port-only features carrying per-note state are a third category, or that they should move out of src/ entirely?

Again, the idea is that the only changes in the src/ directory are GameInteractor_Should related, for the case of streamed audio, there are multiple options, the ones that come to mind are:

• Using GameInteractor_Should calls as hooks all over the audio code
• Redirecting audio playback to a new system outside src/ with one GameInteractor_Should at the creation of the audio thread

As for the fact that it's already merged and shipping, the ... these guidelines have not always been here so some code has not been updated to follow these guidelines yet. ... note is there for a reason.

[bassdr]

Follow-up on the same section: does the rule require literally GameInteractor_Should, or is it the shape that matters?

#6668 does what your second option describes. Audio generation lives entirely in soh/soh/, and the only decomp change is two #if-guarded extern calls out to it:

extern bool SOH_MidiTranslator_ProcessNote(...);
extern void SOH_MidiTranslator_NoteDisabled(int noteIndex);

28 of its 29 files are port code and nothing is decided in src/ — decomp only fires the call. If that counts, it may be worth naming as the pattern. If it has to be GameInteractor_Should, I would rather convert the two now than after review.

[serprex]

SOH_MidiTranslator_ProcessNote pattern is fine