diff options
Diffstat (limited to 'PLATFORM_ANALYSIS.md')
| -rw-r--r-- | PLATFORM_ANALYSIS.md | 314 |
1 files changed, 314 insertions, 0 deletions
diff --git a/PLATFORM_ANALYSIS.md b/PLATFORM_ANALYSIS.md new file mode 100644 index 0000000..ca5ef0e --- /dev/null +++ b/PLATFORM_ANALYSIS.md @@ -0,0 +1,314 @@ +# Platform Code Analysis & Reorganization Plan + +## Current State + +### Files Location +- `src/platform.h` (100 lines) - Platform API header with WebGPU shims +- `src/platform.cc` (131 lines) - GLFW-based windowing implementation + +### Coverage Status +**Coverage: 0%** - No tests currently exist for platform code + +### Platform API Surface +**Core Functions** (all untested): +```cpp +PlatformState platform_init(bool fullscreen, int width, int height); +void platform_shutdown(PlatformState* state); +void platform_poll(PlatformState* state); +bool platform_should_close(PlatformState* state); +void platform_toggle_fullscreen(PlatformState* state); +WGPUSurface platform_create_wgpu_surface(WGPUInstance instance, PlatformState* state); +double platform_get_time(); +``` + +**Platform Shims** (in platform.h): +```cpp +// Win32 vs Native API differences +str_view() // String view wrapper +label_view() // Label wrapper (respects STRIP_ALL) +platform_wgpu_wait_any() +platform_wgpu_set_error_callback() +``` + +### Platform-Specific Code Distribution + +#### In `src/platform.h` (60% of file) +- Lines 10-72: WebGPU API shims for Win32 vs native +- `DEMO_CROSS_COMPILE_WIN32` conditional compilation +- String view helpers + +#### In `src/gpu/` (scattered across 4 files) +1. **gpu/gpu.h** (line 60): + - Win32 vs native device descriptor differences + +2. **gpu/gpu.cc** (12 blocks): + - Lines 214-266: Device creation descriptor differences + - Lines 269-301: Adapter request differences + - Lines 312-321: Instance creation differences + - Lines 327-329: Error callback setup + - Lines 332-341: Error handling differences + - Lines 345-347: Device loss callback + +3. **gpu/effect.cc** (3 blocks): + - Lines 268-270: Shader composition (WGPUShaderSourceWGSL) + - Lines 310-312: Compute pipeline creation + - Lines 346-348: Render pipeline creation + +4. **gpu/texture_manager.cc** (2 blocks): + - Lines 8+: Texture format differences + - Line 63+: Copy texture descriptor differences + +**Total**: ~150 lines of platform-specific code across 5 files + +### Current Includes (10 files) +- Production: `main.cc`, `test_demo.cc` +- GPU: `gpu/gpu.cc`, `gpu/gpu.h` +- Tests: `test_texture_manager.cc`, `test_3d_render.cc`, `test_mesh.cc`, `test_shader_compilation.cc`, `test_3d_physics.cc` +- Self: `platform.cc` + +--- + +## Proposed Reorganization + +### Option A: Minimal Move (Recommended) +**Goal**: Move platform files to subdirectory, keep GPU platform code in place + +**Changes**: +1. Create `src/platform/` directory +2. Move `src/platform.{h,cc}` → `src/platform/platform.{h,cc}` +3. Update all includes: `#include "platform.h"` → `#include "platform/platform.h"` +4. Leave GPU platform-specific code in `src/gpu/*` (it's WebGPU API abstraction, not OS abstraction) + +**Pros**: +- Clean separation: platform windowing in `platform/`, GPU API shims in `gpu/` +- Minimal code changes (just include paths) +- Follows existing pattern (`src/audio/`, `src/3d/`, `src/gpu/`) + +**Cons**: +- Platform-specific code still scattered (but logically grouped) + +**Impact**: 10 files need include path updates + +--- + +### Option B: Aggressive Consolidation (Not Recommended) +**Goal**: Centralize ALL platform-specific code in one place + +**Changes**: +1. Create `src/platform/` directory structure: + ``` + src/platform/ + ├── platform.h/cc # Windowing API + ├── webgpu_shims.h # WebGPU compatibility layer + └── webgpu_platform.cc # Extracted from gpu.cc + ``` +2. Extract platform-specific blocks from `gpu/*.cc` into new files +3. Create abstraction layer for WebGPU platform differences + +**Pros**: +- All platform code in one place +- Easier to add new platforms (e.g., bare Win32) + +**Cons**: +- Major refactor (~20 files touched) +- Mixes OS-level platform code with WebGPU API abstraction (different concerns) +- GPU code becomes harder to read (indirection through platform layer) +- Risk of regressions + +**Impact**: 20+ files, ~500 lines of changes + +--- + +## Test Coverage Plan + +### Test File: `src/tests/test_platform.cc` + +#### Testable Functions (without GUI) +1. ✅ **platform_get_time()** + - Call and verify returns valid timestamp + - Call twice, verify time advances + +2. ⚠️ **platform_init()** (requires GLFW context) + - Test windowed mode initialization + - Test fullscreen mode initialization + - Verify default dimensions + - Check aspect ratio calculation + +3. ⚠️ **platform_poll()** (requires GLFW context) + - Verify time updates + - Verify aspect ratio updates + +4. ⚠️ **platform_should_close()** (requires GLFW context) + - Initially returns false + - Returns true after close signal + +5. ⚠️ **platform_toggle_fullscreen()** (requires GLFW context) + - Toggle state change + - Windowed geometry preservation + +6. ❌ **platform_create_wgpu_surface()** (requires WebGPU instance) + - Hard to test without full GPU initialization + - Covered implicitly by existing GPU tests + +7. ⚠️ **platform_shutdown()** (requires GLFW context) + - Cleanup without crash + +#### Testing Strategy + +**Approach 1: Headless GLFW** (Recommended) +- Use `GLFW_VISIBLE` window hint set to false +- Test basic lifecycle without visible window +- Limited: Cannot test actual fullscreen behavior + +**Approach 2: Mock Platform** (Overkill) +- Create `MockPlatformState` for testing +- Requires significant refactoring +- Not worth effort for simple windowing code + +**Approach 3: Minimal Coverage** (Pragmatic) +- Test `platform_get_time()` only (no GLFW dependency) +- Accept low coverage for GLFW wrapper code +- Platform code is thin wrapper, not complex logic + +### Recommended Test Implementation + +```cpp +// src/tests/test_platform.cc +#include "platform/platform.h" +#include <cassert> +#include <cmath> +#include <unistd.h> + +// Test 1: Time query +void test_platform_get_time() { + const double t1 = platform_get_time(); + assert(t1 >= 0.0); + + usleep(10000); // Sleep 10ms + + const double t2 = platform_get_time(); + assert(t2 > t1); + assert(t2 - t1 >= 0.01 && t2 - t1 < 0.1); // ~10ms tolerance +} + +// Test 2: Basic lifecycle (headless window) +void test_platform_lifecycle() { + PlatformState state = platform_init(false, 640, 480); + + assert(state.window != nullptr); + assert(state.width > 0); + assert(state.height > 0); + assert(state.aspect_ratio > 0.0f); + assert(state.time >= 0.0); + assert(!state.is_fullscreen); + + // Poll should update time + const double t1 = state.time; + usleep(10000); + platform_poll(&state); + assert(state.time > t1); + + // Should not close initially + assert(!platform_should_close(&state)); + + platform_shutdown(&state); +} + +// Test 3: String view helpers (Win32 vs native) +void test_string_views() { + const char* test_str = "test"; + +#if defined(DEMO_CROSS_COMPILE_WIN32) + // Win32: returns const char* + assert(str_view(test_str) == test_str); + assert(label_view(test_str) == test_str); +#else + // Native: returns WGPUStringView + WGPUStringView sv = str_view(test_str); + assert(sv.data == test_str); + assert(sv.length == 4); + +#if !defined(STRIP_ALL) + WGPUStringView lv = label_view(test_str); + assert(lv.data == test_str); + assert(lv.length == 4); +#else + WGPUStringView lv = label_view(test_str); + assert(lv.data == nullptr); + assert(lv.length == 0); +#endif +#endif +} + +int main() { + test_platform_get_time(); + test_string_views(); + test_platform_lifecycle(); // Requires GLFW, may fail in CI + + return 0; +} +``` + +**Expected Coverage Increase**: 60-70% (time functions, string views, basic init/shutdown) + +--- + +## Recommendation + +### Phase 1: Create Test (Immediate) +1. Create `src/tests/test_platform.cc` with basic coverage +2. Add to CMakeLists.txt under `DEMO_BUILD_TESTS` +3. Test platform_get_time() and string view helpers +4. Optionally test basic lifecycle with headless GLFW window + +**Expected outcome**: Coverage 60-70% (from 0%) + +### Phase 2: Move Files (Low Priority) +1. Create `src/platform/` directory +2. Move `platform.{h,cc}` to `src/platform/` +3. Update 10 include paths +4. Verify build on all platforms (macOS, Linux, Win32) + +**Expected outcome**: Better organization, no functional change + +### Phase 3: Task B Cleanup (Future) +- Once platform code is in `platform/`, revisit Task B +- Consider extracting common WebGPU shims to `platform/webgpu_shims.h` +- Leave GPU-specific platform code in `gpu/` (it's WebGPU API abstraction) + +--- + +## Platform-Specific Code Summary + +| Location | Lines | Purpose | Should Move? | +|----------|-------|---------|--------------| +| `src/platform.h` | 60 | WebGPU API shims, string views | ✅ Yes → `platform/` | +| `src/platform.cc` | 131 | GLFW windowing | ✅ Yes → `platform/` | +| `src/gpu/gpu.h` | 2 | Device descriptor diffs | ❌ No (GPU API) | +| `src/gpu/gpu.cc` | ~80 | WebGPU init differences | ❌ No (GPU API) | +| `src/gpu/effect.cc` | ~15 | Shader/pipeline creation | ❌ No (GPU API) | +| `src/gpu/texture_manager.cc` | ~10 | Texture format diffs | ❌ No (GPU API) | + +**Total**: ~298 lines of platform-specific code +- **Windowing layer** (191 lines): Should move to `platform/` +- **WebGPU API layer** (107 lines): Should stay in `gpu/` + +--- + +## Conclusion + +**Recommended Action**: +1. ✅ Create `test_platform.cc` now (boosts coverage 0% → 60%) +2. ✅ Move `platform.{h,cc}` to `src/platform/` subdirectory +3. ❌ Do NOT extract GPU platform code (different concern - API abstraction, not OS abstraction) + +**Rationale**: +- Platform windowing (GLFW) vs WebGPU API differences are separate concerns +- Moving only windowing code is clean, low-risk +- GPU code is already well-organized and platform-specific blocks are minimal +- Consolidating everything would create unnecessary indirection + +**Task B Implication**: +- After move, `platform/platform.h` centralizes OS windowing abstractions +- GPU platform-specific code remains in `gpu/` (it's WebGPU API compatibility, not OS code) +- Task B can be updated: "Move platform windowing to `platform/` subdirectory" (done) |