path: root/doc/CONTRIBUTING.md

# Contributing Guidelines

This document outlines the conventions to follow when contributing to this project.

## Commit Policy

### Verify Build and Tests Before Committing

Before preparing or proposing a commit, you **must** perform the following verifications to prevent regressions:

**Automated Verification (Recommended):**
```bash
./scripts/check_all.sh
```
This script automatically:
- Builds with tests and tools enabled
- Runs the full test suite (26 tests)
- Verifies all tools compile (spectool, specview, specplay)
- Cross-compiles for Windows (if mingw-w64 is installed)

**Manual Verification:**

1.  **MacOS / Linux (Native)**:
    *   Build the project with tests and tools enabled:
        ```bash
        cmake -S . -B build -DDEMO_BUILD_TESTS=ON -DDEMO_BUILD_TOOLS=ON
        cmake --build build -j8
        ```
    *   Run the entire test suite:
        ```bash
        cd build && ctest --output-on-failure
        ```
    *   Verify tools compile:
        ```bash
        cmake --build build --target spectool specview specplay
        ```
    *   Ensure all tests pass and all tools build successfully.

2.  **Windows (Cross-Compilation)**:
    *   If `mingw-w64` is installed on your system, you **must** also verify the Windows build.
    *   Run `./scripts/build_win.sh`.
    *   Ensure the build succeeds and produces the `demo64k_packed.exe` binary.
    *   Check the size report to ensure no unexpected bloat.

Refer to the "Testing" and "Windows Cross-Compilation" sections in `HOWTO.md` for detailed instructions.

### Verify Debug Logging Compiles Before Committing

Before any significant commit (especially those modifying audio, rendering, or asset systems), **verify that the debug logging code compiles** without errors. This ensures that the diagnostic code remains maintainable even when not actively used.

To enable all debug logging and verify compilation:
```bash
cmake -S . -B build_debug_check -DDEMO_ENABLE_DEBUG_LOGS=ON
cmake --build build_debug_check -j4
```

The build **must** succeed without errors or warnings. Debug logging is stripped from release builds but preserved for development and troubleshooting.

#### Debug Logging Categories

The project uses conditional debug logging macros defined in `src/util/debug.h`:
- `DEBUG_LOG_AUDIO`: Audio backend timing, callbacks, device configuration
- `DEBUG_LOG_RING_BUFFER`: Ring buffer state, underruns, bounds checks
- `DEBUG_LOG_TRACKER`: Music pattern triggers, sample caching
- `DEBUG_LOG_SYNTH`: Voice management, spectrogram registration
- `DEBUG_LOG_3D`: 3D rendering, camera, scene queries
- `DEBUG_LOG_ASSETS`: Asset loading, procedural generation
- `DEBUG_LOG_GPU`: WebGPU commands, pipeline state

Use these macros instead of direct `fprintf(stderr, ...)` calls in new diagnostic code.

**Example:**
```cpp
#include "util/debug.h"

#if defined(DEBUG_LOG_AUDIO)
  static int callback_count = 0;
  DEBUG_AUDIO("[CALLBACK #%d] Processing %d frames\n", ++callback_count, frame_count);
#endif /* defined(DEBUG_LOG_AUDIO) */
```

### Format Code Before Committing

All code **must** be formatted using `clang-format` before committing. This ensures a consistent coding style across the entire codebase.

**Warning**: Never apply formatting or make any manual edits to files in the `third_party/` directory. These are external libraries and must remain unmodified.

To format your code, run the following command from the project root:
```bash
clang-format -i $(git ls-files | grep -E '\.(h|cc)$' | grep -vE '^(assets|archive|third_party)/')
```

Refer to the `.clang-format` file in the project root for the specific style rules.

### Ensure Newline at End of File

All source files (`.h`, `.cc`, `.cpp`, etc.) must end with a newline character. This prevents "No newline at end of file" errors from linters and ensures consistent file handling.

### Source File Headers

Every source file (`.h`, `.cc`) must begin with a concise 3-line comment header describing its purpose.

Example:
```cpp
// This file is part of the 64k demo project.
// It implements the core audio synthesis engine.
// This is not a user-facing header, but an internal one.
```

### Function and method comments

Functions and methods, especially if they are internal non user-facing,
should at least have a 1-line comment describing what they do or their
how/when they should be called. Except if they are just 1-line function
or very very short, obvious ones.

### '#endif' directive

The closing #endif directive must recall the corresponding opening #ifdef
clause they are closing

Example:
```cpp
#ifdef MY_TAG
...some code
#endif /* MY TAG */
```

We must also prefer '#if defined(MY_QUITE_LONG_TAG)' over '#ifdef MY_QUITE_LONG_TAG'
especially if there's a risk of having later something like:
```cpp
#if defined(MY_TAG_1) && !defined(MY_TAG_2)
```

### use and abuse 'const' directives

Especially for local variable, use 'const' qualification as much as
possible.

As an example, don't use:
```cpp
StructA variable_name = StructA(...);
```

but prefer instead:
```cpp
const StructA variable_name = StructA(...);
```

if variable_name is not mutated afterward.

Also: pass parameter as "const ref" as much as possible
(```const Struct& param``` instead of pointers or non-const refs)

In summary: use 'const variable = ...;` as much as possible.

### put spaces around code and operators (cosmetics)

Don't compact the code to much horizontally, and prefer adding extra
spaces around code and operators.

Example:
```cpp
const bool v = my_variable && (my_function() / 3. > (1. / x));
const y = function_call(3, x, 2.);
for (int x = 0; x < 24; ++x) { ... }
```

instead of
```cpp
const bool v=my_variable&&my_function()/3.>(1./x);
const y = function_call(3,x,2);
for(int x=0;x<24;++x){ ... }
```

### prefer prefixed incrementation over suffixed

Use pre-incrementation:
```cpp
++x
```

instead of post-incrementation:

```cpp
x++
```

### use extra () for boolean operations

Even if they are not strictly needed due to operator precedence rules,
prefer adding extra ()'s around tests for clarity, with parcimony.

### c++ cast

don't use reinterpret_cast<>, static_cast<> or const_cast<>.

### pointer declaration

prefer ```const T* name``` to ```const T *name```.

### 'auto' type

Don't use 'auto' as type, unless for complex iterators or very complex
types.

### don't use trailing whitespaces

don't.

### no missing \newline at the end of file

make sure each file has a final \n newline.

### c++ keyword indentation:

The keyword 'public', 'protected', 'private' should be intended 1 character
less than the methods.

Example:
```cpp
 private:
  int field_;
```

instead of:
```cpp
private:
  int field_;
```

### try to use per-field initialized const struct

Use the `.field = ...,` initialization pattern instead for `var.field =
...;`, especially if it means you can have the variable be declared 'const'
that way.

Example, instead of:
```cpp
WGPUTextureViewDescriptor view_desc = {};
view_desc.format = g_format;
view_desc.dimension = WGPUTextureViewDimension_2D;
view_desc.mipLevelCount = 1;
view_desc.arrayLayerCount = 1;
```

use:
```cpp
const WGPUTextureViewDescriptor view_desc = {
  .format = g_format,
  .dimension = WGPUTextureViewDimension_2D,
  .mipLevelCount = 1,
  .arrayLayerCount = 1,
};
```

Make sure the `.field = ...,` initialization pattern is compatible with the compiler / c++ version used.

### vertical space

keep the code compact vertically. That includes shader code, too.
Use only one statement per line.

### File size limit

Any file larger than 500 lines should ideally be split into sub-functionalities in separate files. (Exceptions: platform code or generated files).

### finally

Make sure everything is reflected in clang-format.

## Development Protocols

### Adding a New Visual Effect

1.  **Implement**: Create or update a class in `src/gpu/demo_effects.cc` (and declare in `demo_effects.h`) that inherits from `Effect`.
    -   Implement `init()` for one-time resource setup (e.g., using the asset system).
    -   Implement `compute()` if you need GPU-side physics or state updates.
    -   Implement `render()` to record WebGPU draw commands.
2.  **Register**: Add an `EFFECT` entry to `assets/demo.seq` specifying the class name, start/end times, and any constructor arguments.
3.  **Update Tests** (REQUIRED): Add your effect to `src/tests/test_demo_effects.cc`:
    -   Add effect to the appropriate test list (`test_post_process_effects()` or `test_scene_effects()`)
    -   Increment `EXPECTED_POST_PROCESS_COUNT` or `EXPECTED_SCENE_COUNT` at the top of the file
    -   If your effect requires `Renderer3D` with full shader setup, add it to the `requires_3d` check
    -   The test will fail with a clear error message if you forget this step
4.  **Verify**: Build with `DEMO_ALL_OPTIONS=ON` and run tests:
    ```bash
    cmake -S . -B build -DDEMO_BUILD_TESTS=ON
    cmake --build build --target test_demo_effects
    cd build && ./test_demo_effects
    ```
    Then test your effect at its specific timestamp with `--seek`.

### Audio Subsystem Initialization

The audio subsystem uses `AudioEngine` to manage initialization order and lifecycle.

**In production code (`main.cc` or similar):**

```cpp
#include "audio/audio_engine.h"

// 1. Initialize audio backend
audio_init();

// 2. Initialize audio engine (manages synth + tracker)
static AudioEngine g_audio_engine;
g_audio_engine.init();

// 3. In main loop
g_audio_engine.update(music_time);
```

**In tests:**

```cpp
#include "audio/audio_engine.h"

void test_audio_feature() {
  AudioEngine engine;
  engine.init();

  // Test logic here
  engine.update(1.0f);

  engine.shutdown();  // Always cleanup at end
}
```

**Low-level usage (when AudioEngine is not needed):**

For tests that only need synth or tracker (not both), you can still call `synth_init()` or `tracker_init()` directly. However, if you need both, always use `AudioEngine` to ensure correct initialization order.

**Direct synth API calls are valid:**

- `synth_register_spectrogram()` - Register spectrograms
- `synth_trigger_voice()` - Trigger audio playback
- `synth_get_output_peak()` - Get audio levels for visualization
- `synth_render()` - Low-level rendering

These are performance-critical APIs and should be called directly, not wrapped by AudioEngine.

### Fatal Error Checking

The project uses fatal error checking macros that can be stripped for final builds. These are defined in `src/util/fatal_error.h`.

**When to use fatal error checks:**

Use fatal error checking for conditions that indicate programming errors or corrupt state that cannot be recovered from:
- Bounds checking (array/buffer overflows)
- Null pointer checks
- Invariant violations
- Invalid state transitions

**DO NOT use for:**
- Expected runtime errors (file not found, network errors)
- Recoverable conditions
- Input validation (use return codes instead)

**Macro Reference:**

```cpp
#include "util/fatal_error.h"

// Conditional check (most common - 90% of cases)
FATAL_CHECK(write_pos >= capacity,
            "write_pos out of bounds: %d >= %d\n",
            write_pos, capacity);

// Unconditional error (should-never-happen)
FATAL_ERROR("Invalid state: %d\n", state);

// Unreachable code marker (switch defaults)
switch (type) {
  case TYPE_A: return handle_a();
  case TYPE_B: return handle_b();
  default: FATAL_UNREACHABLE();
}

// Assertion-style (documenting invariants)
FATAL_ASSERT(buffer != nullptr);
FATAL_ASSERT(count > 0 && count < MAX_COUNT);

// Complex validation blocks
FATAL_CODE_BEGIN
  static int callback_depth = 0;
  ++callback_depth;
  if (callback_depth > 1) {
    FATAL_ERROR("Callback re-entered! depth=%d", callback_depth);
  }
FATAL_CODE_END
```

**Build modes:**
- **Debug/STRIP_ALL**: All checks enabled, full error messages with file:line info
- **FINAL_STRIP**: All checks compiled out (zero runtime cost, ~500-600 bytes saved)

**Testing requirement:**

Before committing code with fatal error checks, verify both modes compile:

```bash
# Normal build (checks enabled)
cmake -S . -B build
cmake --build build && cd build && ctest

# FINAL_STRIP build (checks stripped)
cmake -S . -B build_final -DDEMO_FINAL_STRIP=ON
cmake --build build_final
```

Or use the convenience script:
```bash
./scripts/build_final.sh
```

**Important:** Never use `FATAL_CHECK` for conditions that can legitimately occur during normal operation. These are for programming errors only.

### Script Maintenance After Hierarchy Changes

After any major source hierarchy change (moving, renaming, or reorganizing files), you **must** review and update all scripts in the `scripts/` directory to ensure they remain functional.

**When to perform this review:**
- Moving source files to new directories (e.g., `src/platform.cc` → `src/platform/platform.cc`)
- Renaming source files or directories
- Reorganizing the build system (CMake changes, new subdirectories)
- Changing asset locations or formats

**Scripts that commonly require updates:**
- `scripts/check_all.sh` - Build verification and testing
- `scripts/gen_coverage_report.sh` - Code coverage analysis
- `scripts/build_win.sh` - Windows cross-compilation
- `scripts/gen_assets.sh` - Asset generation pipeline
- Any scripts with hardcoded paths or assumptions about file locations

**Verification steps:**
1. Run `./scripts/check_all.sh` to verify builds and tests still work
2. Run `./scripts/gen_coverage_report.sh` to ensure coverage tracking handles new paths
3. Test platform-specific scripts if applicable (`build_win.sh`, `run_win.sh`)
4. Check for error messages about missing files or incorrect paths
5. Update documentation if script behavior has changed

**Recent example:**
When `src/platform.cc` was moved to `src/platform/platform.cc`, the coverage script (`gen_coverage_report.sh`) initially failed with "unable to open /Users/skal/demo/src/platform.cc" due to stale coverage data. The fix required:
- Adding 'source' to `LCOV_OPTS` ignore list to handle missing source files
- Enabling automatic cleanup of the `build_coverage/` directory before each run
- Testing the script to verify it handles the new file structure

**Automation:**
The `./scripts/check_all.sh` script is designed to catch most issues automatically. Always run it before committing hierarchy changes to ensure no regressions in build or test infrastructure.