System concepts and rewrite analysis
docs/concepts: one concept per engine system (how it works, invariants and traps, tests, what the review fixed), with an index. docs/analysis/ rewrite-proposals.md: the structural causes behind the review's defects and how to replace them, with reasons, costs and an order; analysis only. AGENTS.md points to both and lists the mods with unit tests.
This commit is contained in:
1 parent
b9dd464bd1
commit
a0207d500b
8 files changed
+666
-1
No files matched your search
@@ -3,6 +3,9 @@
|
||||
Instructions for AI agents (OpenCode, DeepSeek Harness, Claude Code, …). All agent-facing notes in this
|
||||
repository are written in English. Player-facing game text is Russian (see "Localization" below).
|
||||
|
||||
System concepts (how each engine system works, its invariants and traps) are in `docs/concepts/` — read the one
|
||||
for the system you change; larger rewrite proposals are in `docs/analysis/rewrite-proposals.md`.
|
||||
|
||||
Agent skills live in `.opencode/skills/<name>/SKILL.md` (canonical copy; `.dsh/skills` and `.claude/skills`
|
||||
point to it):
|
||||
|
||||
@@ -142,7 +145,7 @@ Details and log analysis — skill `skill-test`.
|
||||
|
||||
### Mod unit tests (`--run-tests`)
|
||||
|
||||
A mod declares `"test_sources": ["tests/*.cs"]` in `manifest.json` (currently `cooking`, `locale_atlas`, `dialog_base`, `items_base`, `quests_base`). Methods
|
||||
A mod declares `"test_sources": ["tests/*.cs"]` in `manifest.json` (currently `cooking`, `locale_atlas`, `dialog_base`, `items_base`, `quests_base`, `turn_battle_base`, `map_base`, `audio_base`). Methods
|
||||
`[ModTest("group/case")]` (static, `void`/`Task`, parameter `ModTestContext`; see `Core/Mods/Testing/`) are compiled
|
||||
into a separate assembly `<mod>__tests` and loaded into their own collectible context (they see the types of the
|
||||
mod and its dependencies).
|
||||
|
||||
@@ -0,0 +1,370 @@
|
||||
# What to rewrite, and why: analysis after the 2026-10-20/21 review
|
||||
|
||||
This is analysis only. Nothing here is implemented.
|
||||
|
||||
The review fixed about 35 defects across the engine (see `docs/concepts/*.md`, sections "Fixed in the
|
||||
review"). Most of them are not isolated slips. They are symptoms of a few structural decisions that
|
||||
keep producing the same kind of bug. This document names those decisions, shows the evidence, and
|
||||
proposes a replacement with its reasons and costs. The themes are ordered by how many defects they
|
||||
caused and how likely they are to cause more.
|
||||
|
||||
## Contents
|
||||
|
||||
1. [Mod-owned singletons survive mod reloads](#1-mod-owned-singletons-survive-mod-reloads)
|
||||
2. [One override rule, implemented once](#2-one-override-rule-implemented-once)
|
||||
3. [Long-lived state aliased by short-lived objects](#3-long-lived-state-aliased-by-short-lived-objects)
|
||||
4. [Global statics with implicit lifecycles](#4-global-statics-with-implicit-lifecycles)
|
||||
5. [Data that bypasses the entity pipeline](#5-data-that-bypasses-the-entity-pipeline)
|
||||
6. [Saves: atomicity and content that leaves a game](#6-saves-atomicity-and-content-that-leaves-a-game)
|
||||
7. [Dependency semantics in one place](#7-dependency-semantics-in-one-place)
|
||||
8. [Dead and half-built features](#8-dead-and-half-built-features)
|
||||
9. [Duplicated battle rules](#9-duplicated-battle-rules)
|
||||
10. [Logs as an oracle; Error-level noise](#10-logs-as-an-oracle-error-level-noise)
|
||||
11. [Polling and path lookups](#11-polling-and-path-lookups)
|
||||
12. [Patching through serialize → merge → deserialize](#12-patching-through-serialize--merge--deserialize)
|
||||
13. [Test infrastructure](#13-test-infrastructure)
|
||||
14. [Decision needed: scenario action precedence](#14-decision-needed-scenario-action-precedence)
|
||||
15. [Suggested order](#15-suggested-order)
|
||||
|
||||
---
|
||||
|
||||
## 1. Mod-owned singletons survive mod reloads
|
||||
|
||||
**Problem.** The core became a mod player, and mods can be reloaded in-process: hot reload, or a game
|
||||
switch where `ModReloadPlan` keeps mods whose code did not change. Subsystem mods still hold their
|
||||
data in process singletons (`static Instance`, `Lazy<T>`) that cached the collected data "once", with
|
||||
a `ModManager.IsInitialized` guard. A kept mod keeps its singleton, its statics and its cache.
|
||||
|
||||
**Evidence.**
|
||||
- `CharacterFactory`, `ActionCharacterFactory`, `ItemFactory`, `RecipeFactory`, `FileFactory` (skills,
|
||||
status effects) and `QuestFactory` served the previous mod set's data after a reload. A data edit
|
||||
under hot reload never reached the game. The extended `hot_reload` test proved it.
|
||||
- `MapZoneService` loaded zones once per process.
|
||||
- `UiRegistry`/`MenuRegistry` are fine only because they are `IGameRegistry` in `AppServices` and get
|
||||
`Clear`/`Rebuild`.
|
||||
|
||||
The fix applied (following the `EntityResolver` dictionary identity as a snapshot) works, but it is a
|
||||
convention every factory must remember.
|
||||
|
||||
**Proposal.** Make mod data an explicit, versioned catalog owned by the core:
|
||||
- `EntityCatalog<T>` (or `IModDataSnapshot`) is produced by the entity resolver per mod-set generation.
|
||||
It exposes `Generation` and `IReadOnlyDictionary<string, T>`, and is reachable from
|
||||
`AppServices.Current`.
|
||||
- Factories no longer keep their own copy. They cache *built* objects keyed by `(generation, id)`, a
|
||||
one-line wrapper (`GenerationCache<TKey, TValue>`).
|
||||
- Mod-owned services with runtime state (map zone overrides, quest state) register as `IGameRegistry`
|
||||
(Rebuild on reload) or `IResettable` instead of being static singletons.
|
||||
|
||||
**Why this way.** The defect class is "a cache that does not know which mod set it belongs to".
|
||||
Attaching the generation to the data itself makes staleness impossible by construction, instead of
|
||||
relying on each author to subscribe to a reload event. `AppServices` already has the
|
||||
registry/resettable loops; this extends a mechanism that exists rather than inventing one.
|
||||
|
||||
**Improves.**
|
||||
- Hot reload of data works everywhere.
|
||||
- Game switches cannot leak content between games.
|
||||
- Mod unit tests get a clean data view per test.
|
||||
- Fewer ad-hoc caches (five copies of the same pattern today).
|
||||
|
||||
**Cost.** Medium. About 8 factories, each a mechanical change. The QuestFactory state split (§4)
|
||||
should go with it.
|
||||
|
||||
## 2. One override rule, implemented once
|
||||
|
||||
**Problem.** The project has a clear rule (AGENTS.md: same name plus `depends` means the dependent mod
|
||||
wins). Each collector implements it by hand, and several got it wrong.
|
||||
|
||||
**Evidence.**
|
||||
- `ScenarioResolver` picked winners in scene-key order instead of load order.
|
||||
- `MapZoneService` iterated the registry dictionary, and the first definition won.
|
||||
- `ScenarioActionFactory` uses first-wins, silently (§14).
|
||||
- The patch docs promised wildcard-as-fallback, while the code overwrites.
|
||||
- Patch keys were matched case-sensitively while entities are read case-insensitively.
|
||||
- `ModCacheManager` shared one disk cache across mods for a shader name.
|
||||
|
||||
**Proposal.**
|
||||
- A single `ModContentIndex<TKey, TValue>` primitive that is fed `(modId, loadIndex, key, value)` and
|
||||
resolves winners by load index. It records conflicts for unlinked mods (today
|
||||
`ModConflictRegistry.RecordConflict` is called from some collectors only) and logs replacements in
|
||||
one format.
|
||||
- Every collector uses it: entities, files/textures, scenarios, zones, interactives, playlists,
|
||||
actions, shader caches.
|
||||
- Document precedence once (AGENTS.md "Adding vs replacing") and let `resource_atlas` read the same
|
||||
index.
|
||||
|
||||
**Why this way.** The rule is simple, and the bugs came from re-implementing it ten times with
|
||||
dictionaries whose iteration order is not load order. A shared primitive also gives the atlas and the
|
||||
runtime the same answer by construction (today the atlas re-derives the rule).
|
||||
|
||||
**Improves.** Overrides behave the same for every resource kind, conflicts are visible everywhere,
|
||||
and less code.
|
||||
|
||||
**Cost.** Medium. The collectors are small; the risk is in behaviour changes where first-wins was
|
||||
relied upon (actions; check with the content).
|
||||
|
||||
## 3. Long-lived state aliased by short-lived objects
|
||||
|
||||
**Problem.** Transient objects (battle UI, battle participants) hold or mutate long-lived objects
|
||||
(the roster's `Attributes`) by reference, and subscribe C# events on them.
|
||||
|
||||
**Evidence.**
|
||||
- `BattleSceneContext` sets `character.Attributes = characterInfo.Attributes`, so health persists by
|
||||
aliasing.
|
||||
- `BattleCardPanel`, `FFPlayerPanel` and `FFEnemyPanel` add lambdas to `Health.OnChange` and never
|
||||
remove them. Every battle leaves its panels and closures attached to the party's attributes. On
|
||||
unload they pin the battle mods' collectible contexts, which `ModEventBus.RemoveOwned` cannot see,
|
||||
because they are plain C# events.
|
||||
- Cached DTO dictionaries are "treated as immutable" by convention only.
|
||||
|
||||
**Proposal.**
|
||||
- Battles work on battle-scoped combatants (`BattleCombatant`: a copy of stats plus the status effect
|
||||
queue). `BattleSceneContext` writes the results (health, experience) back to the roster once at the
|
||||
end, in one place.
|
||||
- UI subscribes to battle-scoped events (owned by the battle scene, dropped with it), or through a
|
||||
disposable subscription token released in `_ExitTree`.
|
||||
- Catalog DTOs become `record`s or init-only types, so mutation of shared data is a compile error.
|
||||
|
||||
**Why this way.** Copy-in/write-back makes the persistence rule explicit and testable (one method),
|
||||
removes the subscription leak at its root, and matches how battle status effects are already kept
|
||||
off the model (`ConditionalWeakTable`).
|
||||
|
||||
**Improves.** No UI leaks across battles; battle mods unload cleanly; battles can be simulated or
|
||||
replayed without touching the roster.
|
||||
|
||||
**Cost.** Medium for battles (both cores read `Attributes` directly), low for records.
|
||||
|
||||
## 4. Global statics with implicit lifecycles
|
||||
|
||||
**Problem.** Much core state is static and reset by convention.
|
||||
- `ModEventBus` typed layer;
|
||||
- `SceneContextNode` name cache (with miss caching);
|
||||
- `ScenarioCondition.flags`;
|
||||
- `SceneManager.IsTestPlayActive`/`TestScenarioText`;
|
||||
- `GameLaunchArgs.Current`;
|
||||
- `ActiveGame`;
|
||||
- `QuestFactory` (definitions plus runtime state);
|
||||
- the `registered` flags in initializers.
|
||||
|
||||
**Evidence.**
|
||||
- `SceneContextNode` cached misses by name for all types: a wrong-type lookup hid nodes, and a
|
||||
wrong-type hit threw.
|
||||
- Tests must remember `SceneContextNode.Instance.ClearData()` (a recurring trap).
|
||||
- The unload pass needs `RemoveOwned` heuristics because subscriptions are not owned.
|
||||
- Quest state sits in a process singleton instead of the game runtime.
|
||||
|
||||
**Proposal.**
|
||||
- **Event bus.** An `IEventBus` instance in `AppServices`. `Subscribe` returns an `IDisposable` token,
|
||||
and each mod gets a scoped bus facade that disposes all its tokens on unload, so no ownership scan
|
||||
is needed. Dispatch by runtime type (walk base types, cached), which removes the
|
||||
"`Publish<ModEvent>` reaches nobody" trap.
|
||||
- **Scene services.** Instead of searching the tree by name, `ViewBox`, `DialogBox`, `TopView` and
|
||||
`Lights` register themselves into a typed `SceneServices` registry on `_EnterTree` and unregister on
|
||||
`_ExitTree`. Lookups become O(1) and always current, with no miss cache and no `ClearData` ritual.
|
||||
- **Game state.** Quest status and data, flags, map overrides and recruiting live in `RuntimeData`
|
||||
sections owned by their mods (saved and reset with the game), while definitions stay in catalogs
|
||||
(§1).
|
||||
- **Launch selection.** A `LaunchSelection` value passed through the pipeline instead of a mutable
|
||||
`GameLaunchArgs.Current` plus `ActiveGame` statics.
|
||||
|
||||
**Why this way.** Ownership and lifetime are what the bugs got wrong. Tokens, registries bound to the
|
||||
scene tree, and state inside the runtime turn lifetime into structure.
|
||||
|
||||
**Improves.** Reload and unload become simpler and safer; tests isolate without rituals; the class of
|
||||
"stale or global" bugs disappears.
|
||||
|
||||
**Cost.** High for the bus (28 typed subscriptions, GDScript bridge) and `SceneContextNode` (about 40
|
||||
call sites). Do it incrementally behind the current static facades.
|
||||
|
||||
## 5. Data that bypasses the entity pipeline
|
||||
|
||||
**Problem.** Several kinds of content are collected by hand-written walkers instead of the
|
||||
kind-routed `EntityResolver`:
|
||||
- map zones (`MapZoneService.ReloadFromMods`);
|
||||
- interactives (`InteractiveResolver`);
|
||||
- playlists;
|
||||
- scene definitions (merge semantics, legitimately different);
|
||||
- scenarios (file-name identity).
|
||||
|
||||
**Evidence.** Map zones had the wrong override rule and no conflict reporting. Interactives and zones
|
||||
get no patches. Each walker repeats the `ListFiles` recursion, JSON parsing and error logging.
|
||||
|
||||
**Proposal.**
|
||||
- Map zones and interactives become entity kinds (`[EntityKind("map_zone")]`,
|
||||
`[EntityKind("interactive")]`), so they get the override rule, patches, conflicts and atlas keys for
|
||||
free.
|
||||
- Keep custom resolvers only where the semantics really differ (scene definition merge, scenario
|
||||
headers), and give scenarios an explicit `id:` header (today the identity is the file name, a global
|
||||
namespace across mods).
|
||||
|
||||
**Improves.** One pipeline, consistent tooling; the validator and atlas see everything.
|
||||
|
||||
**Cost.** Low to medium (JSON envelopes already match).
|
||||
|
||||
## 6. Saves: atomicity and content that leaves a game
|
||||
|
||||
**Problem.**
|
||||
- `SaveService.Save` truncates and rewrites the only copy of a slot (not atomic).
|
||||
- `ModSaveableRegistry.CollectSaveData` catches a failing `OnSave` and *omits the slot*: one bug
|
||||
silently deletes a mod's data from the save.
|
||||
- Dormancy works per mod slot, but several slots hold content of many mods (quests_base holds every
|
||||
content mod's quests; items_base holds every item).
|
||||
|
||||
**Evidence.**
|
||||
- A null item made `ItemsSaveable.OnSave` throw, and the inventory and recipes vanished from that save.
|
||||
- Quests and items of a removed content mod were dropped (fixed with "unresolved" side lists).
|
||||
- A truncated slot made `Load` throw (fixed).
|
||||
|
||||
**Proposal.**
|
||||
- Atomic write: `slot.json.tmp`, flush, then `File.Replace(tmp, slot, slot.bak)`. Keep one backup
|
||||
generation, and fall back to `.bak` on a damaged load.
|
||||
- On `OnSave` failure, write the slot loaded for this session (kept in memory from the last load)
|
||||
instead of dropping it, and surface an error to the player.
|
||||
- Content-scoped dormancy: a slot can mark entries by owning mod (`entries[modId]`), so
|
||||
`SaveDormancy` keeps and revives them like whole slots. This replaces the ad-hoc unresolved lists in
|
||||
items and quests.
|
||||
- One IO path (System.IO on globalized paths) for save and load, so `SaveService` is testable in
|
||||
xUnit. Today Save and Load use Godot `FileAccess` and the rest uses System.IO.
|
||||
- A small save header (scene, time, game, composition) read without parsing the mod blobs. Today
|
||||
`ListSaves` deserializes every full save to draw the list.
|
||||
|
||||
**Improves.** A crash or a bug never costs the player a save; the "game keeps its saves while its
|
||||
mechanics change" promise holds for multi-mod slots too.
|
||||
|
||||
**Cost.** Medium; the save format stays compatible with a header in front.
|
||||
|
||||
## 7. Dependency semantics in one place
|
||||
|
||||
**Problem.** `ModManifest` exposes `Depends` (typed) and `DependencyIds` (ids, optional included).
|
||||
Callers picked the wrong view four times: the collection graph, `GameModSet`, the disabled filter and
|
||||
the mod manager treated optional dependencies as required. `ValidateDependencies` did not cascade.
|
||||
Version semantics are ad hoc (`^` is a prefix match, `=` a zero-padded string compare).
|
||||
|
||||
**Proposal.**
|
||||
- A `ModDependencyGraph` built once per discovered set, with explicit queries: `Required(id)`,
|
||||
`OrderingEdges(id)` (all present dependencies), `Dependents(id, requiredOnly)`,
|
||||
`Closure(id, requiredOnly)`. It is used by the filter, collections, mod manager, compiler closure
|
||||
and load order.
|
||||
- Adopt a semver range library (for example NuGet.Versioning) with documented range syntax.
|
||||
- Validation as a fixed-point resolution that returns `(accepted, rejected[reason])` instead of
|
||||
mutating dictionaries and `LoadFailureReasons` across three passes.
|
||||
|
||||
**Improves.** One definition of "needs"; fewer passes; understandable error reasons.
|
||||
|
||||
**Cost.** Low to medium.
|
||||
|
||||
## 8. Dead and half-built features
|
||||
|
||||
- **Flag conditions.** `FlagCondition` reads `ScenarioCondition.flags`, but `SetFlag` has no callers,
|
||||
the dictionary is never reset or saved, and DSL `set` writes dialog variables instead. The docs
|
||||
admit it. **Decide:** either implement game variables (a `GameVariables` section in `RuntimeData`
|
||||
that is saved, reset on a new game and written by `set` with a `global:` scope or a new `flag`
|
||||
command, publishing `ConditionStateChangedEvent`), or delete `FlagCondition` and the docs. Half a
|
||||
feature misleads mod authors.
|
||||
- **`StatusEffectQueue.OnTurnEnd`** has no callers (now a pure hook). Either a core calls it, or it
|
||||
goes away.
|
||||
- **Starter inventory hardcoded in items_base.** The engine mod seeds `food/wolf_meat` and
|
||||
`resource/branches` in the `Inventory` constructor, which is cthangover content in an engine mod.
|
||||
Every other game logs warnings or gets the items. Move it to the game: a collection setting or a
|
||||
content initializer like `cthangover_party`.
|
||||
- **`ModInitializerRegistry`** documentation claims a non-deterministic load order (it is
|
||||
deterministic now). Its two-way catch-up is complexity for a problem that no longer exists.
|
||||
|
||||
## 9. Duplicated battle rules
|
||||
|
||||
**Problem.** `PhysicsDamageActionCard` (card_battle) and `FFDamageExecutor` (ff_battle) are the same
|
||||
damage formula; the defence and stun executors are duplicated too. Neither clamps negative damage, so
|
||||
a future "shield" status hook that lowers damage below zero would heal the target and grow its
|
||||
defence.
|
||||
|
||||
**Proposal.** A `DamageModel` in turn_battle_base:
|
||||
- `Compute(user, target, multiplier)` → `(damage, defenceLoss)`, with the status hooks, `IgnoreArmor`
|
||||
and clamping in one place;
|
||||
- the cores keep presentation and turn flow;
|
||||
- status effect durations are already centralised in `StatusEffectQueue` (fixed this review).
|
||||
|
||||
**Improves.** One balance surface, testable with mod unit tests, and new cores get correct rules for
|
||||
free.
|
||||
|
||||
**Cost.** Low.
|
||||
|
||||
## 10. Logs as an oracle; Error-level noise
|
||||
|
||||
**Problem.** `skill-test` says every `Error` line is a red flag, but success paths log at `Error`.
|
||||
The victory screen logged "BattleData=found" on every battle (fixed), and similar lines remain
|
||||
elsewhere (search `LogLevel.Error` near "found"/"loaded"/"ENTER"). Tests therefore cannot use the
|
||||
error log as an oracle, and real errors drown.
|
||||
|
||||
**Proposal.**
|
||||
- A test (xUnit over the sources, like `KernelPurityTests`) that flags `LogLevel.Error` calls whose
|
||||
message has no failure condition (constant level with interpolated "found", "ENTER", "EXIT").
|
||||
- The TestRunner reports unexpected `Error` lines per integration test, with an allow-list for tests
|
||||
that provoke errors on purpose. That makes the error log a regression signal.
|
||||
|
||||
**Improves.** Errors mean errors, so regressions show up in the summary instead of hiding in logs.
|
||||
|
||||
## 11. Polling and path lookups
|
||||
|
||||
- `AudioService` polls settings every frame, and `SettingsData` has no change notification. Add a
|
||||
`SettingsChangedEvent` (section, key) published by the settings UI and loader, and subscribe.
|
||||
- Services are found by node path or name: `/root/AudioService`,
|
||||
`FindNode<Node>("AudioService") as IAudioService`, `/root/DialogRunner`. Register them in
|
||||
`AppServices` (`AudioService` is already a mod singleton) and look them up through a typed registry,
|
||||
which goes with the `SceneServices` idea from §4.
|
||||
|
||||
## 12. Patching through serialize → merge → deserialize
|
||||
|
||||
**Problem.** `EntityResolver.ApplyPatches` serializes every entity, merges JSON with `JsonDocument`,
|
||||
and deserializes again. It is slow (every entity of every kind on every collect), it lost patches
|
||||
whose key case differed (fixed), and only `[JsonExtensionData]` properties survive unknown keys.
|
||||
|
||||
**Proposal.** Patch at the `JsonNode` level *before* the first deserialization: read the raw item
|
||||
`JsonObject`, apply the wildcard and targeted patches (case-insensitive, array operations), then
|
||||
deserialize once into the DTO. The merger becomes a pure `JsonObject` transform that is easy to test.
|
||||
|
||||
**Improves.** One deserialization per entity, no lossy round trip, and patches see the authored JSON,
|
||||
not a re-serialization with defaults filled in. That would also make "wildcard as fallback for
|
||||
properties the entity did not author" possible if wanted, which the docs once promised.
|
||||
|
||||
## 13. Test infrastructure
|
||||
|
||||
- Integration tests reach private fields by reflection (`QuestsCoverageTestSetup` injects into
|
||||
`QuestFactory.dataByID`), which broke when the factory's internals improved. Provide a public test
|
||||
seam (a test data mod, or a `Register` API on the catalog) instead.
|
||||
- Coverage guards declared a test that did not exist (`coverage_audio`). Fixed by checking the
|
||||
manifest. Generalise: the TestRunner validates that every declared test exists, that every
|
||||
integration scene in `tests/Cthangover.Tests.Integration/Scenes` is in the manifest, and that every
|
||||
mod with `test_sources` is run.
|
||||
- **Boot cost.** About 75% of each integration test is booting the game (around 15 s on the 9p mount
|
||||
after this review's fixes). Grouping compatible scene tests into one process (a test-host scene that
|
||||
runs several setups in sequence with the mod unit test recovery cycle) would cut the full run from
|
||||
about 12 to about 4 minutes.
|
||||
- The xUnit suite writes into the working directory (`disabled_mods.json`) and the user profile
|
||||
(`LocalApplicationData/cthangover/mod_cache`). Give the core an overridable data root for tests.
|
||||
|
||||
## 14. Decision needed: scenario action precedence
|
||||
|
||||
`ScenarioActionFactory.Register` keeps the **first** registration of a name and ignores later ones
|
||||
without a log line. The class docs say mods can override core actions; the `Register` docs say core
|
||||
actions take precedence; the test `GDScriptScenarioAction_RegisterDuplicateName_Ignored` asserts
|
||||
first-wins. Consequences:
|
||||
- a kernel action can never be replaced by a mod;
|
||||
- between mods the *dependency* beats the dependent, the opposite of the resource rule;
|
||||
- a mod's same-named action is silently dead.
|
||||
|
||||
**Recommendation.** Use the one rule from §2: later in load order wins, with a warning when another
|
||||
type replaces. GDScript actions are registered with their mod's load index, so C# and GDScript order
|
||||
follow mod order. This changes a tested behaviour, so it was left for the owner to decide.
|
||||
|
||||
## 15. Suggested order
|
||||
|
||||
| Step | Theme | Why first |
|
||||
|---|---|---|
|
||||
| 1 | §6 atomic saves + no slot dropping | protects player data; small |
|
||||
| 2 | §1 data catalogs with generation | closes the largest bug class; enables §5 |
|
||||
| 3 | §2 one override primitive (+ §14 decision) | consistency across all content |
|
||||
| 4 | §7 dependency graph | small, removes a recurring mistake |
|
||||
| 5 | §3 battle combatants + §9 damage model | leaks and duplicated rules, contained in battle mods |
|
||||
| 6 | §10 log oracle | makes every later change safer |
|
||||
| 7 | §5 entity kinds for zones and interactives | after §1 and §2 it is mostly deletion |
|
||||
| 8 | §4 scoped bus, scene services, state in runtime | largest, do it incrementally behind facades |
|
||||
| 9 | §12 JsonNode patching, §11 polling, §13 test host | performance and comfort |
|
||||
@@ -0,0 +1,30 @@
|
||||
# System concepts
|
||||
|
||||
One file per engine system: what it is for, how it works, the invariants and traps, its tests, and
|
||||
what the 2026-10-20/21 review fixed in it. Read the concept before changing a system. The proposals
|
||||
for larger rewrites are in [`../analysis/rewrite-proposals.md`](../analysis/rewrite-proposals.md).
|
||||
|
||||
| System | File |
|
||||
|---|---|
|
||||
| Mod pipeline: discovery, filtering, compilation, registration, reload, unload | [mod-pipeline.md](mod-pipeline.md) |
|
||||
| Event bus and its GDScript bridge | [event-bus.md](event-bus.md) |
|
||||
| Data resolvers: entities, patches, files, textures, shaders, scenarios | [data-resolvers.md](data-resolvers.md) |
|
||||
| Collections, games and in-process switching | [collections-and-games.md](collections-and-games.md) |
|
||||
| Scenes, scenario composition, conditions, node lookups, `.tscn` | [scenes-and-conditions.md](scenes-and-conditions.md) |
|
||||
| Dialog queue, DSL and event chains | [dialog-queue.md](dialog-queue.md) |
|
||||
| Scenario actions and GDScript mods | [scenario-actions-and-gdscript.md](scenario-actions-and-gdscript.md) |
|
||||
| Saves and game state | [saves.md](saves.md) |
|
||||
| Characters | [characters.md](characters.md) |
|
||||
| Items, inventory, crafting | [items-and-crafting.md](items-and-crafting.md) |
|
||||
| Quests and the data factories | [quests.md](quests.md) |
|
||||
| Skills | [skills.md](skills.md) |
|
||||
| Battles and recruiting | [battles.md](battles.md) |
|
||||
| Time and lighting | [time-and-lighting.md](time-and-lighting.md) |
|
||||
| Audio | [audio.md](audio.md) |
|
||||
| World map | [world-map.md](world-map.md) |
|
||||
| Interactives | [interactives.md](interactives.md) |
|
||||
| Effects and Live2D | [effects-and-live2d.md](effects-and-live2d.md) |
|
||||
| UI, shell and menus | [ui-and-shell.md](ui-and-shell.md) |
|
||||
| Localization | [localization.md](localization.md) |
|
||||
| Tools: editors, studio, validator, atlases, mod manager, catalog | [tools.md](tools.md) |
|
||||
| Testing infrastructure | [testing.md](testing.md) |
|
||||
@@ -0,0 +1,72 @@
|
||||
# Dialog queue, scenario DSL and event chains (`mods/dialog_base`, `Core/UI/Dialog`, `Core/UI/Executable`)
|
||||
|
||||
Everything a player reads and clicks in a scene is a dialog queue: text, choices, backgrounds, music,
|
||||
actions and scene switches, compiled from a `.scenario` file. Scene events decide *when* a queue
|
||||
starts. The design is **event-driven**: nothing polls in `_Process`. Queues advance on clicks, timers
|
||||
and completion callbacks, and chains start dialogs when conditions or dialog ends change.
|
||||
|
||||
## Pieces
|
||||
|
||||
- **DSL → queue.** `ScenarioLoader`/`ScenarioConverter` parse a `.scenario` (header `scene:`,
|
||||
`priority`, `condition`, `is_one_run`, `after`, `save_allowed`, …, then commands). One
|
||||
`IScenarioCommandStrategy` per command (`text`, `select`/`option`, `goto`/labels, `if`, `set`,
|
||||
`background`, `music`, `effect`, `animation`, `action`, `switch_scene`, `end`, …) appends
|
||||
`ActionCommand`s to a `DialogQueue`.
|
||||
- **Runtime.** `DialogRuntime` holds the queue, variables, the current index, the hold flag (debugger)
|
||||
and the end actions.
|
||||
- `Run` executes actions until one waits (`WaitClick`, `WaitSelect`, `WaitTime`, `WaitEvent`).
|
||||
- A **generation** counter makes a run stop the moment the queue is replaced, ended or aborted
|
||||
inside an action.
|
||||
- `End()` is idempotent and publishes `DialogEndedEvent` (with the locker id) exactly once per queue.
|
||||
- Actions have a life cycle `Construct` → `Run` → `Destruct` (`DestructType` decides when). A
|
||||
`goto` loop gives every pass a full cycle.
|
||||
- **Box.** `DialogBox` (UI, the `IDialogBox` contract):
|
||||
- `SetDialogQueueAndRun` refuses while a queue runs (`IsRunning`: a locker or an unfinished queue,
|
||||
so locker-less click dialogs count too);
|
||||
- clicks call `NextAction`, choices call `SelectVariant` (an empty target just advances);
|
||||
- a wait-time timer is held off while the debugger holds;
|
||||
- `CompleteAction(action)` is the completion entry for `WaitEvent` actions (timed updates, animations).
|
||||
- **Timed and visual actions** use scene-tree timers and tweens, never update loops:
|
||||
- `ActionTimedUpdate` uses the real frame delta;
|
||||
- `ActionAnimatedForeground` schedules one `SceneTreeTimer` per frame, and only one animation is
|
||||
active;
|
||||
- `ActionEffect` instantiates into the `ViewBox` "ui" layer and frees itself when the queue ends;
|
||||
- `ActionBackgroundShowHide` keeps one shared tween.
|
||||
- **Event chains.** `ExecutableEvent`/`ScenarioEvent` are scene events with a condition and one-run
|
||||
state. `ExecutableMainEventChain` (one per scene, composed by `SceneManager`) is push-based. It
|
||||
subscribes to `ConditionStateChangedEvent` and `DialogEndedEvent`, coalesces them into one deferred
|
||||
scan, and starts the first startable event when the box is free. An event that just ended does not
|
||||
restart until the condition state changed after its end (the version guard against ping-pong).
|
||||
Standalone events use the same mechanism on their own.
|
||||
- **Interactives** run inline DSL and click dialogs through `/root/DialogRunner`
|
||||
(`IInteractiveDialogRunner`).
|
||||
- **Core side.** `SceneEventController` (autoload) dispatches the remaining per-frame and per-second
|
||||
listeners: ascending priority, stable order, tolerant of list changes during dispatch.
|
||||
`ConditionStateMonitor` bumps a version and publishes `ConditionStateChangedEvent`.
|
||||
|
||||
## Tests
|
||||
|
||||
- Mod unit tests `mods/dialog_base/tests` (18 cases: runtime end, goto loops, `if`, busy box, held
|
||||
delay, options, timed update, effect, animation, background tween, event controller order,
|
||||
push-started chain, refused runs).
|
||||
- Integration: `coverage_dialog`, `gdscript`, every `game_*` and autoplay test (`--dialogs=N` for hub
|
||||
games).
|
||||
|
||||
## Fixed in the 2026-10-20 review
|
||||
|
||||
There were 13 runtime and queue defects:
|
||||
- double `End`;
|
||||
- a missing-`goto` end;
|
||||
- lifecycle on loops;
|
||||
- `if` skipping actions;
|
||||
- busy-box overwrite;
|
||||
- a held delay skipped;
|
||||
- empty options;
|
||||
- a fixed 1/60 timed step;
|
||||
- effects never parented;
|
||||
- an animation stuck on loops;
|
||||
- background tweens fighting;
|
||||
- unstable event-controller order;
|
||||
- update-loop polling chains (replaced by push).
|
||||
|
||||
Details are in the commit "Dialog queue is event-driven and its runtime bugs are fixed".
|
||||
@@ -0,0 +1,38 @@
|
||||
# Localization (`Core/Localization`, `mods/locale_atlas`)
|
||||
|
||||
Player-facing text is Russian, and translations live in `.properties` files of every mod and every
|
||||
collection package. One Godot `Translation` per language is assembled from all of them at boot and
|
||||
after each mod reload.
|
||||
|
||||
## Loading
|
||||
|
||||
- `LocaleLoader.LoadCurrentLanguage` / `LoadLocale(lang)` merge into one dictionary:
|
||||
1. collection packages (`collections/<id>/locale`, then `user://collections`);
|
||||
2. every loaded mod **in load order** (later mods override keys).
|
||||
|
||||
Each source contributes `locale/<lang>.properties` and `locale/*_<lang>.properties` from the top
|
||||
level of `locale/` only. The result replaces the previous `Translation` of that language in the
|
||||
`TranslationServer` and becomes the current locale.
|
||||
- Format: one `key=value` per line. `#` starts a comment, the first `=` splits, both sides are
|
||||
trimmed, and there is no escaping.
|
||||
- Scenario text: the Russian literal sits in the `.scenario` (the fallback), and `key=` names the
|
||||
translation in `scenarios_<lang>.properties`, including `scenarios_ru`. The template game
|
||||
`my_novel_game` is the exception: English literals, Russian only in the properties.
|
||||
- C# translates through `TranslationServer.Translate` / `Tr`. A missing key shows the key itself;
|
||||
there is no fallback language.
|
||||
- Boot screens use `BootMessages` (before mods load), and `ILocalizationProvider` /
|
||||
`GodotTranslationProvider` let tools and the DSL translate without Godot in tests.
|
||||
|
||||
## Checking
|
||||
|
||||
`locale_atlas` (a tool mod, run as `--locale-atlas` or the `locale_atlas` test) parses declarations
|
||||
exactly like the loader and maps every reference: `.scenario` `key=`, C# `Translate`/`Tr`/`Locale =`,
|
||||
JSON text fields, `@key` in collections, `.tscn` texts, manifest names. It reports:
|
||||
- missing languages, undeclared references, empty values;
|
||||
- conflicting duplicates;
|
||||
- encoding problems;
|
||||
- warnings such as BOM, mixed line endings, placeholder mismatch, Cyrillic in `en`.
|
||||
|
||||
Report: `tests/artifacts/locale_atlas/report.md`.
|
||||
|
||||
No defects were found in this review.
|
||||
@@ -0,0 +1,49 @@
|
||||
# Scenario actions and GDScript mods (`Core/Actions`, `Core/Mods/ModGDScriptLoader`)
|
||||
|
||||
The scenario DSL drives the game through **actions**: named commands such as `quest.set_status`,
|
||||
`character.add_to_party`, `battle.init` and `map.open_zone`, written as
|
||||
`action <name> key=value …` in a `.scenario`. Actions are C# classes or GDScript objects. The
|
||||
same loader that registers GDScript actions also lets `.gd` files hook into events, conditions,
|
||||
item actions and mod lifecycle.
|
||||
|
||||
## Actions
|
||||
|
||||
- `IScenarioAction` has a `Name` and `Run(IActionContext)`. `ScenarioActionFactory` (an `AppServices`
|
||||
slot) discovers every public parameterless implementation in the loaded assemblies, kernel ones at
|
||||
construction. Mod assemblies are added through the `ScenarioActionConsumer` fan-out and removed on
|
||||
unload. **The first registration of a name wins** (see the rewrite analysis: this conflicts with the
|
||||
resource replace rule and needs a decision).
|
||||
- `IActionContext` is the facade an action gets:
|
||||
- `GetParam` reads dialog variables, set by DSL `set` or by earlier actions;
|
||||
- services: `IQuestService`, `ICharacterService`, `ILightingService`, `ISceneNodeService`,
|
||||
`IMapZoneService`, inventory, mod registry, logging.
|
||||
|
||||
Mods publish the implementations through kernel registries such as `QuestServiceRegistry` and
|
||||
`LightingBridge`. Gameplay mods pass extra data through dialog variables instead of growing the
|
||||
context.
|
||||
- Kernel atomic actions (`Core/Actions/Atomic`) cover party, skills, notifications, scene objects and
|
||||
panels. Quest, map, lighting, battle and recruiting actions live in their mods.
|
||||
- dialog_base's `ActionScenario` (the DSL `action` command) bridges a dialog queue to the factory.
|
||||
|
||||
## GDScript mods
|
||||
|
||||
`ModGDScriptLoader` (in `FinalizeGodotInit`, after the C# assemblies are registered) compiles every
|
||||
`.gd` matched by the manifest's `gd_sources`, instantiates it once, and inspects its methods:
|
||||
|
||||
| Pattern | Becomes |
|
||||
|---|---|
|
||||
| `get_name()` + `run(ctx)` | `GDScriptScenarioAction` in the action factory; `ctx` is a `GDActionContext` over `GDActionContextCore` (six null-safe GD service wrappers) |
|
||||
| `get_condition_name()` + `evaluate(flags, args)` | condition leaf parser registered first (`gds.<name>(...)`) |
|
||||
| `get_item_action_id()` + `use(item)` | item action through `ItemActionBridge` |
|
||||
| `on_mod_event_<key>(data)` | string-layer event bus subscription, with the mod id for cleanup |
|
||||
| `on_mod_loaded` / `on_mod_resources_ready` | `GDScriptModInitializer` |
|
||||
| subsystem hooks (`IGDScriptHook`) | claimed by mods, e.g. battle executors and victory-screen listeners in turn_battle_base |
|
||||
|
||||
One script can implement several patterns. GDScript sees data as Dictionaries with snake_case keys
|
||||
(`BattleGDScriptBridge`, the event bus payload converter).
|
||||
|
||||
## Tests
|
||||
|
||||
xUnit: action factory discovery, `GDActionContextCore`, `GDScriptScenarioAction`, loader patterns.
|
||||
Integration: `actioncontext` (31 checks), `gdscript` (the `dialog_base__gdscript` suite runs real
|
||||
`.gd` actions, conditions and initializers), `event_bus` (the GDScript bridge).
|
||||
@@ -0,0 +1,44 @@
|
||||
# Testing infrastructure (`tests/`, `Core/Mods/Testing`, mod `test_suites`)
|
||||
|
||||
Four layers, from the cheapest to the most complete. `tests/Cthangover.TestRunner` runs all of them
|
||||
as one command: build, then xUnit, then every integration test of `tests/tests.manifest.json`, then a
|
||||
summary and a JSON report.
|
||||
|
||||
| Layer | Where | Runs in | Use for |
|
||||
|---|---|---|---|
|
||||
| xUnit | `tests/Cthangover.Tests` | plain .NET, no Godot (Godot calls must be guarded by `GameEnvironment.IsRuntime`) | pure core logic: pipeline, resolvers, patches, saves, time, DSL parsing |
|
||||
| Mod unit tests | `mods/<mod>/tests` via `test_sources`, `[ModTest("group/case")]` + `ModAssert` | the real game process (`--run-tests [--mod=]`), compiled into `<mod>__tests` against the mod and its dependencies | mod code that needs Godot or mod types: dialog runtime, inventory, quests, status effects, map, audio |
|
||||
| Integration scenes | `tests/Cthangover.Tests.Integration` (dev build), one setup node per scene printing `[TEST_RESULT]` | a headless (or xvfb) Godot process per test | core runtime behaviour: event bus bridge, scene context, saves, coverage guards |
|
||||
| Mod test suites | `mods/<host>/test_suites/<id>` (virtual mod `<host>__<id>`, mounted by `activate` args) | full game, driven like a player | GUI flows: menu, inventory, hot reload, studio, tools, battles, dialog autoplay |
|
||||
|
||||
## Mechanics worth knowing
|
||||
|
||||
- **Mod unit tests.** Each test runs, then the runner recovers state:
|
||||
- `IResettable`s reset;
|
||||
- registries rebuild;
|
||||
- root nodes the test added are removed (removing a core node is an error);
|
||||
- `Engine.TimeScale` is restored;
|
||||
- the per-mod sandbox is wiped.
|
||||
|
||||
It then checks invariants. Async tests can await frames, and `TimeoutSeconds` applies to them. The
|
||||
report goes to `user://mod_tests/last-report.json`. Mods with tests: cooking, locale_atlas,
|
||||
dialog_base, items_base, quests_base, turn_battle_base, map_base, audio_base.
|
||||
- **Coverage guards.** `coverage` (battle cores) and `coverage_subsystems` (migrated subsystems) fail
|
||||
when a registered core or subsystem has no declared test, or when the declared test is not in
|
||||
`tests.manifest.json`.
|
||||
- **Dialog autoplay** (`dialog_base__autoplay`):
|
||||
- `--choices=` / `--timeout=` mount a driver that clicks through dialogs;
|
||||
- by default it passes when the box goes idle;
|
||||
- `--dialogs=N` passes after N finished dialogs instead (hub games never go idle).
|
||||
- **Runner conventions.** It adds `--all-mods` to tests without a game, and adds `--no-hot-reload`
|
||||
unless a test asks for `--hot-reload`. It checks scene path case before Godot starts. Logs go to
|
||||
`tests/artifacts/logs/<name>.log`.
|
||||
- **Speed.** A full run takes about 12 minutes on the 9p workspace. Most of each integration test's
|
||||
time is the game boot; mod compilation is cached and recompiles after every core rebuild.
|
||||
|
||||
## Rules (skill-code / skill-test)
|
||||
|
||||
- Test the public contract, check data rather than calls, and run JSON round trips for saved types.
|
||||
- Use reflection only to set up static singletons.
|
||||
- A failing test is never weakened. When it fails for a reason other than your change, ask.
|
||||
- Read the logs: every `Error` line is a red flag even when the test passes.
|
||||
@@ -0,0 +1,59 @@
|
||||
# Developer and player tools (`editor_tools`, `scenario_studio`, `validator`, `resource_atlas`, `locale_atlas`, `mod_manager`, catalog)
|
||||
|
||||
Tools are mods like everything else. A game includes them through the `dev_tools`/`novel_editor`
|
||||
collections or `--all-mods`. They build on the same registries the game uses, so what they show is
|
||||
what the game will do.
|
||||
|
||||
## editor_tools
|
||||
|
||||
- `NovelEditorHub` is the entry scene of the `novel_editor` collection and lists every
|
||||
`IToolProvider`'s button, as the main menu does.
|
||||
- The tool windows (`ToolWindow` chrome, `ToolTheme`) open on the tree root and survive scene
|
||||
switches:
|
||||
- the scenario editor, with play-test inside a running game (`SceneManager.IsTestPlayActive`);
|
||||
- the scene builder;
|
||||
- the map editor (zones, through `MapZoneService`);
|
||||
- the interactive editor;
|
||||
- the light editor;
|
||||
- catalog export (`ModCatalogExporter`: zip from the mod's content files plus a catalog JSON with
|
||||
archive and install SHA256).
|
||||
- Tests: `tools_windows` (every window opens with no engine error), `tools_hub`, `ingame_play`,
|
||||
`map_editor`, `scene_builder`.
|
||||
|
||||
## scenario_studio
|
||||
|
||||
An in-game scenario debugger and live editor, docked at `/root`. `StudioLive` installs a debugger into
|
||||
the dialog runtime: current line, pause, step, breakpoints, and dragging the next-line marker.
|
||||
- It feeds a journal from the event bus.
|
||||
- It edits scenarios into the folder mod itself, or into the per-game layer `user://studio/<game>`
|
||||
(`StudioLayer`, mounted through `ModOverlays`; it wins over every mod).
|
||||
- It restarts a scenario with state rollback (`StudioSnapshot` = `SaveService.BuildPackage` /
|
||||
`ApplyPackage`).
|
||||
- It exports the layer as a mod.
|
||||
|
||||
Test: `studio_live` (windowed).
|
||||
|
||||
## Static analysis
|
||||
|
||||
- `validator` runs in the background at start (settings: enabled, run on start, write report). It sees
|
||||
only `IModInfo`/providers, never Godot or resolver caches. Strategies run in (phase, order, id):
|
||||
contract strategies build a `ValidationGraph` (declarations and references), and reconciliation
|
||||
strategies report problems such as `depends`, entities and scenarios.
|
||||
- `resource_atlas` builds on the validator graph, plus texture/shader/scene/locale keys and C#
|
||||
literals, and reports cross-mod use without `depends`, ambiguous overrides and undeclared patches.
|
||||
It runs per game. Tests: `resource_atlas*`.
|
||||
- `locale_atlas`: see the localization concept.
|
||||
|
||||
## mod_manager and the catalog
|
||||
|
||||
- `ModManagerModel` merges local mods (`ModRegistry.AllDiscoveredMods`, enable state, deletability)
|
||||
with online catalog entries into cards, filters and dependency views.
|
||||
- Enabling or disabling cascades over **required** dependencies (`RequiredDependencyIds`).
|
||||
- Installs and removals are session overrides, with a restart banner.
|
||||
- `CatalogService` fetches and merges the catalogs of `ModConfig.CatalogUrls` (the highest version
|
||||
wins) with a disk cache.
|
||||
- `ModDownloader` downloads, checks the archive SHA256 and extracts to staging
|
||||
(`ZipFile.ExtractToDirectory`, which refuses entries outside the target), then verifies the install
|
||||
SHA256 and moves into `user://mods`.
|
||||
- `ModUninstaller` removes mods.
|
||||
- Test: `mod_manager`.
|
||||
Reference in new issue
Block a user