refactor: try improving overall code quality again - #72
Open
painfulexistence wants to merge 34 commits into
Open
refactor: try improving overall code quality again#72painfulexistence wants to merge 34 commits into
painfulexistence wants to merge 34 commits into
Conversation
The RHI layer merged after PR #55 reintroduced 53 C-style casts across the GPU backend (asset_manager, renderer, voxel_chunk_pass, gpu_pipeline, gpu_canvas_pass, gpu_render_target) — mostly (uint32_t)/(size_t)/(float) on sizes and handles. Convert them all to static_cast to match the rest of the engine (clang-tidy google-readability-casting), bringing the engine's C-style-cast count back to zero. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
Two unscoped enums the PR #55 sweep missed: - MessageType (message.hpp): a top-level unscoped enum leaking ON_QUIT, DRAW_CALL, etc. into the global namespace. Call sites already qualify (MessageType::ON_QUIT), so enum class is a clean, zero-ripple change. - Plane::Halfspace (frustum.hpp): nested so not globally polluting, but the struct also had a method named Halfspace() returning Halfspace — a name-lookup hazard. Make it enum class, rename the method to ClassifyPoint(), and replace the value-dependent '< 0' checks in Frustum::Intersects with '== Halfspace::NEGATIVE'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
globals.hpp defined PI as 3.1416 — a lossy 5-significant-figure value used in camera clamps and sphere-mesh generation. Replace it and the sibling #define constants (GRAVITY, FIXED_TIME_STEP, CAMERA_*) with full-precision, typed 'inline constexpr float' values: scoped, debuggable, no macro substitution surprises. Feature-toggle macros used in #if (MSAA_ON, RUNTIME_LOG_ON, SINGLE_THREAD, SHOW_*_COST) stay macros — the preprocessor can't see constexpr. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
…macros Splits config.hpp by a clear rule: values (window/framebuffer dimensions, shadow map size, light counts, MSAA samples, and the derived GL texture-unit reserve indices) become typed inline constexpr; feature toggles (SINGLE_THREAD, RUNTIME_LOG_ON, MSAA_ON, VSYNC_ON, SHOW_*_COST, FRUSTUM_CULLING_ON) stay #define because several are consumed by #if. Verified none of the converted constants appear in a preprocessor conditional. Note: the texture-index chain (SHADOW_MAP_COUNT → SCENE_TEXTURE_BASE_INDEX) currently has no external users — kept as constexpr for the texture-unit allocator rather than deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
…e-comparison-oofdu6
painfulexistence
changed the title
The video (recorder/player), RmlUI, renderer, asset_manager, and physics_subsystem_3d modules used the m_ member prefix (inherited from their FFmpeg/RmlUI/WebGPU heritage), while the rest of the engine — and the naming pass already applied on main — uses a leading-underscore _camelCase. Rename the 450 m_ members to _ across those modules so there is one member convention engine-wide. Verified before renaming: no m_X/_X collisions, no cross-module m_ access, no m_ inside string literals; miniaudio's ma_ prefix is untouched (word boundary) and s_/g_ static prefixes are preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
Introduces class Log with static Debug/Info/Warn/Error taking a compile-time-checked fmt::format_string plus args, dispatching to spdlog (native) / the browser console (web) from one place (log.cpp). It is stateless and non-instantiable (constructor deleted), so it is safe to call before any subsystem exists — unlike routing through ConsoleSubsystem::Get(). ConsoleSubsystem::Info/Warn/Error now forward to Log instead of duplicating the platform dispatch, so the existing LOG/ENGINE_LOG macros already funnel through Log. Nothing else changes yet — call-site migration off the seven ad-hoc logging styles onto Log:: follows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
Replaces all 37 ENGINE_LOG/LOG call sites with Log::Info and removes both macro definitions. Two ENGINE_LOG(fmt::format(...)) sites that double-formatted are unwrapped to a single Log::Info(fmt, args). The [Engine] prefix is dropped — most of those lines already carry their own tag ([FileSystem], [Scene], …), and a bespoke logging macro isn't worth a preprocessor layer that hurts tooling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
Routes the 49 direct spdlog::info/warn/error/debug call sites through Log (only log.cpp — the backend — still calls spdlog). All used literal format strings, so the mapping is direct. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
79 call sites across 21 files routed to Log via a balanced-paren parser
that handled all three shapes:
Console->Info(fmt::format("x {}", a)) -> Log::Info("x {}", a) (unwrap)
Console->Info("literal") -> Log::Info("literal")
Console->Info(runtimeString) -> Log::Info("{}", runtimeString)
Receivers were exactly ConsoleSubsystem::Get() / _console / con, and no
non-Console class exposes Info/Warn/Error, so nothing else was touched.
ConsoleSubsystem::Info/Warn/Error remain (delegating to Log) for the
command-palette code paths.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
47 fmt::print sites → Log: fmt::print(stderr, ...) becomes Log::Error and plain fmt::print(...) becomes Log::Info. A single trailing \n in each format string is dropped (Log/spdlog append their own newline); mid-message newlines are preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
…dation
The Emscripten memory-stats printf block becomes Log::Info (%.2f -> {:.2f}),
and the SDL init/window/context failure SDL_Log calls become Log::Error
(%s -> {}), trailing newlines dropped. All seven ad-hoc logging styles
(spdlog::, fmt::print, cout/printf, SDL_Log, direct Console, LOG/ENGINE_LOG)
now funnel through the single Log front-end.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
…e-comparison-oofdu6 # Conflicts: # Engine/src/rmlui_manager.cpp
…e-comparison-oofdu6
The old UIPage + UIPageManager was a parallel object model to Component (duplicate OnAttach/OnDetach/OnUpdate lifecycle) that no code used: zero UIPage subclasses, zero callers of any UIPageManager method — it was constructed, owned by Application, and ticked empty every frame. Retire it and fold the idea into the component architecture: UIPageComponent : Component loads an RmlUi document through the standard component lifecycle, owns the Rml::EventListener->std::function adapters, and exposes GetElement/AddListener/Show/Hide helpers. Games attach it to a GameObject instead of hand-rolling RmlUi glue. Terrain's HUD is the first consumer: it drops its own RmlEventCallback adapter class, the _listeners ownership vector, the AddListener helper, and the manual LoadDocument/Show lifecycle (-22 lines), keeping only its domain logic. Also removes the Application _uiPages member, the game_layer tick, and the umbrella/CMake entries for the old system. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
Build fix: examples include the engine only through the "Atmospheric.hpp" umbrella (their target sees -IEngine/include, not Engine/include/Atmospheric), so the direct `#include "log.hpp"` added during the logging migration failed to resolve. Add log.hpp to the umbrella and drop the direct includes from the seven example/component files, matching how every other engine header is reached. Format fix: run clang-format 21 (the repo-pinned version) across the checker's file set; the logging migration and m_ -> _camelCase renames had left 41 files unformatted. Changes are purely whitespace/reflow and include ordering.
The m_ -> _camelCase private-member rename wrongly rewrote inherited Bullet members in Physics3DSubsystem::Raycast: ClosestRayResultCallback's m_hitPointWorld / m_hitNormalWorld / m_collisionObject are library fields, not ours, and must keep their m_ names. Also fixes the RaycastCallback's own members that came out as the double-prefixed _m_rayFromWorld / _m_rayToWorld (the in-file comment already referred to the intended _rayFromWorld / _rayToWorld). Only physics_subsystem_3d.cpp was affected: it is the sole renamed TU that subclasses/accesses a third-party type using the m_ convention.
Animator2D::Play built its message as `"Animation not found: " + name`, a
runtime std::string. Log::Error's first parameter is fmt::format_string, which
must be a compile-time constant, so this failed to compile ("name is not a
constant expression"). Pass the name as a {} argument instead. This was the
only Log:: call in the codebase that concatenated into the format string.
The LuaScripting example embeds the Lua frontend .cpp/.hpp files directly and compiles them with the example's include set (-IEngine/include, -IEngine/frontends/lua) which lacks -IEngine/include/Atmospheric. The bare `#include "log.hpp"` added during the logging migration only resolved in the engine-library build, so the example failed with "log.hpp file not found". Use the Atmospheric/-qualified path, matching how these files already include every other engine header (e.g. "Atmospheric/gfx_factory.hpp"); it resolves via -IEngine/include in both the library and example compilations.
The logging migration collapsed a run of statements in the Emscripten-only
memory-stats dump into one run-on expression: `Log::Info(...) Log::Info(...)
if (...) {...}` with no `;` separators, causing "expected ';' after expression"
on the WebAssembly build (desktop never compiles this __EMSCRIPTEN__ block, so
it stayed green). Restore one statement per line and the proper if/else. A
full scan confirms every other Log:: call in the tree is correctly terminated.
The m_ -> _camelCase rename also rewrote basis_universal struct members in the KTX2 loaders: basist::ktx2_image_level_info's m_total_blocks / m_width / m_height / m_orig_width / m_orig_height are library fields and must keep m_. These accesses live in #ifdef AE_USE_BASIS_UNIVERSAL / __EMSCRIPTEN__ blocks that the desktop matrix never compiles, so only the WebAssembly build caught them. Second and final third-party type the rename touched (Bullet was the other); a scan of the other renamed TUs' platform-guarded blocks found no more.
Scene/SceneNode (raw-pointer parent/children, root permanently null) had no users: nothing ever constructed a node, and Application's std::vector<Scene> member was never touched. Real scene management is the named-container system (GoScene/AddScene/UnloadScene + SceneBlueprint); entity hierarchy lives in GameObject itself. This is the same anti-pattern retired twice already this review — a parallel object model shadowing the real mechanism (UIPage/UIPageManager here, the Node graph in vapor) — kept alive only by the transitive includes it happened to provide, which now move to the files that actually need them.
…e-comparison-oofdu6 # Conflicts: # Engine/src/application.cpp # Engine/src/gpu_canvas_pass.cpp # Engine/src/renderer.cpp # Engine/src/voxel_chunk_pass.cpp # Examples/Terrain/main.cpp # Examples/VoxelWorld/main.cpp
The main merge cleanly combined two independent changes to Frustum: this branch renamed Plane::Halfspace() to ClassifyPoint() and made the enum an `enum class` (29db129), while main added a box-corner Intersects(array<vec3,8>) overload still calling the old `plane->Halfspace(points[i]) >= 0`. Neither side conflicted textually, so the stale call compiled straight into the merge. Use ClassifyPoint() and express the old `>= 0` ("not in the negative halfspace") as `!= Plane::Halfspace::NEGATIVE`, which the scoped enum requires.
…e-comparison-oofdu6 # Conflicts: # Examples/VoxelWorld/main.cpp
…e-comparison-oofdu6 # Conflicts: # Engine/src/asset_manager.cpp
…e-comparison-oofdu6 # Conflicts: # Engine/include/Atmospheric.hpp
…e-comparison-oofdu6 # Conflicts: # Engine/CMakeLists.txt # Engine/include/Atmospheric/application.hpp # Engine/src/animator_2d.cpp # Engine/src/application.cpp # Engine/src/asset_manager.cpp # Engine/src/gfx_factory.cpp # Engine/src/gpu_pipeline.cpp # Engine/src/scene_loader.cpp # Examples/VoxelWorld/main.cpp
…M_PI)
Strict review of main's newly-merged code found it largely conformant (named
casts, _-prefixed members, enum class, typed handles — no m_, NULL, or C-casts).
Two standard violations fixed:
- easing.cpp defined and used the POSIX M_PI macro; replace with a typed
`inline constexpr float PI = std::numbers::pi_v<float>`.
- The prefab importers (prefab_gltf/prefab_usd) and the GLTF/USD viewer examples
logged through ConsoleSubsystem::Get()->Warn/Info; route them through the Log::
front-end instead (14 sites), unwrapping the redundant fmt::format wrappers and
converting two runtime string-concatenations to {} placeholders.
…e-comparison-oofdu6 # Conflicts: # Engine/include/Atmospheric/frustum.hpp # Engine/src/asset_manager.cpp # Engine/src/frustum.cpp # Examples/Terrain/main.cpp
Retire the Log:: front-end in favour of one-liner macros over spdlog, the single sink ConsoleSubsystem already configures (level, pattern, overlay radios). Two categories by prefix: ENGINE_DEBUG/INFO/WARN/ERROR — engine internals -> "[Engine] ..." APP_DEBUG/INFO/WARN/ERROR — game/example code -> "[App] ..." - New logging.hpp defines the macros; spdlog checks the format string at compile time (same guarantee Log's fmt::format_string gave), so the format must be a literal. RUNTIME_LOG_ON=0 expands them to ((void)0), so arguments are not even formatted when logging is off. - No separate web path: Emscripten maps stdout to the browser console, so the redundant EM_ASM branch is gone (spdlog is already linked on WASM). Deletes log.hpp / log.cpp. - Migrated ~275 call sites: Log:: -> ENGINE_*/APP_* by directory; ConsoleSubsystem::Get()->Info/Warn/Error -> macros (unwrapping fmt::format, dropping manual [Engine] prefixes, turning runtime string concatenation into format placeholders). - Removed the now-redundant ConsoleSubsystem::Info/Warn/Error passthrough; ConsoleSubsystem stays the spdlog configurator/router. Direct spdlog:: calls that carry their own finer subsystem tag (e.g. "[Physics]") are intentionally left as-is — a second, finer level, not the Log/ConsoleSubsystem redundancy this change targets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
The last raw spdlog::info/warn/error/debug call sites in the engine (networking — websocket/udp/relay/network — plus a few in application.cpp and scene_loader.cpp) now go through the ENGINE_* front door like everything else, so nothing outside logging.hpp and ConsoleSubsystem's spdlog configuration touches spdlog directly. Tags are kept as written (colon-style "WebSocketClient: ...") rather than normalized to brackets: ~40% of the existing ENGINE_* call sites already use colon tags (SceneLoader:, LoadGLTF:, ...), so folding these in as-is is the consistent choice, and the [Engine] prefix now fronts them all ([Engine] WebSocketClient: ...). A uniform colon-vs-bracket sweep is left as a separate low-value cleanup; structured per-subsystem categories wait until the ConsoleSubsystem overlay actually renders a filterable log view. Swapped the folded files' <spdlog/spdlog.h> include for logging.hpp (which pulls spdlog), and dropped the now-redundant direct include from application.cpp / scene_loader.cpp. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
…e-comparison-oofdu6
…e-comparison-oofdu6 # Conflicts: # Engine/src/mesh.cpp # Engine/src/prefab_usd.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to the earlier code-quality PR. The RHI/WebGPU layer that landed afterward reintroduced patterns the engine had already cleaned up; this restores the standard and fixes two unscoped enums the previous sweep missed.
Changes
Named casts (RHI layer)
asset_manager,renderer,voxel_chunk_pass,gpu_pipeline,gpu_canvas_pass,gpu_render_target— tostatic_cast(mostly(uint32_t)/(size_t)/(float)on sizes and handles). The engine's C-style-cast count is back to zero.enum class
MessageType(message.hpp): top-level unscoped enum leakingON_QUIT/DRAW_CALL/… into the global namespace →enum class. Call sites already qualify, so no ripple.Plane::Halfspace(frustum.hpp): nested (not globally polluting) but the struct also had a method namedHalfspace()returningHalfspace— a name-lookup hazard. Made itenum class, renamed the method toClassifyPoint(), and replaced the value-dependent< 0checks with== Halfspace::NEGATIVE.Notes
unique_ptr,enum class, builder pattern) — this only touches the implementation-level cast/enum hygiene.🤖 Generated with Claude Code
https://claude.ai/code/session_01HSL8DuPuohwywCNThXGsAG
Generated by Claude Code