feat(Function): mission system

[CloudeaSoft]

No description provided.

[github-actions]

Holistic Review

Motivation: This PR introduces a comprehensive mission system for the Everglow mod, split into world-side (multiplayer-synced, world-saved) and player-side (per-player, player-saved) halves. The scope is justified—Everglow needs structured content progression.

Approach: The architecture is well-designed: server-authoritative world-side with 3 routing destinations (WorldOnly/MainServer/AllDownstream), a clean DSL for building mission objectives (Add/AddParallel/AddOptional/AddBranch), an ISidebarElement interface replacing the old abstract class, and a FontStashSharp-based text rendering pipeline. The partial-class decomposition of WorldMissionBase (Metadata/Behavior/Netcode/Persistence/Presentation) is excellent. However, the PR contains a few significant issues and several warnings.

Summary: ⚠️ Needs Human Review. This is a large, well-structured mission system with good architecture but a critical localization-key deletion violation, a null-event NRE risk, and a false-positive unit test. The PR author should address the localization issue before merge. A human reviewer should also verify the multiplayer routing logic in production conditions and confirm that the FontStashSharp DLL licensing is compatible with GPL-3.0.

---

Detailed Findings

❌ Localization Key Deletion — AcytaeaScratch_TownNPC.DisplayName removed

Localization/en-US_Mods.Everglow.hjson deletes the entry AcytaeaScratch_TownNPC.DisplayName: Acytaea Scratch_ Town N P C (previously at line ~4903). This violates AGENTS.md:「本地化键只增不删」— localization keys must never be deleted, as they can break content lookup and save compatibility.

If the underlying content class (AcytaeaScratch_TownNPC) was renamed or removed, all references (including the hjson key, ItemID/NPCID.Search lookups, and save files) must be globally searched and updated. If it still exists but had a typo in the display name, the old key must be preserved—only the value should be changed.

Warning

This is a must-fix before merge. Revert the deletion or ensure the internal name and all references are properly migrated.

---

⚠️ TriggerOnKillNPCEvent — Null-forgiving Invoke on Static Event

Sources/Everglow.Function/Mechanics/Mission/Hooks/MissionGlobalNPC.cs:17 calls OnKillNPCEvent.Invoke(npc) without a null-conditional operator. While PlayerMissionManager.Load() subscribes to this event early in MissionSystem.Load(), the null-forgiving pattern is fragile:

• If mod loading partially fails, the event may not be subscribed
• Adjacent code in SpecialOnKill (line 22) correctly uses OnNPCKilled?.Invoke(npc) — inconsistent style
• Future refactors that remove the subscription would silently cause NREs

(Inline comment with suggestion posted on the file.)

---

⚠️ MissionManagerTest.TimeLimitTest() — Vacuous Test (Commented-Out Body)

Sources/Everglow.UnitTests/Function/MissionSystem/MissionManagerTest.cs:50-73 — The entire method body is commented out. This test passes without asserting anything. If the test cannot run in the current environment (e.g., the WorldMissionManager constructor requires tML runtime types), add [Ignore] with a clear reason. A vacuous test that "passes" by doing nothing is worse than no test — it creates false confidence.

(Inline comment with suggestion posted on the file.)

---

$$⚠️ build.txt — BOM Removed (Unnecessary Diff)

Sources/Everglow/build.txt: The UTF-8 BOM (\u2060) at the start of the file was removed. This is a cosmetic change that creates unnecessary diff noise. Per AGENTS.md「最小改动」principle, unrelated formatting changes should be avoided.

---

💡 WorldCollectItemObjective — Dictionary Key Collision Risk

Sources/Everglow.Function/Mechanics/Mission/WorldSide/Objectives/WorldCollectItemObjective.cs uses SubworldSystem.Current?.Name ?? "MainWorld" as a dictionary key. If a subworld has a namespace that evaluates to null at runtime (unlikely but possible), the fallback "MainWorld" could collide with the actual main world entry. Consider using a more robust key scheme, e.g., a tuple of (SubworldSystem.Current?.GetType().FullName ?? "MainWorld").

---

💡 Auto-Generated Display Name — SlashProjectile3 D has unexpected space

Localization/en-US_Mods.Everglow.hjson:4532: SlashProjectile3D.DisplayName: Slash Projectile3 D contains an unexpected space in "3 D". This looks like an auto-generation artifact. Verify the intended display name.

---

✅ Positive Aspects

1. Architecture: The separation of world-side vs player-side mission systems, RouteDestination enum for net routing, and the StructuralObjectiveContainer DSL are clean and maintainable.
2. ISidebarElement refactoring: Replacing the abstract class with an interface with default methods is a good modernization. The GetOutOfTheRoom adoption in SubSpace module is clean.
3. FontStashSharp integration: The FontManager class with ILoadable integration is well-implemented, using GetFileBytes and GetFileNames correctly.
4. Persistence/Netcode separation: The partial-class decomposition of WorldMissionBase into *Metadata.cs, *Behavior.cs, *Netcode.cs, *Persistence.cs, *Presentation.cs is excellent.
5. Unit tests: The PlayerMissionBasePersistenceTest with [DataRow] parameterized tests and the BasisTest.RemainingData_Should_BeForwardedWithoutParsing are well-written.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement human review.

Generated by Everglow Holistic Review for #380 · ⊞ 13.7K

[github-actions]

⚠️ 测试方法体被完全注释掉,测试形同虚设并通过。

TimeLimitTest() 的整个方法体被注释掉了,这意味着该测试永远通过,但不会验证任何行为。这是一个假阳性。

Suggested change

	public void TimeLimitTest()
	[TestMethod]
	public void TimeLimitTest()
	{
	var provider = new TestStateProvider();
	provider.TimeForVisualEffects = 60;
	var manager = new WorldMissionManager(provider);
	manager.AddMission(new TestMission2());
	var m = manager.GetMission<TestMission2>();
	m.Initialize();
	Assert.IsNotNull(m);
	for (int i = 0; i < 30; i++)
	{
	manager.Update();
	if (i < 20)
	{
	Assert.AreEqual(WorldMissionState.Active, m.State);
	}
	else
	{
	Assert.AreEqual(WorldMissionState.Failed, m.State);
	}
	}
	}

如果该测试当前因环境限制无法运行,应添加 [Ignore] 注解并说明原因,而不是注释掉测试体。

[github-actions]

⚠️ OnKillNPCEvent.Invoke(npc) 缺少 null 检查。

OnKillNPCEvent 是静态事件,在无订阅者时调用 .Invoke() 会抛出 NullReferenceException。虽然当前运行路径下 PlayerMissionManager.Load() 在 MissionSystem.Load() 中提前订阅了此事件,但以下场景仍有风险:

1. Mod 加载部分失败时,事件可能未被订阅
2. 未来代码重构时,订阅代码可能被移除或条件化
3. 相邻方法 SpecialOnKill 中 OnNPCKilled?.Invoke(npc) 正确地使用了 null 条件运算符,风格不一致

Suggested change

	OnKillNPCEvent.Invoke(npc);
	OnKillNPCEvent?.Invoke(npc);

[github-actions]

Holistic Review

Motivation: This PR introduces a comprehensive mission system for the Everglow mod with player-side missions (per-player, player-saved). The scope is justified—Everglow needs structured content progression and the architecture (player/world separation, objective DSL, presentation layer) is well-documented in the README and TODO docs.

Approach: The architecture remains well-designed: PlayerMissionManager as a static manager with PlayerMissionBase, structured objectives (KillNPC, CollectItem, ConsumeItem, TalkNPC, GiveItem, Explore, etc.), special structures (Parallel, Branching), a MissionView presentation layer, and a MissionObjectiveContainer with linked-list traversal. However, the previous review's critical issues remain entirely unaddressed.

Summary: ⚠️ Needs Human Review. The only changes since the previous review are trailing newline normalization (adding newline to files that were missing them). None of the previously identified issues have been resolved: the localization key deletion (AcytaeaScratch_TownNPC.DisplayName) remains, the null-forgiving event invoke persists, the vacuous test is still commented-out, and cosmetic diffs remain. A human reviewer must check these before merge.

• review PRR_kwDOIA7Lc88AAAABHRrEUg — commit 74130d3a9, verdict was ⚠️ Needs Human Review, now ⚠️ Needs Human Review; assessment unchanged (the only incremental change is trailing newline normalization; all substantive issues remain).

---

Detailed Findings

All findings from the previous holistic review remain applicable and unaddressed:

• ❌ Localization Key Deletion — Localization/en-US_Mods.Everglow.hjson still deletes AcytaeaScratch_TownNPC.DisplayName. Per AGENTS.md the localitaion keys must never be deleted.

• ⚠️ TriggerOnKillNPCEvent — Null-forgiving Invoke — MissionGlobalNPC.cs:17 still calls OnKillNPCEvent.Invoke(npc) without null-conditional (while SpecialOnKill on line 22 correctly uses ?.).

• ⚠️ MissionManagerTest.TimeLimitTest() — Vacuous Test — The entire method body remains commented-out.

• ⚠️ build.txt — BOM Removal (Unnecessary Diff) — Unrelated BOM removal persists.

• 💡 WorldCollectItemObjective — Dictionary Key Collision Risk — Same design concern.

• 💡 SlashProjectile3D.DisplayName: Slash Projectile3 D — Unexpected space persists.

✅ Incremental Changes Verified

The incremental changes in this revision (commits 74130d3a9..7228f95ce) consist entirely of adding trailing newlines to files that were missing them. This normalization is consistent with the .editorconfig requirement and does not introduce any regressions. No other functional or structural changes were made.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement human review.

Generated by Everglow Holistic Review for #380 · ⊞ 16K

[github-actions]

Holistic Review

Motivation: The incremental changes are a merge of origin/master into the PR branch, bringing in workflow infrastructure updates (model upgrade, multi-provider support, secret rename) and .editorconfig/.gitattributes line-ending normalization, plus a focused UI commit ("Remove old reward.") that cleans up the mission detail panel.

Approach: The workflow changes are auto-generated gh-aw recompiles and are consistent with the configuration source (holistic-review.md). The editor/git-attributes changes correctly enforce LF for C#/JSON/TXT/FX and CRLF for .sln/.csproj, matching the updated AGENTS.md convention. The mission UI changes are straightforward: the reward panel section is commented out (commit message says "remove"), the detail panel layout is simplified to use more absolute positioning, a decorative board background is added to DrawSelf, the unused DrawMissionPanel stub is removed, and the scrollbar thumb is doubled in size for visibility.

Summary: ⚠️ Needs Human Review. The incremental changes are low-risk and mechanically sound — no new bugs, regressions, or convention violations. However, the prior review's substantive concerns (localization key deletion, null-forgiving event invoke, vacuous test) remain entirely unaddressed. A human reviewer should decide whether to merge with those known issues.

• review 4783923527 — commit 7228f95ce, verdict was ⚠️ Needs Human Review, now ⚠️ Needs Human Review; assessment unchanged (the incremental changes are config normalization and UI refinement; all prior substantive findings remain applicable).

---

Detailed Findings

✅ Incremental Changes Verified

The following changes in the 7228f95ce..2a317e33 range are clean:

• Workflow recompile (.github/workflows/holistic-review.lock.yml): Auto-generated gh-aw output from holistic-review.md. Model changed deepseek-v4-flash → deepseek-v4-pro, secret renamed DEEPSEEK_API_KEY → LLM_API_KEY, multi-provider domain allowlists added, setup action pinned to tag. All consistent with the source config.

• .editorconfig / .gitattributes / AGENTS.md: Line-ending normalization — .sln/.csproj → CRLF, everything else → LF. The .editorconfig rule, .gitattributes explicit settings, and AGENTS.md documentation are all self-consistent.

• MissionContainer.cs: Default panel dimensions reduced (2048×1440 → 1840×1020), resolution *= 0.6f removed from UpdateResolutionFactor, width/height now use raw pixels instead of scaled values. The detail panel uses absolute positioning (Left = 740, Top = 60) while child elements retain * ResolutionFactor scaling — a reasonable layout choice.

• UIMissionDetail.cs: Reward panel section commented out; detail panel width calculation simplified (two-column instead of three-column layout); description/objective panels repositioned; unused DrawMissionPanel(int, int, int, int) stub removed; DrawSelf now draws a stretched MissionIconBoard slice as a decorative background.

• UIMissionTextVerticalScrollbar.cs: Thumb scale changed from 1f to 2f — doubled size for better visibility.

• MissionMessageBoard.png: Binary update (1940 → 1952 bytes), consistent with the UI refresh.

⚠️ Prior Issues Still Unaddressed

The following findings from the previous review remain applicable and are not in the incremental scope:

• ❌ Localization Key Deletion — Localization/en-US_Mods.Everglow.hjson still deletes AcytaeaScratch_TownNPC.DisplayName.
• ⚠️ Null-Forgiving Invoke — MissionGlobalNPC.cs still calls OnKillNPCEvent.Invoke(npc) without null-conditional (while SpecialOnKill correctly uses ?.).
• ⚠️ Vacuous Test — TimeLimitTest() body remains fully commented out.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement human review.

Generated by Everglow Holistic Review for #380 · ⊞ 13.7K