diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md new file mode 100644 index 0000000..bfa2995 --- /dev/null +++ b/CODE_REVIEW.md @@ -0,0 +1,346 @@ +# Auditoría arquitectónica — Orni Attack + +> Pasada de revisión de **diseño y arquitectura** (no de estilo) sobre la rama `main` tras cerrar el ciclo de lint (cppcheck/clang-tidy a 0 hits). +> Generada el 2026-05-20 por el subagente `Plan` y verificada parcialmente. +> Las referencias `file:line` son relativas a la raíz del repo. + +## Cómo usar este documento + +- **Prioridad** indica urgencia subjetiva (alta/media/baja). +- **Tipo** distingue: + - **[estructural]** = bug latente, deuda técnica medible, código muerto + - **[opinable]** = cambio de estilo/diseño que el usuario puede rechazar legítimamente +- **Esfuerzo**: small (1 commit < 30 min) / medium (1-3 commits) / large (varias sesiones). + +Antes de actuar sobre un hallazgo, **re-verificar** que sigue siendo válido (el código habrá cambiado). + +--- + +## Hallazgos por área + +### A. Escenas + +#### 1. `Scene` no declara `init()`, pero `GameScene` lo expone — [PRIORIDAD: alta, estructural] +- **Dónde**: `source/core/system/scene.hpp:31-40`, `source/game/scenes/game_scene.hpp:47`, `source/game/scenes/game_scene.cpp:64` (`init()` se llama desde el ctor). +- **Problema**: La interfaz `Scene` declara `handleEvent/update/draw/isFinished` como obligatorios. `GameScene::init()` es un método público adicional, pero el Director nunca lo llama — `GameScene` lo invoca desde su propio ctor. El comentario del header dice "init() — llamado por Director tras crear la escena", pero eso no ocurre. `TitleScene` y `LogoScene` no tienen ese método: hacen todo en el ctor. Inconsistencia de lifecycle. +- **Propuesta**: (a) eliminar `init()` y mover su contenido al ctor (lo más simple), o (b) añadirlo a la interfaz `Scene` y llamarlo desde el Director tras `buildScene`. Lo coherente es (a). +- **Esfuerzo**: small + +#### 2. No existe clase base `MenuScene` para el patrón `timer + state-machine + skip-button` — [PRIORIDAD: media, opinable] +- **Dónde**: `source/game/scenes/title_scene.cpp:368-388`, `source/game/scenes/logo_scene.cpp:219-280`, partes de `game_scene.cpp` (INIT_HUD/LEVEL_START). +- **Problema**: Las tres escenas implementan la misma idea: un enum `State`, un timer acumulado, un `switch` por estado, una transición "skip". `checkSkipButtonPressed()` aparece en `TitleScene` y `LogoScene` con la misma signatura estática. +- **Propuesta**: Mixin pequeño `TimedStateScene` con `enterState(EnumT)`, `time_in_state()`, helpers de "esperar N segundos y avanzar". Al menos consolidar `checkSkipButtonPressed()` y la pareja `temps_estat_actual_ + cambio de estado` en un helper. +- **Esfuerzo**: medium + +#### 3. `TitleScene::initTitle()` duplica dos veces el cálculo de bounding box + posicionado — [PRIORIDAD: alta, estructural] +- **Dónde**: `source/game/scenes/title_scene.cpp:104-254` (función de 150 líneas). +- **Problema**: Carga "ORNI" (108-166) y "ATTACK!" (178-241) con el mismo código copiado. `inicialitzarJailgames()` (256-325) hace lo mismo por tercera vez. Tres copias del cómputo de bounding box. +- **Propuesta**: Extraer `loadLetterRow(filenames, scale, y_position, espai) -> std::vector`. +- **Esfuerzo**: small + +#### 4. `PLAYER_JOIN_PHASE` en TitleScene mezcla animación + audio + late join — [PRIORIDAD: media, opinable] +- **Dónde**: `source/game/scenes/title_scene.cpp:451-483`. +- **Problema**: `updatePlayerJoinPhaseState` hace varias cosas no relacionadas. La lógica de "late join" reaprovecha `checkStartGameButtonPressed()` (que *muta* `match_config_` — efecto secundario no advertido por la firma). +- **Propuesta**: Renombrar `checkStartGameButtonPressed()` a `tryActivatePlayersOnStart()`. Separar la lógica de "late join" en un helper. +- **Esfuerzo**: small + +#### 5. `SceneContext::actual` es estado global redundante — [PRIORIDAD: baja, opinable] +- **Dónde**: `source/core/system/scene_context.hpp:81`, escrita en `director.cpp:267,275` y `global_events.cpp:29,58`. +- **Problema**: `inline SceneContext::SceneType actual` como variable global, sincronizada manualmente. No la consulta nadie excepto el propio Director (que lee `context.nextScene()`, no `actual`). +- **Propuesta**: Verificación profunda. Si nadie la consulta, eliminarla. +- **Esfuerzo**: small + +### B. Sistemas + +#### 6. `StageManager::update(dt, pause_spawn)` tiene `pause_spawn` muerto — [PRIORIDAD: alta, estructural] +- **Dónde**: `source/game/stage_system/stage_manager.hpp:28`, `source/game/stage_system/stage_manager.cpp:128-135`, `game_scene.cpp:359-374,423-424`. +- **Problema**: El parámetro `pause_spawn` existe en la API pero ningún caller lo pasa con sentido — la pausa real ocurre por otra ruta (`spawn_controller_.update(dt, enemies, PAUSE_SPAWN)`). El comentario en `stage_manager.cpp:131-134` ya admite que no se usa. +- **Propuesta**: Quitar el parámetro. +- **Esfuerzo**: small + +#### 7. `SpawnController` hardcoda `std::array` en su firma — [PRIORIDAD: media, estructural] +- **Dónde**: `source/game/stage_system/spawn_controller.hpp:33,37,39`; `.cpp:44,89,96`. +- **Problema**: Literal `15` en lugar de `Defaults::Entities::MAX_ORNIS`. Acoplamiento silencioso por número. +- **Propuesta**: Usar `std::array&` o un alias `using EnemyArray = ...;`. +- **Esfuerzo**: small + +#### 8. `Systems::Collision::Context` y `Systems::ContinueScreen::Context` son "god structs" — [PRIORIDAD: alta, estructural] +- **Dónde**: `source/game/systems/collision_system.hpp:31-43`, `source/game/systems/continue_system.hpp:25-38`. +- **Problema**: Cada `Context` agrupa entre 9 y 12 referencias mutables a estado de `GameScene` + un `std::function` callback. Punto único donde cualquier cambio de campo en GameScene rompe el módulo. Los sistemas pueden mutar todo (no hay distinción "lo que leo" vs "lo que modifico"). La separación `GameScene` ↔ `Systems::*` es nominal. +- **Propuesta**: Que `detectAll` devuelva una struct "outcome" (qué balas se desactivaron, qué enemigos murieron, qué jugador fue hit) y que GameScene aplique el resultado. Si es demasiado, al menos partir el Context en `In` (lectura) e `Out` (escritura). +- **Esfuerzo**: large + +#### 9. `CollisionSystem` y `PhysicsWorld` se solapan — [PRIORIDAD: media, opinable] +- **Dónde**: `source/game/systems/collision_system.cpp` (todo), `source/core/physics/physics_world.cpp:118-179`. +- **Problema**: `PhysicsWorld::resolveBodyCollisions` resuelve colisiones cuerpo-cuerpo con impulsos elásticos; `CollisionSystem::detectBulletEnemy` itera otra vez los mismos pares con un *amplifier* distinto y aplica daño. Dos bucles O(n²) sobre el mismo conjunto. El comentario en `game_scene.cpp:159` explica la sutileza pero es frágil. +- **Propuesta**: Verificación profunda. ¿Deberían las balas vivir fuera del world directamente? +- **Esfuerzo**: medium + +#### 10. `init_hud_animator` es solo 3 funciones libres en un namespace — [PRIORIDAD: baja, opinable] +- **Dónde**: `source/game/systems/init_hud_animator.hpp`, `.cpp` (99 líneas). +- **Problema**: A diferencia de `CollisionSystem`/`ContinueSystem`, no tiene `Context`; son 3 funciones puras. La nomenclatura sugiere algo más grande. +- **Propuesta**: Mover a `source/core/rendering/` o renombrar `systems/` a `gameplay/`. +- **Esfuerzo**: small + +### C. Entidades + +#### 11. `Ship` expone tres métodos equivalentes: `isAlive()`, `isHit()`, `isActive()` — [PRIORIDAD: alta, estructural] +- **Dónde**: `source/game/entities/ship.hpp:27,38,39`. +- **Problema**: `isActive() = !is_hit_`, `isAlive() = !is_hit_`, `isHit() = is_hit_`. Tres formas de preguntar lo mismo, usadas en sitios distintos. +- **Propuesta**: Quedarse con `isActive()` (override de Entity) y reescribir los call-sites. +- **Esfuerzo**: small + +#### 12. `Entity::renderer_` raw pointer; ctor por defecto deja la entidad inservible — [PRIORIDAD: media, estructural] +- **Dónde**: `source/core/entities/entity.hpp:56,67`, `source/game/entities/ship.hpp:15-16` (`Ship() : Entity(nullptr) {}`), idem en `enemy.hpp:38-39`, `bullet.hpp:15-16`. +- **Problema**: El ctor por defecto existe solo para soportar `std::array`, pero deja `renderer_` nullptr. Si alguien dibuja antes de la reasignación, segfault. `renderer_` no tiene in-class initializer. +- **Propuesta**: (a) Inicializador en miembro `Rendering::Renderer* renderer_{nullptr};`. (b) Guard en `draw()` para `renderer_ == nullptr`. (c) Considerar `std::array, 2>`. +- **Esfuerzo**: small (a)+(b); medium (c) + +#### 13. Duplicación de estado: `center_/angle_` en Entity vs `body_.position/body_.angle` en RigidBody — [PRIORIDAD: media, estructural] +- **Dónde**: `source/core/entities/entity.hpp:58-59,64`; `source/game/entities/ship.cpp:131-135` (postUpdate sincroniza), `enemy.cpp:214-219`, `bullet.cpp:121-125`. +- **Problema**: Cada entidad mantiene `center_/angle_` como mirror del body. Cualquier código que lea `getCenter()` durante `update()` (antes del `postUpdate`) verá el valor del frame anterior. El patrón está documentado pero es frágil. +- **Propuesta**: Hacer que `getCenter()` devuelva `body_.position` directamente (eliminando `center_`). +- **Esfuerzo**: medium + +#### 14. `Entity::renderer_` `protected:` con acceso directo + getter por referencia mutable a `body_` — [PRIORIDAD: media, estructural] +- **Dónde**: `source/core/entities/entity.hpp:51-52` (`getBody() -> RigidBody&`), `:54-64`. +- **Problema**: Encapsulación abierta + getters que devuelven referencias no-const al `body_`. El contrato de quién muta `body_.velocity`/`position` no está claro. +- **Propuesta**: Documentar el contrato. Hacer `body_` privado con API estricta (`registerInWorld(world)`). +- **Esfuerzo**: medium + +#### 15. `Enemy` tiene `drotacio_/rotacio_` (rotación visual) Y `body_.angle/angular_velocity` — [PRIORIDAD: baja, estructural] +- **Dónde**: `source/game/entities/enemy.hpp:92-93`, `.cpp:210-212`. +- **Problema**: El body de un enemy nunca rota (angular_velocity siempre 0), pero la visual sí. Dos representaciones de orientación viviendo en la misma entidad. +- **Propuesta**: Eliminar `body_.angle/angular_velocity` para enemies (no se usa). +- **Esfuerzo**: small + +### D. Renderizado y recursos + +#### 16. `Rotation3D` (struct + parámetro en `renderShape`) es código muerto — [PRIORIDAD: media, estructural] +- **Dónde**: `source/core/rendering/shape_renderer.hpp:18-35,52`, `.cpp:13-78`. +- **Problema**: La función `renderShape` acepta `const Rotation3D* rotation_3d = nullptr`. **Todos** los callers pasan `nullptr` (`ship.cpp:161`, `enemy.cpp:232`, `bullet.cpp:137`). 28 líneas de código muerto. +- **Propuesta**: Eliminar `Rotation3D`, el parámetro y el branch. +- **Esfuerzo**: small + +#### 17. `Resource::Helper` es una fachada delgada sobre `Resource::Loader` — [PRIORIDAD: media, estructural] +- **Dónde**: `source/core/resources/resource_helper.hpp`, `.cpp` (80 líneas); `resource_loader.hpp` (54 líneas). +- **Problema**: `Helper::loadFile()` es `Loader::get().loadResource(normalizePath(...))`. La única "lógica real" de Helper es `normalizePath()`. Duplicación de API. +- **Propuesta**: Mover `normalizePath` a `Resource::Loader` y eliminar `Helper`. +- **Esfuerzo**: small + +#### 18. `ShapeLoader::resolvePath` parece código muerto — [PRIORIDAD: baja, opinable] +- **Dónde**: `source/core/graphics/shape_loader.hpp:36`, `.cpp:70-83`. +- **Problema**: `resolvePath` está declarada como helper privado, pero no se llama desde ningún sitio. +- **Propuesta**: Eliminar `resolvePath` y `BASE_PATH`. +- **Esfuerzo**: small + +#### 19. `ShapeLoader::cache` sin política de invalidación — [PRIORIDAD: baja, estructural] +- **Dónde**: `source/core/graphics/shape_loader.cpp:13,15-60`. +- **Problema**: Caché estática `unordered_map>`. `clearCache()` existe pero nadie la llama. Sin límite de tamaño. +- **Propuesta**: Documentar en el header que la caché vive toda la vida del proceso. +- **Esfuerzo**: small + +#### 20. `GpuFrameRenderer::device()` y `isInsideFrame()` son getters públicos sin callers — [PRIORIDAD: media, estructural] +- **Dónde**: `source/core/rendering/gpu/gpu_frame_renderer.hpp:96,97`. Similar `SDLManager::getScaleFactor()` (línea 42). +- **Problema**: API pública sin clientes. +- **Propuesta**: Eliminar o documentar como API reservada. +- **Esfuerzo**: small + +### E. Configuración + +#### 21. `Options::physics`, `Options::audio`, `Options::gameplay` se cargan desde YAML pero nunca se consultan en runtime — [PRIORIDAD: alta, estructural] +- **Dónde**: + - `Options::physics` se rellena en `options.cpp:194-201`, parse 286-291, write 597-603. Único call-site lector: `director.cpp:109` (informativo). Ship/Enemy/Bullet leen `Defaults::Physics::*` directamente. + - `Options::audio` documentado como "deliberadamente desacoplado" (`audio.hpp:22-25`); `director.cpp:235-244` construye `Audio::Config` desde `Defaults::Audio`. + - `Options::gameplay` (max_enemies, max_bullets): las arrays son compile-time (`std::array`). +- **Problema**: El usuario edita su `config.yaml`, cambia `physics.rotation_speed: 6.0`, reinicia el juego — el juego sigue usando el default. Prometemos configurabilidad que no entregamos. +- **Propuesta**: O bien (a) conectar Options→runtime, o (b) eliminar los campos muertos. Híbrido: borrar `audio`/`gameplay`, conectar `physics` (apoya Fase 10). +- **Esfuerzo**: small (eliminar) o medium (conectar correctamente). + +#### 22. `defaults.hpp` mezcla constantes de cinco dominios distintos en un solo archivo de 530 líneas — [PRIORIDAD: media, estructural] +- **Dónde**: `source/core/defaults.hpp` (todo). +- **Problema**: Contiene Window, Game, Zones, Entities, Palette, Ship, Physics, Math, Brightness, Rendering, Audio, Music, Sound, Controls, Enemies (con sub-namespaces), Title, FloatingScore. **22 archivos** lo incluyen. Editar una constante de Audio fuerza recompilación de todo. Dos `namespace Game` separados (líneas 22-25 y 132-210). +- **Propuesta**: Partir en `defaults/window.hpp`, `defaults/game.hpp`, `defaults/zones.hpp`, etc. Con `defaults.hpp` como umbrella. +- **Esfuerzo**: medium + +#### 23. `Defaults::Window::WIDTH = 1280` y `Defaults::Game::WIDTH = 1280` son duplicados sin relación enforced — [PRIORIDAD: media, estructural] +- **Dónde**: `source/core/defaults.hpp:10` vs `:23`. +- **Problema**: Dos `WIDTH` distintos pero con el mismo valor. Nadie obliga a que coincidan. Si alguien quisiera arrancar a 1366×768 cambiando `Window::WIDTH`, el cálculo `zoom_factor = (float)width / Window::WIDTH` (`sdl_manager.cpp:101`) usa `Window::WIDTH`, no `Game::WIDTH` — bug latente. +- **Propuesta**: Decidir la fuente de verdad. Si es `Game::{WIDTH,HEIGHT}`, derivar `Window::WIDTH = Game::WIDTH`. +- **Esfuerzo**: small + +#### 24. `game/constants.hpp` contiene aliases legacy mayoritariamente muertos — [PRIORIDAD: baja, estructural] +- **Dónde**: `source/game/constants.hpp:10-23`. +- **Problema**: `Constants::MARGIN_*`, `MAX_ORNIS`, `MAX_BALES`, `VELOCITAT`, `VELOCITAT_MAX` — no se usan. Solo se usan los helpers (PI, isInPlayArea, getSafePlayAreaBounds, getPlayAreaCenter). +- **Propuesta**: Eliminar las constantes muertas. +- **Esfuerzo**: small + +#### 25. `Defaults::Physics::{ENEMY_SPEED, BULLET_SPEED, VELOCITY_SCALE}` legacy de Pascal son código muerto — [PRIORIDAD: media, estructural] +- **Dónde**: `source/core/defaults.hpp:218-220`. +- **Problema**: Los valores están en unidades/frame, comentados como "actuales del juego", pero la migración a SDL3 los reemplazó (`bullet.cpp:22` define su propio `BULLET_SPEED = 140.0F`; enemy speeds vienen de `Defaults::Enemies::{Pentagon,Cuadrado,Molinillo}::VELOCITAT`). +- **Propuesta**: Eliminar. +- **Esfuerzo**: small + +### F. Naming y consistencia de idioma + +#### 26. Variables del estado de juego mezclan catalán y castellano — [PRIORIDAD: baja, opinable] +- **Dónde**: `source/game/scenes/game_scene.hpp:67-77`, `source/game/scenes/title_scene.hpp:65-84`. +- **Problema**: Conviven `hit_timer_per_player_` (inglés), `lives_per_player_` (inglés), `match_config_.jugador1_actiu` (catalán). En TitleScene: `estat_actual_` (catalán) junto a `factor_lerp_`. No hay convención clara dentro de la misma clase. +- **Propuesta**: Si el usuario quiere preservar el sabor, reglar: identificadores expuestos en headers públicos en catalán, detalles internos en inglés. +- **Esfuerzo**: medium + +#### 27. Dos namespaces `Constants` no relacionados — [PRIORIDAD: baja, opinable] +- **Dónde**: `source/game/constants.hpp:8` (`namespace Constants`) y `source/game/stage_system/stage_config.hpp:82` (`namespace StageSystem::Constants`). +- **Problema**: Mismo nombre, scopes distintos. Lecturalmente confuso. +- **Propuesta**: Renombrar el del stage system a `StageSystem::Messages`. +- **Esfuerzo**: small + +### G. Headers y dependencias + +#### 28. `core/` depende de `game/` — violación de capas — [PRIORIDAD: alta, estructural] +- **Dónde**: + - `source/core/system/director.cpp:25-28` incluye `game/scenes/*.hpp` y `game/options.hpp` + - `source/core/rendering/sdl_manager.cpp:16` incluye `game/options.hpp` + - `source/core/system/debug_overlay.cpp:8` incluye `game/options.hpp` + - `source/core/input/input.cpp:11` incluye `game/options.hpp` +- **Problema**: La estructura sugiere un layered design, pero `core/` lee y muta `game::Options`. Si se intenta convertir `core/` en librería estática reusable, no compila. +- **Propuesta**: Mover `Options` a `source/core/config/`, o invertir la dependencia con `IConfigProvider*`. Relacionado con #21. +- **Esfuerzo**: medium + +#### 29. `continue_system.hpp` forward-declara `enum class GameOverState` definida a nivel global en `game_scene.hpp` — [PRIORIDAD: media, estructural] +- **Dónde**: `source/game/scenes/game_scene.hpp:28-32` (declaración en namespace global, NO dentro de la clase), `source/game/systems/continue_system.hpp:18-20`. +- **Problema**: Si la subyacente cambia de `uint8_t` a `int`, las dos declaraciones divergen silenciosamente. +- **Propuesta**: Mover `GameOverState` a `source/game/systems/game_over_state.hpp` (header de tipos puros). +- **Esfuerzo**: small + +#### 30. `core/defaults.hpp` se incluye en 22 archivos arrastrando ~530 líneas — [PRIORIDAD: media, estructural] +- **Dónde**: Ver #22. +- **Problema**: Tiempo de compilación incremental innecesariamente alto. +- **Propuesta**: Ver #22. +- **Esfuerzo**: medium + +### H. Bugs latentes / code smells + +#### 31. `hit_timer_per_player_` usa el valor mágico `999.0F` como sentinela "jugador permanentemente inactivo" — [PRIORIDAD: alta, estructural] +- **Dónde**: `source/game/scenes/game_scene.cpp:140,229,230,239,312,332`. +- **Problema**: El timer combina tres estados en un float: `== 0.0F` = vivo, `> 0 && < 3.0` = secuencia de muerte, `== 999.0F` = sin vidas. Si en algún momento se decrementa accidentalmente, el sentinela se pierde. +- **Propuesta**: Introducir un enum `PlayerStatus { ALIVE, DYING, OUT }` separado del timer; el timer solo cuenta `DYING`. +- **Esfuerzo**: medium + +#### 32. `std::rand()` en todo el gameplay; `std::mt19937` solo en LogoScene; reseed solo en GameScene::init — [PRIORIDAD: media, estructural] +- **Dónde**: `std::srand` en `game_scene.cpp:80`; `std::rand` en `spawn_controller.cpp:131`, `enemy.cpp:133-447`, `stage_manager.cpp:84`, `debris_manager.cpp:121-352`. `std::mt19937` en `logo_scene.cpp:171-173`. +- **Problema**: Mezcla de PRNG. `std::rand()` tiene calidad mala y es global. `srand` solo se llama una vez por partida; el logo y el título usan la seed por defecto. Repetibilidad accidental. +- **Propuesta**: Centralizar un `Random` (single mt19937 con seeds explícitas). Al menos `srand` en main/Director para que se siembre antes. +- **Esfuerzo**: small (centralizar srand) o medium (reemplazar todos). + +#### 33. `LineRenderer` mantiene dos globales mutables: `g_current_line_color` y `g_current_line_thickness` — [PRIORIDAD: media, estructural] +- **Dónde**: `source/core/rendering/line_renderer.cpp:11,15`. +- **Problema**: Estado mutable global en la TU. Si alguna escena cambia color y se olvida de restaurarlo, contamina las siguientes. Misma problemática con `g_current_scale_factor` en `coordinate_transform.cpp:9`. +- **Propuesta**: Moverlos a miembros del `GpuFrameRenderer`. El color global puede vivir como uniform de pipeline. +- **Esfuerzo**: medium + +#### 34. `GameScene::init()` reasigna `enemies_[i] = Enemy(...)` duplicando el trabajo del constructor — [PRIORIDAD: alta, estructural] +- **Dónde**: `source/game/scenes/game_scene.cpp:56-59` (ctor) vs `:149-153` (init). +- **Problema**: En el ctor se hace `enemy = Enemy(sdl.getRenderer())`. Inmediatamente después se llama `init()` (línea 64), que reasigna otra vez. Trabajo perdido. Inconsistencia con bullets (que solo se inicializan una vez). +- **Propuesta**: Eliminar la inicialización en el ctor o en init. Decidir cuál es la fuente única. +- **Esfuerzo**: small + +#### 35. `runCollisionDetections()` reconstruye un `Context` enorme cada frame — [PRIORIDAD: media, estructural] +- **Dónde**: `source/game/scenes/game_scene.cpp:470-484`, similar `stepContinueScreen` (`:252-282`). +- **Problema**: Cada frame en PLAYING se construye una struct con 9 referencias + un `std::function` (que aloca en heap por la captura `[this]`). 60 allocs/segundo del `std::function`. +- **Propuesta**: Cachear el Context como miembro o pasar el callback como puntero a función miembro. +- **Esfuerzo**: small + +#### 36. `SpawnController::generateSpawnEvents` precomputa toda la cola pero `update()` solo procesa secuencialmente — [PRIORIDAD: media, estructural] +- **Dónde**: `source/game/stage_system/spawn_controller.cpp:54-82`. +- **Problema**: El flag `event.spawnejat` parece sobrante (el index ya marca progreso). +- **Propuesta**: Verificación profunda. Simplificable. +- **Esfuerzo**: small + +#### 37. `Director::run()` es estático pero usa estado de instancia via singletons — [PRIORIDAD: baja, estructural] +- **Dónde**: `source/core/system/director.hpp:21-25`, `.cpp:41-48,150`. +- **Problema**: `executable_path_` se asigna en ctor pero nadie lo lee. `system_folder_` se usa en ctor pero no después. El patrón "Director con state" es decorativo. +- **Propuesta**: O hacer `run()` no-estática, o eliminar los miembros y mover `run()` a una función libre `App::main()`. +- **Esfuerzo**: small + +### Otros + +#### 38. `MatchConfig::jugador{1,2}_actiu` mutación desde dos sistemas independientes sin notificar — [PRIORIDAD: media, estructural] +- **Dónde**: `source/game/scenes/game_scene.cpp:912-916` (joinPlayer), `source/game/systems/continue_system.cpp:29-33` (revivePlayer). +- **Problema**: `MatchConfig` viaja como referencia mutable por los `Context` y se modifica desde múltiples sistemas. Rompe la intuición de que `MatchConfig` es "la configuración decidida en TitleScene". +- **Propuesta**: Renombrar el campo a `player_active_runtime` o mover el flag a otro lado (ej. dentro de `lives_per_player_`: si lives > 0 → activo). +- **Esfuerzo**: medium + +#### 39. Inconsistencia: `LogoScene::~LogoScene()` llama `stopAllSounds()`, `TitleScene::~TitleScene()` llama `stopMusic()`, `GameScene::~GameScene()` es default — [PRIORIDAD: baja, opinable] +- **Dónde**: `source/game/scenes/logo_scene.cpp:62-66`, `title_scene.cpp:99-102`, `game_scene.hpp:38`. +- **Problema**: Cada escena gestiona limpieza de audio a su manera. Si una olvida un sonido en loop, fuga. +- **Propuesta**: Decidir si el Director debe llamar `Audio::stopAllSounds()` y `stopMusic()` antes de destruir cada escena. +- **Esfuerzo**: small + +#### 40. `Bullet::BULLET_SPEED = 140.0F` en anonymous namespace, no en `defaults.hpp` — [PRIORIDAD: baja, opinable] +- **Dónde**: `source/game/entities/bullet.cpp:22-23`. +- **Problema**: Constante de gameplay escondida en una TU privada. Si alguien quiere tunear, mira en defaults.hpp y no la encuentra (ahí hay otro `BULLET_SPEED` diferente y muerto, ver #25). +- **Propuesta**: Mover a `Defaults::Entities::BULLET_SPEED_PXS`; eliminar el legacy de Pascal. +- **Esfuerzo**: small + +--- + +## Resumen ejecutivo + +### Top picks "bug latente / deuda clara" + +| # | Hallazgo | Esfuerzo | +|---|---|---| +| **21** | `Options::physics/audio/gameplay` se cargan del YAML pero nadie las consulta | small-medium | +| **28** | `core/` depende de `game/` (4 ficheros incluyen `game/options.hpp`) | medium | +| **11** | `Ship::isAlive() == isActive() == !isHit()` (tres alias del mismo bool) | small | +| **34** | `GameScene` construye `enemies_` dos veces (en ctor y en init) | small | +| **31** | `hit_timer_per_player_ = 999.0F` como sentinela | medium | +| **16** | `Rotation3D` es código muerto (28 líneas que nunca se ejecutan) | small | +| **1** | `GameScene::init()` no encaja en el lifecycle de `Scene` | small | + +### Top picks "limpieza con impacto" + +| # | Hallazgo | Esfuerzo | +|---|---|---| +| **22** | `defaults.hpp` (530 líneas, 22 includes) dividido en subarchivos | medium | +| **13** | `Entity::center_/angle_` mirror de `body_.position/angle` | medium | +| **25** | Constantes legacy del Pascal muertas en `Defaults::Physics` | small | +| **18** | `ShapeLoader::resolvePath` + `BASE_PATH` sin callers | small | + +### Hallazgos que requieren verificación más profunda antes de actuar +- **#5** (`SceneManager::actual`) +- **#9** (separación CollisionSystem/PhysicsWorld) +- **#20** (getters dead public) +- **#36** (`event.spawnejat`) + +### Hallazgos puramente opinables que el usuario podría rechazar legítimamente +- **#2, #4, #10, #14, #15, #19, #26, #27, #37, #39, #40** + +### Hallazgos estructurales claros (bug latente o deuda técnica medible) +- **#1, #6, #11, #12, #16, #21, #28, #31, #34** + +--- + +## Ya identificados y resueltos en la rama `chore/lint` (no incluidos arriba) +- Patrón draw-by-state de `GameScene` (refactorizado: complejidad 59 → 3) +- Complejidad de `TitleScene::update` (68 → 4) +- Duplicado `LetraLogo` Title/Logo +- Double `isActive()` en Bullet (redeclaración tras rename) +- Refactor de `DebrisManager::explode` (39 → 3) +- `Options::loadXXXConfigFromYaml` boilerplate (con templates `readField`) +- `PhysicsWorld::resolveBodyCollisions` (35 → 5) + +## Plan de ataque sugerido + +1. **#21** (en curso): borrar `Options::audio` y `Options::gameplay`, conectar `Options::physics` a runtime +2. **#16, #18, #24, #25** (dead code, commits limpios y rápidos) +3. **#34** (eliminar doble inicialización de `enemies_`) +4. **#11** (consolidar `isActive/isAlive/isHit`) +5. **#1** (limpiar lifecycle de `Scene`) +6. **#22 + #30** (partir `defaults.hpp` en subarchivos) +7. **#28** (mover `Options` a `core/`) +8. **#31** (enum `PlayerStatus` reemplazando 999.0F) +9. **#13** (eliminar mirror `center_/angle_`) + +A partir de ahí, decidir si seguir con el resto o pasar a otras prioridades (SDL_CALLBACKS, tuning de físicas).