diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..c1709be8d --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,163 @@ +# MaSzyna: notes for Claude Code + +## The code builds and works with GCC, not only MSVC + +The Linux CI (`.github/workflows/build.yml`) builds with GCC 13 on ubuntu-24.04, through the Sonar build-wrapper, and `sonar.yml` +analyses that build once it has passed. A change that compiles with MSVC only fails there, and a failed GCC build also leaves Sonar +without an analysis of the PR. So every change has to compile with GCC, and behave the same with it, as well as with MSVC. The CI +builds with `-DWITH_BETTER_RENDERER=OFF` and Sonar excludes `betterRenderer/`, so that part is neither built on CI nor analysed. + +Where no GCC is at hand, read the new code for what MSVC lets through and GCC doesn't: + +- `constexpr` initialised from a floating or class-type constant declared only `const`: make the source `constexpr` too. +- Standard headers MSVC pulls in through others: include them yourself. For example, `` for `std::views`, ``, + ``, ``, ``, `` and ``. +- `typename` and `this->` where a template needs them. +- A temporary bound to a non-const reference. +- Explicit specialisations inside a class. +- A name a class uses and then declares again in its own body (GCC: "changes meaning"). +- A nested type with default member initialisers used as a default argument inside the class that encloses it. +- A declaration changed in a header (`const`, parameters) without its definition, or a new member function defined but not declared. +- `std::ifstream` / `std::ofstream` opened with a `std::wstring`. +- MSVC-only functions: `_stricmp`, `sprintf_s`, `fopen_s`, `localtime_s` and the like. +- Designated initialisers out of declaration order. +- `emplace_back(a, b, c)` with parenthesised aggregate initialisation is fine on GCC 13, but not for `std::array`. There is no brace + elision in parentheses, so build a named local instead. + +The same behaviour needs portable code. Don't rely on the order arguments are evaluated in, on the signedness of `char`, on the size of +`wchar_t`, or on `long` being 32 bits. + +## SonarCloud: every remark on the changed code gets fixed + +SonarCloud (`sonar.yml`, `sonar-project.properties`) analyses each PR. A PR counts as finished only when every remark Sonar raises on +the code it changes is fixed. That means the code smells too, not just the bugs, the vulnerabilities and the quality gate. The analysis +runs on the GCC build, so a PR that fails to build with GCC gets no analysis at all. + +Fixing a remark must not change what the code does: + +- Keep the order of side effects, the conditions and the floating-point results as they were. +- Text in `STR(...)`, `STR_C(...)` and `_(...)` is looked up in the translations (`lang/*.po`), so it stays byte for byte the same. + The same goes for what is written to the log. +- `// NOSONAR: ` is only for a real false positive, or where a fix would change behaviour. Use it rarely, always with the + reason. + +Write new code so that it doesn't raise the rules that keep coming up: + +- **S134** (nesting deeper than 3 `if` / `for` / `while` / `switch` / `try`) and **S3776** (cognitive complexity over 25): + - use early `continue` / `return`; + - move loop bodies and branches into named helpers, preferably free functions in the anonymous namespace; + - group the state of a long algorithm in a small struct with methods. + + Every `else if`, every `&&` / `||` sequence and every nested lambda adds to the complexity. +- **S1188**: a lambda over 20 lines becomes a named function. +- **S107**: a function with more than 7 parameters takes a struct. +- **S3608**: lambdas capture explicitly (`[this, &points]`, not `[&]` or `[=]`). The only exception is `[&]` on a lambda passed + straight to a standard algorithm; `[=]` is flagged even there, and so is `[&]` passed to one of our own functions. +- **S6197** / **S5566**: use the `std::ranges::` algorithm that takes the container rather than a begin/end pair or a hand-written + loop. +- **S6165**: use `std::erase_if` rather than erase-remove. +- **S6004**: a variable used only by the condition is declared in the `if (init; condition)`. +- **S3358**: no nested `?:`; use an `if` or a small function. +- **S1121**: no assignment inside an expression, chained ones included (`a = b = c`). +- **S1117**: no declaration shadowing a local, a member or an enumerator. +- **S6003**: `v.emplace_back(a, b, c)` rather than `v.push_back({a, b, c})`. For a `std::array` element, push a named local instead + (see the GCC notes above). +- **S6012**: let class template argument deduction work: `std::vector values{1.0, 2.0}`. +- **S5945**: `std::array` / `std::vector` / `std::string`, not C arrays. +- **S1659**: one declaration per statement. +- **S6009**: `std::string_view` rather than `std::string const &` for a string parameter that is only read. +- **S6185** / **S6494**: `std::format` rather than concatenation or `snprintf`. +- **S924**: at most one `break` per loop. +- **S886**: a `for` whose counter changes in its body becomes a `while`. +- **S5997**: `std::scoped_lock` rather than `std::lock_guard`. +- **S6168**: `std::jthread` rather than `std::thread`. A `std::jthread` joins in its destructor where a `std::thread` would + terminate, so in existing code switch only where the thread is always joined or detached before it is destroyed. +- **S6177**: `using enum` where a function names many enumerators. +- **S5350** / **S995**: `const` on references and pointers that aren't written through. +- **S5817**: `const` on member functions that change nothing. +- **S2193**: no `float` loop counters. +- **S5421**: no non-const globals. +- **S6179**: `std::lerp` rather than a hand-written interpolation, in new code. In existing code only where it gives the very same + result. +- **S6164**: `std::numbers::pi` rather than a literal, in new code. In existing code only where it gives the very same result. + +The project is C++20 (`CMAKE_CXX_STANDARD 20`), so no C++23 library addition is available on any compiler. That rules out +`std::ranges::to`, `views::zip`, `views::enumerate`, `ranges::contains`, `std::print` and `std::expected`. + +## Renderers: the move to OpenGL 4.6 and Vulkan + +The renderers on `master` are to be replaced. `feat/gl46-migration` (PR #216, a draft with milestone 27.04, to be merged after the +26.11 release) replaces the OpenGL 3.3 renderer with an OpenGL 4.6 core one and adds a Vulkan 1.4 renderer beside it. It also +removes the legacy OpenGL 2.1, the OpenGL ES / ANGLE and the NVRHI (`betterRenderer/`) paths. + +- `betterRenderer/` will be removed. Don't put work into it. +- On that branch, of the `rendering/opengl*` files only `openglcamera.*` and `openglmatrixstack.*` stay. The whole + `rendering/opengl33*` family and the rest of the legacy `rendering/opengl*` go, and the new renderers are in + `rendering/opengl46/`, `rendering/vulkan14/` and `rendering/common/`. The shared `rendering/renderer.*`, `geometrybank.*`, + `particles.*` and the like stay. The shaders move from `shaders/` to `shaders/ogl46/` and `shaders/vk14/`. +- So a change on `master` to the code that goes away, or to the shaders, may be pointless. Keep it to fixes and to what a feature + on `master` needs in order to be drawn at all. For such a change, say in the PR where `feat/gl46-migration` has the counterpart, + or that the branch already carries the change. + +## Building, testing and running + +- The unit tests (`tests/unit`) run at the end of the build, which prints `Unit tests: N/N passed` and fails on any failed case. + `-DWITH_TESTS=OFF` builds the game without them. +- A new feature goes on its own branch from `master`, not on whatever branch is checked out. Say which branch the work lands on + before the first edit. A fresh worktree has empty submodules, so it doesn't build until they are filled. +- The editor runs a script when `EU07_EDITOR_SELFTEST=` is set: one command per line, `#` for comments. The commands are in + `application/editormodeselftest*.cpp` (`camera`, `wait`, `select`, `lay`, `screenshot`, `save`, `quit` and many more). + - Run it on a copy of the scenery, never on the original, and delete the copy afterwards. + - Give every run a timeout and keep an eye on its memory. A loop gone wrong can fill the RAM of the whole machine. + - `screenshot` only queues the grab and waits 3 frames; the image is written later on another thread. A click right after it + has been seen to lose the selection, so leave a `wait` of about 15 frames after a `screenshot` before clicking. +- At run time the editor's texts load from `lang/editor_.po` in the game folder (`utilities/translation.cpp`), not from + the build. New strings of a branch show in English until that file has them. Without the `fonts` folder Polish letters show as `?`. +- When a build runs on a copy of the game data made of links to the real folders, turn off whatever writes into the data + (`convertmodels 0` in `eu07.ini`), or the run changes the real game folder through the links. +- A test of code that moves, migrates or deletes files in the user's profile (`%APPDATA%`, `%LOCALAPPDATA%`, caches) gets its own + test folder. Never point it at the real profile. +- A GPU hang (Windows TDR) can freeze the whole machine, not just the game. Don't test a hang hypothesis by running it. Guard the + code against what causes it (degenerate geometry, NaN in the camera or in transforms) and read the `LiveKernelEvent` / watchdog + dumps instead. +- PowerShell variable names are case-insensitive: `$Track` and `$track` are the same variable. + +## Game data and backward compatibility + +The code and the game data live apart. The assets (sceneries, vehicles with their `.fiz` and `.mmd`, models, textures, sounds, +timetables, most of `lang` and the shipped `eu07.ini`) are in a separate SVN repository, `https://svn.eu07.pl/pctga`. It needs an +account; there is no anonymous checkout. Of `lang/`, only `editor_pl.po` is in this repository. Players install and +launch the game through Steam, GOG and the like, and run new builds on the data they already have. + +The official starter is StarterNG (`https://github.com/MaSzyna-EU07/StarterNG`), a separate repository; the older +`MaSzyna-EU07/Starter` is not the one in use. `launcher/` in this repository is the simulator's built-in scenario and vehicle +picker (`launcher_mode`), not StarterNG. + +- Every existing file must still be read the way it was. Don't rename or drop a keyword, change the meaning or the unit of a value, + or make an optional entry required. This holds for every format the game reads: for example `.scn` and its includes, `.fiz`, + `.mmd`, `.e3d` / `.t3d`, `.mat`, the `.txt` timetables and track profiles, `eu07.ini` and the Python screens. +- A bug fix may change what an existing file leads to, such as how a vehicle from its `.fiz` behaves, as long as the PR says so. +- Something new in a format is optional, and without it the code does exactly what it did before. A file that loaded correctly + before doesn't get new warnings or errors in the log. A real fault that is newly detected may be logged. +- A code change must not need a change in the SVN to work. Where it uses new or changed assets, it falls back to the old behaviour + without them, and the PR says what is to be added to the SVN and in what order the two go in. +- A change that uses new or changed assets (and falls back without them, see above) says so plainly in its commit message: which + files, and what is to change in them. It is never left implied. Before such a commit, either make the asset change right away in + the user's SVN working copy, or get the user's agreement to leave it for later. Ask the user where their SVN working copy is; the + installed game folder is not one. Pushing to git and committing to the SVN both need the user's consent. +- A scenery saved by the editor without using a new feature holds nothing the current release can't read. The entries of a new + feature (such as a new kind of node) appear only where that feature was used, and the PR says that the release doesn't read + them. +- Saving patches the scenery files by byte spans: unchanged definitions stay byte for byte, and an edited node is written anew by + its `export_as_text`. A change to saving must not widen what gets rewritten, nor make `export_as_text` drop more of an edited + node (comments, unknown tokens) than it did before. +- A new feature that changes what the simulator does or shows with existing sceneries and vehicles can be turned off in + `eu07.ini`, and turned off, the simulator behaves as it did before the feature. Editor tools and new optional entries of a + format are covered by the points above. The key is parsed in `utilities/Globals.cpp` (`global_settings::ConfigParse*`) and + written by `global_settings::export_as_text`, which puts the settings in the log at start. `eu07.ini` is read from the user's + profile (`user_config_path`), and from the game folder only when the profile has none; test the switch in the file that is + actually read. +- A feature doesn't make existing sceneries and vehicles look suddenly different after an update. A change to how things look is + something the player turns on, or comes in gradually. +- When a format really has to change, read both the old and the new form, and say in the PR what changes for the data and for + the people who make it.