Repository navigation
Combat: target choice, special damage per enemy, host rolls - #233
Conversation
In an all-at-once combat with more than one living enemy the player selects the target (attack = the first living enemy, attack.<enemy> for the others). The enemy properties deals and takes set the STAMINA a hit costs the player or the enemy; Luck shifts them. With set_host_rolls the host answers the combat dice as roll awaits; CombatState keeps the totals so far, so a save mid-round restores. Closes #190
There was a problem hiding this comment.
Note
Copilot was unable to complete this review at the selected tier due to model policy or plan restrictions.
🟡 Changes recommended
A documentation nit remains in docs/writing-stories.md where the stamina rule is described as always “takes 2 stamina,” which is now only the default due to newly introduced per-enemy damage overrides and should be clarified to avoid misleading readers.
1 open issue
What changed in this PR
This pull request enhances the built-in combat system to support per-enemy/per-combat damage tuning (deals/takes), explicit target selection in “all-at-once” fights while keeping backward-compatible attack behavior, and optional host-driven dice rolling (including save/load continuity while awaiting rolls).
Changes:
- Add optional enemy damage overrides (
deals,takes) with Luck shifting behavior and project validation for non-numeric values. - Add target-choice commands for
together: truecombats with multiple living enemies (attack.<enemy>), preservingattackas the first living enemy. - Add host-roll sequencing for combat rounds and Luck tests, including refactoring roll recording so host totals appear in the dice display.
| File | Summary |
|---|---|
docs/writing-stories.md |
Documents target choice, special damage (deals/takes), and host-roll behavior for combat. |
crates/loremaster-runtime/tests/combat.rs |
Adds tests covering target choice, special damage + Luck shifting, checker rejection, and host rolls (including save/load mid-await). |
crates/loremaster-runtime/src/state.rs |
Extends CombatState with serde-defaulted fields (by, target, rolls) to support new combat behavior while maintaining save compatibility. |
crates/loremaster-runtime/src/game/combat.rs |
Implements target selection, variable damage rules, host-roll awaits, and Luck interactions in core combat flow. |
crates/loremaster-runtime/src/game/awaits.rs |
Splits out roll recording (record_roll) so host-supplied totals are logged/displayed like session rolls. |
crates/loremaster-runtime/src/explore.rs |
Updates explorer behavior to handle the new “Attack ” options in multi-enemy combats. |
crates/loremaster-project/src/check.rs |
Validates that optional deals/takes enemy properties are numeric. |
CHANGELOG.md |
Records the new combat capabilities and the (library) breaking change to CombatState struct shape. |
Review effort: Balanced
| `win`, `loss` and `escape` are rooms. `escape` is optional. With `together: true`, all enemies fight in the same round; without it, they fight one after the other. Effects after `combat` run in the room where the combat ends. | ||
|
|
||
| In each round, the player selects "Attack" or, if the combat has `escape`, "Escape". The enemy and the player each roll 2d6 and add their `skill`. The enemy rolls first. The higher total hits and takes 2 `stamina`. In an all-at-once combat, each enemy rolls and the player rolls once. The player attacks the first living enemy, and the other enemies only attack. | ||
| In each round, the player selects "Attack" or, if the combat has `escape`, "Escape". The enemy and the player each roll 2d6 and add their `skill`. The enemy rolls first. The higher total hits and takes 2 `stamina`. In an all-at-once combat, each enemy rolls and the player rolls once. While more than one enemy lives, the player selects the target: "Attack GATE WARDEN" (`attack`, the first living enemy) or "Attack HOUND" (`attack.hound`). The other enemies only attack. |
There was a problem hiding this comment.
Fixed in 84eaa70. The paragraph now says a hit takes 2 stamina unless the enemy has deals or takes, which the next paragraph explains.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| } | ||
| // Special rules of the enemy: optional, but numbers. | ||
| for prop in ["deals", "takes"] { |
There was a problem hiding this comment.
WARNING: deals/takes are only type-checked as Int, not bounded below. The runtime clamps them with .max(0) (combat.rs:123), so takes: -1 (or a rule setting it negative) silently makes the enemy undamageable, and deals: -1 makes the player invincible — a combat can become unwinnable with no diagnostic. Reject negative literals here and consider a diagnostic when a rule can set them negative.
There was a problem hiding this comment.
Fixed in 84eaa70. loremaster check rejects a negative deals/takes literal on the enemy (test the_checks_reject_a_negative_special_rule). A value a rule sets at run time is not tracked by the checker (it has no value ranges for set); the combat counts it as 0, and the docs say so.
| let damage = self.hit_damage(&hit); | ||
| let helps = if hit.as_str() == PLAYER { | ||
| self.int("stamina") > HIT - 1 | ||
| self.int("stamina") > damage - 1 |
There was a problem hiding this comment.
SUGGESTION: With deals: 0 a wound deals 0, but damage - 1 is -1, so stamina > -1 holds and 'Test your Luck' is offered although it cannot help: a lucky result is clamped to 0 (same as no test) and an unlucky one raises it to 1. The 'no test where it can not help' guard should also require damage > 0.
There was a problem hiding this comment.
Fixed in 84eaa70. A wound on the player now needs damage > 0 for the Luck offer (test no_luck_test_for_a_wound_that_costs_nothing). A hit on an enemy with takes: 0 keeps the offer: a lucky result gives 2 there, so the test helps.
| /// The hit that waits for "Test your Luck": an enemy, or `player`. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub pending: Option<Id>, | ||
| /// The enemy that made the pending hit on the player, for its `deals`. |
There was a problem hiding this comment.
SUGGESTION: by is set for every pending hit (combat.rs:338 and combat_strike at combat.rs:362), including a player hit on an enemy, where it holds the target rather than the enemy that wounded the player. Reword to 'the enemy of the pending hit (for deals when the hit is on the player)' so the field is not mistaken for wound-only state.
There was a problem hiding this comment.
Fixed in 84eaa70. The doc now says: the enemy of the pending hit, the one hit or the one that hit the player (then its deals sets the damage).
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit db6827e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit db6827e)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 files)
Reviewed by deepseek-v4.1-flash-1017 · Input: 53.4K · Output: 33.1K · Cached: 835.1K |
# Conflicts: # CHANGELOG.md # docs/writing-stories.md
- check rejects a negative deals/takes literal. - No Luck test for a wound that costs nothing. - Docs: 2 is the default damage; deals/takes are 0 or more. - The doc of CombatState::by names both uses.
| ); | ||
| } | ||
| let value = (self.story().thing(e.as_str())).and_then(|t| t.props.get(prop)); | ||
| if let Some(loremaster_model::Value::Int(n @ ..0)) = value { |
There was a problem hiding this comment.
CRITICAL: n @ ..0 is a range pattern in binding position (bindings_with_range, rust-lang/rust#67258), which is a hard error in edition 2021 and later. This workspace is on edition 2024 (Cargo.toml:21), so loremaster-project does not compile here: error: @ bindings cannot be used with range patterns. The whole workspace build and the new the_checks_reject_a_negative_special_rule test fail.
Bind the integer and test it instead, for example if let Some(loremaster_model::Value::Int(n)) = value.copied() && n < 0 { (or keep the borrow and compare *n < 0).
There was a problem hiding this comment.
This compiles: CI on the merged head built and tested it (edition 2024, current stable). Half-open range patterns in binding position (n @ ..0) are stable since Rust 1.80; #67258 was about ..=-less inclusive forms.
Closes #190
What
All three parts, in the built-in combat (
crates/loremaster-runtime/src/game/combat.rs):deals(STAMINA a hit of the enemy costs the player) andtakes(STAMINA a hit of the player costs the enemy) replace the fixed 2. A Luck test shifts them as it shifted the 2. A rule of one combat ("the troll takes 1 extra damage from fire") is a rule that setstroll.takesbefore thecombateffect; writing-stories.md shows it and a test plays it.loremaster checkrejects a non-numberdeals/takes. Enemies were already things with number propsskill/stamina, so this stays data, works in the web player andloremaster test.together: trueand more than one living enemy, the round options are "Attack GATE WARDEN" (attack, the first living enemy, as before) and "Attack HOUND" (attack.hound). Keepingattackfor the first target keeps existing walkthroughs (do: attack) and old saves working. The web player shows the options through the existing await prompt; the explorer tries each target.set_host_rolls(true), a round waits for onerollawait (2d6) per fighting enemy, inenemiesorder, then the player; "Test your Luck" waits for one. Forced rolls (walkthroughs, explorer) still go first, as forawait: { roll }. The webAwaitPromptalready handles roll awaits.CombatStatehas new fieldsby,target,rolls(breaking for code that builds it with a struct literal); all serde-default, so saves stay compatible without a version bump (ADR 0001: a defaulted field needs no step).Game::record_rollis split out oftake_rollso host totals show on the dice display like session rolls.Salt Tower unchanged: its warden has no special rule, and changing it would rebalance its walkthroughs for no gain.
Left open (new issues)
AwaitRequest::Rollhas no label; adding one is a breaking enum change).How verified
New tests in
crates/loremaster-runtime/tests/combat.rs(each fails on main: unknown option / fixed damage / no roll await):the_player_chooses_the_target_of_an_all_at_once_combatan_enemy_can_deal_and_take_other_damage,luck_shifts_the_special_damage,a_rule_before_the_combat_sets_a_special_rule_for_itthe_checks_reject_a_special_rule_that_is_no_numberthe_host_can_roll_the_dice_of_a_combat(including a save and load while the round waits for its second roll, and the Luck roll)cargo fmt --all -- --check,cargo clippy --workspace --all-targets --no-default-features -- -D warnings,cargo test --workspace --no-default-featuresall pass. No web files changed.