Task.work() takes effort as a float, with the guard on the base class #115

Merged

liamjd merged 2 commits from 110-work-effort-float into main

2026-09-18 18:34:53 +01:00

liamjd commented

2026-09-18 18:30:40 +01:00

Owner

Closes #110.

Task.work() takes effort: float instead of ticks: int, and the guard that comes with it moves onto the base class.

Why the int had to go

work_remaining was already a float and Pawn already stubs PawnLevel {NEWBIE, SKILLED, EXPERT, LEADER}, so the parameter was the odd one out. The failure mode is silent: GDScript converts a float argument to an int parameter without complaint, so work(ctx, 1.5) became one unit of effort and work(ctx, 0.5) became none at all. Every existing call passed a whole number, which is exactly why nothing had noticed — 1.0 converts cleanly.

The unit does not change. Effort is still pawn-ticks, per #88. What changes is that a tick of a pawn's time is no longer required to be worth exactly one of them, which is what #69 and #74 need.

`work()` becomes a template method

The signature change opened a second hole. The usual

work_remaining = maxf(0.0, work_remaining - effort)

clamps the result, which does nothing for a negative input — subtracting a negative effort grows the remainder. A building under construction would quietly un-build itself rather than erroring.

So work() is now validate, then delegate — the same split can_start() already uses for _can_start() in the same file — with subclasses implementing _work(). No caller changed; they were already calling work().

That placement matters rather than being tidiness. The guard was briefly written once in ConstructionTask and once in StubTask, with a third copy owed by each of #72's DeconstructionTask and #73's HaulTask. One rule in four places, where forgetting it is silent, is the shape of a bug that arrives in six months.

Zero effort is now a no-op rather than an error. ConstructionTask had guarded effort <= 0.0, which made a pawn that contributed nothing this tick a caller error. Blocked, or between waypoints, is not a bug — #75's loop will produce that case routinely.

A note on the test double

Worth recording, because it nearly shipped a false pass.

The first version of the negative-effort test ran against StubTask. The stub then grew its own guard to satisfy it, and the test went green while ConstructionTask was still unprotected — it could not distinguish "the base class caught it" from "the subclass caught it early".

StubTask is now deliberately unguarded and carries a comment saying so. It is test support, not game behaviour; a guard there protects no task the game actually runs. The pair of tests that replaced it does the real work: one pins the mechanism (a rejected effort never reaches the subclass hook), one pins that a concrete subclass inherits it (a negative effort cannot un-build a real building).

Verification

522 passing, 0 failing, 1 pending across 523 tests. The pending is the pre-existing #85 marker in test_task_tree.gd. Editor scan run; project imports clean.

Eight tests added or renamed:

test_task.gd — fractional effort lands exactly; fractional efforts accumulate to zero; zero effort leaves the remainder alone; negative effort does not add work back; the guard belongs to the base class, not the subclass; a valid effort still reaches the subclass hook
test_construction_task.gd — a fractional effort lands exactly on the building; a fractional first effort still breaks ground; a negative effort cannot un-build a building

The four red tests this branch opened with were all the truncation: 1.5 arriving as 1, 0.5 arriving as 0, and -5.0 growing the remainder from 3.0 to 8.0.

Not done here

BuildingType.build_work_ticks() still returns an int, deliberately. Build effort is authored in whole pawn-days; it is the per-tick contribution that varies.
Nothing computes a skill multiplier yet. work() can now carry one, but #74 puts roles on the pawn record and #69 weights selection by them - until then every caller passes 1.0.
Nothing drives work() at all: #75's pawn work loop is still the consumer.

🤖 Generated with Claude Code

Closes #110. `Task.work()` takes `effort: float` instead of `ticks: int`, and the guard that comes with it moves onto the base class. ## Why the int had to go `work_remaining` was already a `float` and `Pawn` already stubs `PawnLevel {NEWBIE, SKILLED, EXPERT, LEADER}`, so the parameter was the odd one out. The failure mode is silent: GDScript converts a float argument to an `int` parameter without complaint, so `work(ctx, 1.5)` became one unit of effort and `work(ctx, 0.5)` became **none at all**. Every existing call passed a whole number, which is exactly why nothing had noticed — `1.0` converts cleanly. The unit does not change. Effort is still pawn-ticks, per #88. What changes is that a tick of a pawn's time is no longer required to be worth exactly one of them, which is what #69 and #74 need. ## `work()` becomes a template method The signature change opened a second hole. The usual ```gdscript work_remaining = maxf(0.0, work_remaining - effort) ``` clamps the **result**, which does nothing for a negative input — subtracting a negative effort *grows* the remainder. A building under construction would quietly un-build itself rather than erroring. So `work()` is now `validate, then delegate` — the same split `can_start()` already uses for `_can_start()` in the same file — with subclasses implementing `_work()`. No caller changed; they were already calling `work()`. That placement matters rather than being tidiness. The guard was briefly written once in `ConstructionTask` and once in `StubTask`, with a third copy owed by each of #72's `DeconstructionTask` and #73's `HaulTask`. One rule in four places, where forgetting it is silent, is the shape of a bug that arrives in six months. **Zero effort is now a no-op rather than an error.** `ConstructionTask` had guarded `effort <= 0.0`, which made a pawn that contributed nothing this tick a caller error. Blocked, or between waypoints, is not a bug — #75's loop will produce that case routinely. ## A note on the test double Worth recording, because it nearly shipped a false pass. The first version of the negative-effort test ran against `StubTask`. The stub then grew its own guard to satisfy it, and the test went green **while `ConstructionTask` was still unprotected** — it could not distinguish "the base class caught it" from "the subclass caught it early". `StubTask` is now deliberately unguarded and carries a comment saying so. It is test support, not game behaviour; a guard there protects no task the game actually runs. The pair of tests that replaced it does the real work: one pins the mechanism (a rejected effort never reaches the subclass hook), one pins that a concrete subclass inherits it (a negative effort cannot un-build a real building). ## Verification `522 passing, 0 failing, 1 pending` across 523 tests. The pending is the pre-existing #85 marker in `test_task_tree.gd`. Editor scan run; project imports clean. Eight tests added or renamed: - `test_task.gd` — fractional effort lands exactly; fractional efforts accumulate to zero; zero effort leaves the remainder alone; negative effort does not add work back; the guard belongs to the base class, not the subclass; a valid effort still reaches the subclass hook - `test_construction_task.gd` — a fractional effort lands exactly on the building; a fractional first effort still breaks ground; a negative effort cannot un-build a building The four red tests this branch opened with were all the truncation: `1.5` arriving as `1`, `0.5` arriving as `0`, and `-5.0` growing the remainder from `3.0` to `8.0`. ## Not done here - `BuildingType.build_work_ticks()` still returns an `int`, deliberately. Build effort is *authored* in whole pawn-days; it is the per-tick contribution that varies. - Nothing computes a skill multiplier yet. `work()` can now carry one, but #74 puts roles on the pawn record and #69 weights selection by them - until then every caller passes `1.0`. - Nothing drives `work()` at all: #75's pawn work loop is still the consumer. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

liamjd added 2 commits

2026-09-18 18:30:41 +01:00

Spec effort as a float, not ticks as an int (#110 ) f3c3f3b56b

Tests only; the implementation is #110's to write. Four red, all of them the
silent truncation the issue exists to remove.

GDScript converts a float argument to an int parameter without complaint, so
work(ctx, 1.5) becomes one unit of effort and work(ctx, 0.5) becomes none at
all. Every existing call passes a whole number, which is why nothing has
noticed: 1.0 converts cleanly. A SKILLED builder worth 1.5 per tick does not.

The fourth failure is a separate hole the same change opens up. Today's
`maxf(0.0, work_remaining - ticks)` GROWS the remainder for a negative input,
so the guard has to move to the input rather than the result. Specced as
ignored-and-reported, deliberately unlike GameClock.advance()'s silent refusal
of a negative delta: a negative frame delta is an edge of the engine's loop,
while a negative effort can only come from a caller's own arithmetic.

Note the unit does not change - effort is still pawn-ticks, per #88. What
changes is that a tick of a pawn's time is no longer required to be worth
exactly one of them, which is what Pawn.PawnLevel needs (#69, #74).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Move the effort guard onto Task.work(), delegating to an abstract _work() f8a8e43b67

work() becomes a template method - validate, then delegate - exactly as
can_start() already wraps _can_start() in the same file. ConstructionTask and
StubTask rename their hook to _work() and drop their own guards. No caller
changes: they were already calling work().

The guard was previously written once in ConstructionTask and once in
StubTask, which is two copies of one rule and a third owed by each of #72's
DeconstructionTask and #73's HaulTask. Forgetting it is silent: subtracting a
negative effort GROWS work_remaining, and the usual maxf(0.0, ...) clamps the
result rather than the input, so a building under construction would un-build
itself rather than erroring.

Zero effort now reaches the subclass as a no-op rather than being rejected -
ConstructionTask guarded `effort <= 0.0`, which made a pawn that contributed
nothing this tick a caller error. Blocked, or between waypoints, is not a bug.

StubTask stays deliberately unguarded, and says so. It is test support, not
game behaviour; a guard there protects nothing the game runs and would have
made the placement test in test_task.gd pass whatever production code did -
which is exactly what it did before this change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

liamjd merged commit 629cd1f6cb into main

2026-09-18 18:34:53 +01:00

liamjd referenced this pull request from a commit

2026-09-18 18:34:53 +01:00

Merge pull request 'Task.work() takes effort as a float, with the guard on the base class' (#115) from 110-work-effort-float into main