feat: add milestone goals for early progression#170
Open
itsmiso-ai wants to merge 1 commit into
Open
Conversation
- Define fixed early-game milestone catalog (build hut, stockpile food, build workshop, build garden, support third worker) - Add MilestoneManager with pure-data evaluation logic for all milestone types - Add comprehensive unit tests covering progression, save compatibility, and milestone description display
There was a problem hiding this comment.
AI Automated Review
Analysis engine: MiniMax@https://litellm.jory.dev/v1 (anthropic)
Recommendation: Approve
This PR delivers a focused, well-tested data layer for the early-game milestone goal system described in #132. The implementation cleanly separates evaluation logic from persistence and UI concerns, matches existing repo patterns (e.g. scripts/rotating_goal.gd, scripts/layout_math.gd), and ships with thorough tests. One acceptance-criteria item (UI rendering) is intentionally deferred to a follow-up; the manager exposes everything needed to wire that up later.
Change-by-Change Findings
scripts/milestone_manager.gd (new, +136)
class_name MilestoneManager— matches the static-utility class style used elsewhere (rotating_goal.gd,layout_math.gd).MILESTONE_CATALOG— fixed 5-entry chain exactly matching the example sequence in #132 (build hut → stockpile food → build workshop → build garden → support third worker). Order, IDs, names, types, and targets are deterministic.- Minor deviation: the issue example says "Stockpile 5 food" but the PR uses
amount: 10. The issue labels this as an Example chain, so this is a reasonable tuning choice, but worth flagging in the PR description for visibility.
- Minor deviation: the issue example says "Stockpile 5 food" but the PR uses
make_goal_state()— returns a JSON-serializable{milestone_id, completed_ids}dictionary; tests confirm the array is fresh (not a shared reference) and round-trips throughJSON.stringify/JSON.parse_string.get_current_milestone(catalog, id)— deep-duplicates the entry so callers can mutate safely; returns{}for unknown IDs (defensive).evaluate_milestone(milestone, game_state)— pure-data, reads onlybuilds,harvested, andworkerskeys (all existing game state). Returns{progress, total}for UI display.BUILD: matches onkind+complete.STOCKPILE: clampsmini(current, amount)— over-target does not regress.WORKER: counts workers withbreak_ticks <= 0(active). Thebreak_ticksfield exists in existing worker records pertest_e2e.gdsave-compatibility flow.
is_milestone_complete()— simple wrapper, well-defined boundary.advance_to_next(completed_ids, current_id)— uses catalog index; thecompleted_idsparameter is unused, which is harmless but slightly noisy. Returns the current id at chain end or for unknown ids (defensive — matches test expectations).milestone_description()— convenient accessor for the event log / UI.
tests/test_milestone_goals.gd (new, +446)
extends SceneTree— matches the pattern used bytest_rotating_goal.gdand other headless tests, so it will plug into the existingtest_runner.gdflow.- 10 test functions cover: catalog structure/ordering, goal-state creation, current-milestone lookup, build/stockpile/worker evaluation (including clamping and
break_ticksexclusion), completion boundary, chain advancement, description rendering, and a 7-case save/load round-trip including integration with asave_version: 2save schema. - Uses
_assert/_assert_eq/_assert_str_eqhelpers consistent withtest_rotating_goal.gd. Exits with code 1 on failure so CI fails loudly.
Standards Compliance (AGENTS.md)
- Desktop companion feel / low-attention: milestone system is read-only with respect to game state and surfaces a single current goal — no urgent clicks, no modal replacement. Aligns with "the player plans and places; workers execute."
- Bottom-dock-first, no sidebar menus: no UI in this PR; the data layer is dock-agnostic.
- Conventions: pure-data class with
static funcmirrorsrotating_goal.gd/layout_math.gd. Tab-indented GDScript, snake_case identifiers,MILESTONE_*constants for enum-like values. - Validation: the new test file follows the headless SceneTree pattern (
./.tools/Godot_v4.2.2-stable_linux.x86_64 --headless --path . --script res://tests/test_runner.gd) so it should slot into CI alongside the existing matrix.
Linked Issue Fit (#132)
- ✅ "Define a small fixed list of early milestones" — exactly 5 entries, hard-coded, deterministic order.
- ✅ "Milestones use existing game state only" —
builds,harvested,workersonly; no new state surface beyond the per-savemilestone_id/completed_ids. - ✅ "Completed milestones persist in save data" —
make_goal_state()is a plain Dictionary;test_save_load_compatibilityround-trips it throughJSON.stringify/JSON.parse_stringand embeds it inside a fullsave_version: 2save. ⚠️ "UI shows the current milestone without replacing the ambient desktop-companion feel" — not addressed in this PR. The manager exposesget_current_milestoneandevaluate_milestoneso UI can read everything it needs, but no rendering/wiring inmain.gdis included. This is a legitimate follow-up (git history shows a prior attempt at integrating MilestoneManager intomain.gdwas reverted in7c2497cas "unrelated", suggesting scope discipline is intentional here). Worth calling out in the merge message so the UI piece doesn't get forgotten.- ✅ "Tests cover completed/current milestone behavior and save/load compatibility" — comprehensive coverage in 10 functions + 7 sub-cases.
- ✅ "Avoid a full questline or story system" — no narrative, no branching, no rewards gating; just a fixed progression chain.
Evidence Provider Findings
- No evidence providers configured; nothing to report.
Tool Harness Findings
- Planning warning: Could not parse planning response as JSON — benign; no planned requests were made. No follow-up needed.
Unknowns / Needs Verification
- The PR doesn't touch
main.gd, so the current milestone is not yet displayed anywhere in-game. Verify the follow-up UI PR is on the roadmap before closing #132. advance_to_nextignores itscompleted_idsargument. If a future contributor needs to validate againstcompleted_ids(e.g. for reordering or branching), they'll need to extend it. Not a blocker.- The
class_name MilestoneManagerintroduces a global identifier; confirm no name clash with any future class in the main scene.
Sources
- PR #170 metadata, diff, and files (this corpus)
- Issue #132 body and acceptance criteria
scripts/rotating_goal.gdandtests/test_rotating_goal.gd(existing comparable pattern, repo history)docs/DESIGN.mdsave schema (save_version 2, key inventory)tests/test_e2e.gdsave-compatibility flow (worker field shape)AGENTS.md(project pillars, validation, contribution norms)CONTRIBUTING.md(test placement convention, persistence test requirement)
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
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.
Fixes #132