From f9dd7f9ad9552612b48bcc0b6e0f3039380debb0 Mon Sep 17 00:00:00 2001 From: prode Date: Sun, 20 Sep 2026 17:28:29 -0300 Subject: [PATCH 1/2] feat(characters): one animation over many directions, joined on this side PixelLab starts a new animation group for every call to POST /v2/characters/animations: animation_group_id is extra_forbidden on the way in, and repeating animation_name or template_animation_id makes a second group rather than extending the first. So the directions of one motion cannot be joined on the provider, and this joins them here. - character animate refuses a direction the motion already holds, before anything is sent, naming what it holds and what it lacks; --again buys a second take. Animating a direction it lacks says where the frames join. - character show folds the groups of one motion into one animation and says how many PixelLab holds behind it. - The subject record gathers the animation runs of one motion into one animation over every direction they covered and every file they wrote, so inspect and the atlas built from it carry all of them. Names and direction labels coming back from the provider now reach a refusal and the terminal, so they go through the same control-character table the record already applies to a manifest. specs/characters-and-animation R2.39, R2.40, R4.5; specs/asset-workspace R7.5. Verified with scc validate --checks --pr: 1691 passed, 46 skipped, coverage 95.34%, ruff check and format clean, artifacts clean. --- .../skills/pixellab-cli-animation/SKILL.md | 22 +- docs/glossary.md | 1 + docs/notes.md | 3 + docs/wiki/pages/animation-frames.md | 31 ++ specs/asset-workspace/design.md | 10 + specs/asset-workspace/requirements.md | 1 + specs/asset-workspace/tasks.md | 5 + specs/characters-and-animation/design.md | 47 +++ .../characters-and-animation/requirements.md | 8 +- specs/characters-and-animation/tasks.md | 15 + src/pixellab_cli/commands/character.py | 156 +++++++-- src/pixellab_cli/commands/inspect_command.py | 10 +- .../skill/pixellab-cli-animation/SKILL.md | 22 +- src/pixellab_cli/subject.py | 36 +- tests/test_commands_character.py | 313 ++++++++++++++++++ tests/test_subject.py | 66 +++- 16 files changed, 710 insertions(+), 36 deletions(-) diff --git a/.claude/skills/pixellab-cli-animation/SKILL.md b/.claude/skills/pixellab-cli-animation/SKILL.md index f586997..50672cf 100644 --- a/.claude/skills/pixellab-cli-animation/SKILL.md +++ b/.claude/skills/pixellab-cli-animation/SKILL.md @@ -130,16 +130,30 @@ nothing else to offer and nothing to refuse against. pixellab-cli character animate --action/-a --template --direction/-d --frames --start-pose --end-pose --enhance --drop-first-frame --terse --any-pose - --name --seed + --name --seed --again ``` **Every direction is a separate job and a separate charge**, at one generation per frame per direction. It defaults to south alone. Frames times directions is the bill. -Three things are refused before anything is sent, and each has an escape for the +**Name every direction in one call where you can.** PixelLab starts a new animation for +each call and will not add a direction to one that exists — `animation_group_id` is +refused on the way in, and repeating the name or the template makes a second animation +rather than extending the first. So `-d south -d east -d north` is one animation over +three directions, and three calls are three animations of one motion. + +That split is real on the account and not here: `character show` reads the groups of one +motion as one animation and says how many PixelLab holds, `inspect` gathers the runs the +same way, and an atlas built from that animation is built from every direction of it. So +coming back later to animate the directions you skipped is fine and is what `--again` +guards: a direction the motion already has is refused before anything is sent, because a +second take of one it holds is a copy nobody asked for. `--again` buys it anyway. + +Four things are refused before anything is sent, and each has an escape for the person who means it: a pose belonging to another character, a pose the record says was -made for a different motion (`--any-pose`), and an action that is a label rather than a -motion (`--terse`). None of them fires on something the record has not seen. +made for a different motion (`--any-pose`), an action that is a label rather than a +motion (`--terse`), and a direction this motion already holds (`--again`). None of them +fires on something the record has not seen. `--start-pose` takes a state's id or a file and is the frame the motion starts on. Without it the animation starts from the neutral rotation and has to invent the pose and diff --git a/docs/glossary.md b/docs/glossary.md index 22ba806..4fbe232 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -26,6 +26,7 @@ findings. - **object** — a PixelLab-managed prop with a stable `object_id`, generated in one direction or eight. Avoid: item sprite - **rotation** — one directional view of a character or object. Avoid: facing direction - **animation** — an ordered frame sequence belonging to one character or object and one direction. Avoid: anim, clip +- **animation group** — the directions PixelLab holds together as one animation of a character or object, identified by an `animation_group_id` that `GET /v2/characters/{character_id}` reports. One per call: a motion animated over two calls has two, and only an object's routes accept the id to extend one. Avoid: animation set - **spritesheet** — one uniform-grid image holding a character's rotations and animation frames, plus the layout JSON that says which cell is which. Avoid: sprite atlas - **tileset** — a set of seamlessly connecting terrain tiles generated in one PixelLab call. Avoid: terrain set - **concept image** — a non-pixel-art image generated on fal, used as a reference for a PixelLab route or delivered as box art. Avoid: concept art diff --git a/docs/notes.md b/docs/notes.md index 81de3d5..f7d198d 100644 --- a/docs/notes.md +++ b/docs/notes.md @@ -90,3 +90,6 @@ over this file answers with the example above as well as with the notes. --> - n-0050 2026-09-20 #gotcha @docs/wiki/pages/character-consistency.md @src/pixellab_cli/skill/pixellab-cli-animation/SKILL.md — a state animated as itself carries one character already in the pose; naming the state with custom_start_frame instead makes the call hold a neutral character and another's frame, for the same price - n-0051 2026-09-20 #gotcha @src/pixellab_cli/commands/character.py — the enrichment action must not name a direction: -d picks which rotation to read the frame from, and the text it returns is the -a of every direction's animation call, so a south in the wording describes the north animation as walking south - n-0052 2026-09-20 #gotcha @src/pixellab_cli/harness.py — pointing Claude Code at a skill's SKILL.md gets a file read rather than the skill loaded, so the managed block names the skill and carries no path +- n-0053 2026-09-20 #gotcha @src/pixellab_cli/commands/character.py — characters/animations names a v3 animation custom- plus the action's first thirty characters, truncated mid-word, which is how a motion is matched to one the character already has +- n-0054 2026-09-20 #gotcha @src/pixellab_cli/commands/character.py — animation_group_id is extra_forbidden on characters/animations and only the site's unprefixed animate-with-text-v3/character/background takes it, which refuses the API key +- n-0055 2026-09-20 #ceiling @src/pixellab_cli/subject.py — a motion is matched to one the character already has by the name the call would carry, so a bare action on one call and --name on the next are two motions and neither the refusal nor the join reporting fires diff --git a/docs/wiki/pages/animation-frames.md b/docs/wiki/pages/animation-frames.md index 244554b..f20dde3 100644 --- a/docs/wiki/pages/animation-frames.md +++ b/docs/wiki/pages/animation-frames.md @@ -72,6 +72,37 @@ alone, which is also the only thing that makes an eight-frame test call cheap. eight frames that is two and a half to five times the price for one direction, which is why v3 is both PixelLab's recommendation and this tool's default. +## One animation, or several + +The directions of one motion are an **animation group**, and PixelLab makes a new one +for every call. `animation_group_id` is returned by `GET /v2/characters/{character_id}` +and accepted on the way in only by the object routes — `AnimateObjectRequest` documents +it as *"pass the animation_group_id of an existing animation on this object to add more +directions to it"*. `POST /v2/characters/animations` refuses it outright: + +```json +{"type": "extra_forbidden", "loc": ["body", "animation_group_id"], + "msg": "Extra inputs are not permitted"} +``` + +Nor is there an implicit join to fall back on. Repeating `animation_name`, or the same +`template_animation_id`, makes a second group rather than extending the first — an +account checked for this held `walk`, `jump` and `run` twice over on one character, and +a `spell` over six directions beside a `spell 2` holding the two added later. + +So one group over eight directions comes from **one call naming eight directions**, and +a posed animation can never be one at all: `custom_start_frame` is a single frame per +call and the pose differs per direction ([[pixellab-asset-routing]]). + +`pixellab-cli` closes the split on this side rather than pretending it is closed. +`character animate` refuses a direction the motion already holds before spending +anything, `character show` reads the groups of one motion as one animation and says how +many PixelLab holds, and the subject record gathers the runs the same way, so the atlas +built from that animation carries every direction of it. PixelLab's own web app extends +a group over `POST /animate-with-text-v3/character/background`, unprefixed and taking a +singular `direction`; it answers `403 {"detail": "Invalid token"}` to an API key, which +is why nothing here calls it. + ## Mirroring is not a route There is no mirror or flip operation anywhere in REST v2. `mirror` does not occur in the diff --git a/specs/asset-workspace/design.md b/specs/asset-workspace/design.md index c63e8dc..b5c8fd2 100644 --- a/specs/asset-workspace/design.md +++ b/specs/asset-workspace/design.md @@ -99,3 +99,13 @@ together, a table of prices copied from a pricing page can never be corrected. Both injected. A run id derived from `datetime.now()` makes a test that asserts a directory name unwritable, and this is a feature whose entire behaviour is about what gets written where. + +## An animation made over two calls is one animation + +PixelLab starts a new animation for every call, so a character animated south today and +east tomorrow holds two animations of one motion — the reasoning is in +`specs/characters-and-animation/design.md`. The record does not repeat that split: runs +of one character are gathered by the motion they animate, which is the name the run +carried or the action it was made from, and the gathered animation holds every +direction they covered and every file they wrote. An atlas built from one animation is +then built from every direction of it, which is what `export atlas` is given. diff --git a/specs/asset-workspace/requirements.md b/specs/asset-workspace/requirements.md index 5f2c976..3f9a76c 100644 --- a/specs/asset-workspace/requirements.md +++ b/specs/asset-workspace/requirements.md @@ -68,6 +68,7 @@ A file nobody can account for is worth less than the same file with a manifest. - **R7.2** The asset workspace shall derive the subject's manifest from the run manifests every time it writes it, so that it cannot disagree with what was paid for. - **R7.3** Where a call is made from an identifier an earlier call produced, the asset workspace shall record that identifier beside the request, because a request carrying a frame does not say which pose the frame came from. - **R7.4** When asked to describe a subject, the asset workspace shall report it without calling a provider, and shall report the subjects it holds where none is named. +- **R7.5** (ADDED) The asset workspace shall gather the animation runs of one character made for one motion into a single animation holding every direction they cover and every file they wrote. ## Out of scope diff --git a/specs/asset-workspace/tasks.md b/specs/asset-workspace/tasks.md index 8edfb43..7ff5b84 100644 --- a/specs/asset-workspace/tasks.md +++ b/specs/asset-workspace/tasks.md @@ -65,3 +65,8 @@ - [x] 6.5 (Unit) Give each run of a kind its own version directory, numbering from the first free one and leaving loose files alone — R1.5, R1.10, R2.1 _Depends 6.1_ _Reason a second attempt at the same asset became a file called -2, which is a name rather than a history_ + +## 7 · One animation over many directions +- [x] 7.1 (Unit) Gather one character's animation runs for a motion into one — R7.5 + _Reason the provider starts a new animation per call_ + diff --git a/specs/characters-and-animation/design.md b/specs/characters-and-animation/design.md index 51a3a79..6694e84 100644 --- a/specs/characters-and-animation/design.md +++ b/specs/characters-and-animation/design.md @@ -328,3 +328,50 @@ The matcher underneath is word overlap between the state's `--edit` text and the stores a character, not an idle. That is a real ceiling: a state described in words the motion does not reuse reads as unrelated. It fails towards the refusal rather than away from it, and `--any-pose` is the way past. + +## One animation, many directions — and why the provider will not extend one + +A character animation is one group with a direction each, and PixelLab returns its +`animation_group_id` from `GET /characters/{character_id}`. Extending that group is +what `AnimateObjectRequest` documents for objects — *"pass the animation_group_id of an +existing animation on this object to add more directions to it"* — and what +`CreateCharacterAnimationRequest` does not offer. Sent anyway, the route answers: + +```json +{"type": "extra_forbidden", "loc": ["body", "animation_group_id"], + "msg": "Extra inputs are not permitted"} +``` + +The body is `extra="forbid"`, confirmed by sending an invented field alongside it and +reading the same error back. Nor does the provider join by name or by template: the +account this was checked against holds `walk`, `jump` and `run` twice each on one +character as separate groups, and a `spell` over six directions beside a `spell 2` over +the remaining two. **Every POST to `/characters/animations` starts a new group.** One +group over eight directions comes from one call naming eight directions, which +`character animate -d south -d east …` already sends. + +PixelLab's own site does extend a group, over an endpoint that is not in the v2 schema +and not reachable with an API key: +`POST https://api.pixellab.ai/animate-with-text-v3/character/background`, unprefixed, +taking `direction` in the singular beside `animation_group_id`. It answers +`403 {"detail":"Invalid token"}` to the bearer token this tool holds, and +`https://api.pixellab.ai/openapi.json` is `404`. Reaching for it would mean a second +credential, taken out of a browser session, for an endpoint nobody documented and +nothing obliges PixelLab to keep. Not adopted, and this paragraph is why. + +So the seam moves to this side of the wire. R2.39 spends nothing to prevent the waste: +the character payload `_require_character` already fetches names every animation and +every direction it covers, so a direction asked for twice is a refusal that names the +animation holding it, with `--again` for the case where a second take is the point. +Matching a motion to a group is by the name the call would carry — `--name` when given, +otherwise `custom-` and the first thirty characters of the action, which is how the +provider derives `animation_type` for a v3 animation, or the template id for a +template. + +R4.5, and the record requirement added to `specs/asset-workspace/` beside it, then read +several groups as one animation: +`character show` folds the account's groups by that name and says how many the provider +holds behind it, and the subject record folds the runs the same way, so an animation +built over two calls reaches `inspect` — and the atlas built from it — as one animation +over every direction it covers. The grouping the provider refuses is real everywhere a +game engine can see it. diff --git a/specs/characters-and-animation/requirements.md b/specs/characters-and-animation/requirements.md index 4acfb93..67b3b8b 100644 --- a/specs/characters-and-animation/requirements.md +++ b/specs/characters-and-animation/requirements.md @@ -1,8 +1,8 @@ --- autonomy: auto ci: wait -branch: feat/room-for-the-motion -delivery: merged +branch: feat/one-animation-many-directions +delivery: in-progress pr: 68 --- @@ -82,6 +82,9 @@ different endpoint family and no skeleton. - **R2.37** (ADDED) If no start pose is given and the subject's record holds no pose of that character made for the motion being animated, then the character commands shall refuse the call and say that the state the motion starts from is made first, before spending anything, unless the caller asks to animate from the character's rest frame. - **R2.38** (ADDED) The character commands shall offer a check that reports whether a motion and a character's poses go together, and shall complete it without calling a provider. +- **R2.39** (ADDED) If a direction asked for is already animated on that character for the motion being animated, then the character commands shall refuse the call, name the animation and the directions it already holds, and say that the provider starts a new animation rather than adding a direction to one, before spending anything, unless the caller asks for it to be generated again. +- **R2.40** (ADDED) Where a direction is animated for a motion the character already has, the character commands shall report that the frames join that motion in the workspace and not in the animation the provider holds. + ## R3 · Rotating @@ -93,6 +96,7 @@ different endpoint family and no skeleton. - **R4.2** The character commands shall show one character's rotations, animations and settings. - **R4.3** When asked for a spritesheet, the character commands shall download the sheet and its layout file and write both to the workspace. - **R4.4** Where a command names an asset that does not exist, the character commands shall say so rather than reporting an empty result. +- **R4.5** (ADDED) When showing a character, the character commands shall present the animations it holds for one motion as a single animation over every direction they cover, and shall say how many the provider holds for it. ## Out of scope diff --git a/specs/characters-and-animation/tasks.md b/specs/characters-and-animation/tasks.md index 49312f1..d8bff1e 100644 --- a/specs/characters-and-animation/tasks.md +++ b/specs/characters-and-animation/tasks.md @@ -118,3 +118,18 @@ R2.27 _Depends 6.3_ _Reason the interpolation tool was asked for after delivery_ + +## 7 · One animation over many directions + +- [x] 7.1 (Unit) Read the directions each animation covers from the character — R2.39 + _Reason the provider refuses animation_group_id for characters_ +- [x] 7.2 (Unit) Refuse a direction this motion already has, unless asked again — R2.39, + R2.40 + _Depends 7.1_ + _Reason the provider refuses animation_group_id for characters_ +- [x] 7.3 (Unit) Show the animations of one motion as one, over every direction — R4.5 + _Depends 7.1_ + _Reason the provider refuses animation_group_id for characters_ +- [x] 7.4 (Unit) Teach the skill that one call carries every direction — R2.40 + _Depends 7.2_ + _Reason the provider refuses animation_group_id for characters_ diff --git a/src/pixellab_cli/commands/character.py b/src/pixellab_cli/commands/character.py index 614e609..8fe4e2a 100644 --- a/src/pixellab_cli/commands/character.py +++ b/src/pixellab_cli/commands/character.py @@ -72,16 +72,125 @@ def load_templates() -> dict[str, Any]: return json.loads(TEMPLATES_PATH.read_text(encoding="utf-8")) -def _require_character(app_context, character_id: str) -> None: - """Refuse an animation for a character this account does not have. +def _require_character(app_context, character_id: str) -> dict[str, Any]: + """Refuse an animation for a character this account does not have, and hand it back. Free, and it turns a provider rejection mid-run into a refusal that names the identifier — including under a dry run, where the preview would otherwise describe a call that could never have been made. + + The payload is returned rather than dropped because it already names every + animation the character holds and every direction each covers, which is what + R2.39 refuses on: fetching it twice would be a second call for something + already in hand. """ payload = app_context.pixellab().call("character", character_id=character_id).raw if not payload: raise ValidationError(f"no character {character_id!r} on this account") + return payload + + +# PixelLab derives an animation's `animation_type` from the action it was made from: +# `custom-` and the first thirty characters of the description, which is how a motion +# asked for here is matched against one the character already holds. +MOTION_PREFIX = "custom-" +MOTION_NAME_LIMIT = 30 + + +def motion_asked_for(animation_name: str | None, action: str | None, template: str | None) -> str: + """The name the call about to be made will be known by on the character.""" + if animation_name: + return animation_name + if template: + return template + # Truncation lands mid-word as readily as on a space, and a name ending in a + # space is a name nobody can see the end of: both sides of the comparison are + # trimmed rather than one. + return f"{MOTION_PREFIX}{(action or '')[:MOTION_NAME_LIMIT].rstrip()}" + + +def motion_of(animation: dict[str, Any]) -> str: + """What one animation the provider holds was made for. + + `.get(key, default)` does not reach the default when the key is there holding + None, which is what an animation with no display name carries. + + The name is whatever was last typed into it, on the account or in the web app, + and it now reaches a refusal message and the terminal. Control characters come + out of it the same way the subject's record takes them out of a manifest: a name + carrying an escape sequence would rewrite what the operator sees, on a line whose + whole job is to say what has already been paid for. + """ + return ( + str(animation.get("display_name") or animation.get("animation_type") or "unnamed") + .translate(subjects.CONTROL) + .rstrip() + ) + + +def check_the_directions_are_new( + payload: dict[str, Any], motion: str, wanted: list[str], again: bool +) -> dict[str, Any] | None: + """Refuse a direction this character already has for this motion. + + The whole reason a second call happens at all is to reach the directions the + first one did not cover, so covering one twice is money spent on a copy. What it + cannot do is join them: `animation_group_id` is refused on the way in, so the + directions of one motion arrive as separate animations however they are asked + for — and the refusal says so, because the obvious reading of a second call is + that it extends the first. + """ + held = next((entry for entry in gather_animations(payload) if entry["motion"] == motion), None) + if held is None: + return None + + repeated = [name for name in wanted if name in held["directions"]] + if repeated and not again: + rotations = [name for name, _ in ordered_rotations(payload.get("rotation_urls") or {})] + missing = [name for name in rotations if name not in held["directions"]] + raise ValidationError( + f"{', '.join(repeated)} is already animated for {motion!r}, which holds " + f"{', '.join(held['directions'])}. PixelLab starts a new animation rather " + "than adding a direction to one, so this would pay for a second copy. " + + ( + f"Animate what it lacks — {', '.join(missing)} — or pass --again to " + "generate it a second time." + if missing + else "Pass --again to generate it a second time." + if not rotations + else "It covers every rotation this character has; pass --again to " + "generate it a second time." + ), + context={"motion": motion, "held": held["directions"], "repeated": repeated}, + ) + return held + + +def gather_animations(payload: dict[str, Any]) -> list[dict[str, Any]]: + """One entry per motion, over every direction the character holds for it. + + PixelLab starts a new animation group for every call — `animation_group_id` is + refused on the way in, and animating south today and east tomorrow leaves two + animations of one walk. They are one animation everywhere this tool can say so, + and `groups` is how many the provider holds behind it. + """ + gathered: dict[str, dict[str, Any]] = {} + for animation in payload.get("animations") or []: + if not isinstance(animation, dict): + continue + # `directions` holds an object per direction — the name, how many frames it + # has, and their URLs — not a list of names. + covered = [ + str(entry.get("direction", "?")).translate(subjects.CONTROL) + for entry in animation.get("directions") or [] + if isinstance(entry, dict) + ] + entry = gathered.setdefault( + motion_of(animation), {"motion": motion_of(animation), "directions": [], "groups": 0} + ) + entry["groups"] += 1 + entry["directions"].extend(name for name in covered if name not in entry["directions"]) + return list(gathered.values()) def read_subject(app_context) -> subjects.Subject | None: @@ -770,6 +879,9 @@ def animate( any_pose: bool = typer.Option( False, "--any-pose", help="Start from the pose named, even if another suits the action." ), + again: bool = typer.Option( + False, "--again", help="Animate a direction this motion already has, a second time." + ), seed: int = typer.Option(None, "--seed", help="Repeat a previous generation."), ) -> None: """Animate a character. Every direction is a separate job and a separate charge.""" @@ -788,6 +900,7 @@ def animate( drop_first_frame, terse, any_pose, + again, seed, ) except PixellabCliError as failure: @@ -808,6 +921,7 @@ def _animate( drop_first_frame, terse, any_pose, + again, seed, ) -> None: app_context: AppContext = context.obj @@ -863,7 +977,15 @@ def _animate( context={"directions": wanted}, ) - _require_character(app_context, character_id) + character = _require_character(app_context, character_id) + motion = motion_asked_for(animation_name, action, template) + held = check_the_directions_are_new(character, motion, wanted, again) + if held: + output.stderr( + f"{motion!r} is already animated over {', '.join(held['directions'])}. These " + "frames join it here and in what an engine loads; PixelLab holds them as an " + "animation of their own." + ) subject = read_subject(app_context) check_pose_belongs(subject, character_id, start_pose, "--start-pose") check_pose_belongs(subject, character_id, end_pose, "--end-pose") @@ -937,9 +1059,9 @@ def _animate( # the file names, and a frame count is usually chosen to fit a loop. if not template: generated = int(frames or _frame_default(route)) - held = generated if drop_first_frame else generated + 1 + stored = generated if drop_first_frame else generated + 1 output.stderr( - f"{generated} frame(s) generated per direction, {held} held" + f"{generated} frame(s) generated per direction, {stored} held" + ("" if drop_first_frame else " — the frame it starts on is kept as frame 0") + "." ) @@ -1179,25 +1301,21 @@ def _show(context, character_id) -> None: raise ValidationError(f"no character {character_id!r} on this account") rotations = ordered_rotations(payload.get("rotation_urls") or {}) - animations = payload.get("animations") or [] + # One line per motion rather than per group: a walk generated south first and + # east later is two animations on the account and one animation to work with, and + # the count the provider holds is said rather than hidden. + gathered = gather_animations(payload) + groups = sum(entry["groups"] for entry in gathered) + counted = f"{len(gathered)}" + (f" ({groups} on PixelLab)" if groups != len(gathered) else "") lines = [ f"{payload.get('name', character_id)} ({character_id})", f"status: {payload.get('status', 'unknown')}", f"rotations: {', '.join(name for name, _ in rotations) or 'none'}", - f"animations: {len(animations)}", + f"animations: {counted}", ] - for animation in animations: - # `directions` holds an object per direction — the name, how many frames it - # has, and their URLs — not a list of names. - covered = [ - entry.get("direction", "?") - for entry in animation.get("directions") or [] - if isinstance(entry, dict) - ] - # `.get(key, default)` does not reach the default when the key is there - # holding None, which is what an animation with no display name carries. - name = animation.get("display_name") or animation.get("animation_type") or "unnamed" - lines.append(f" {name} {', '.join(covered) or 'none'}") + for entry in gathered: + held = f" [{entry['groups']} animations]" if entry["groups"] > 1 else "" + lines.append(f" {entry['motion']} {', '.join(entry['directions']) or 'none'}{held}") output.emit(payload, lines, as_json=app_context.as_json) diff --git a/src/pixellab_cli/commands/inspect_command.py b/src/pixellab_cli/commands/inspect_command.py index 22308d2..c864719 100644 --- a/src/pixellab_cli/commands/inspect_command.py +++ b/src/pixellab_cli/commands/inspect_command.py @@ -97,10 +97,12 @@ def _lines(subject: subjects.Subject) -> list[str]: f" animation {animation.get('name') or animation.get('action') or '?'}" f" {directions} {len(animation['files'])} frame(s)" ) - lines.append( - f" {animation['directory']}" - + (f" from pose {animation['start_pose']}" if animation.get("start_pose") else "") - ) + pose = f" from pose {animation['start_pose']}" if animation.get("start_pose") else "" + # One motion animated over several calls has a directory per call: the + # frames are one animation and they are not in one place, and a line + # naming only the first would send the reader to a fraction of them. + for run in animation["runs"]: + lines.append(f" {run['directory']}{pose}") if subject.loose: lines.append("") lines.append(f"{len(subject.loose)} run(s) belonging to no character:") diff --git a/src/pixellab_cli/skill/pixellab-cli-animation/SKILL.md b/src/pixellab_cli/skill/pixellab-cli-animation/SKILL.md index f586997..50672cf 100644 --- a/src/pixellab_cli/skill/pixellab-cli-animation/SKILL.md +++ b/src/pixellab_cli/skill/pixellab-cli-animation/SKILL.md @@ -130,16 +130,30 @@ nothing else to offer and nothing to refuse against. pixellab-cli character animate --action/-a --template --direction/-d --frames --start-pose --end-pose --enhance --drop-first-frame --terse --any-pose - --name --seed + --name --seed --again ``` **Every direction is a separate job and a separate charge**, at one generation per frame per direction. It defaults to south alone. Frames times directions is the bill. -Three things are refused before anything is sent, and each has an escape for the +**Name every direction in one call where you can.** PixelLab starts a new animation for +each call and will not add a direction to one that exists — `animation_group_id` is +refused on the way in, and repeating the name or the template makes a second animation +rather than extending the first. So `-d south -d east -d north` is one animation over +three directions, and three calls are three animations of one motion. + +That split is real on the account and not here: `character show` reads the groups of one +motion as one animation and says how many PixelLab holds, `inspect` gathers the runs the +same way, and an atlas built from that animation is built from every direction of it. So +coming back later to animate the directions you skipped is fine and is what `--again` +guards: a direction the motion already has is refused before anything is sent, because a +second take of one it holds is a copy nobody asked for. `--again` buys it anyway. + +Four things are refused before anything is sent, and each has an escape for the person who means it: a pose belonging to another character, a pose the record says was -made for a different motion (`--any-pose`), and an action that is a label rather than a -motion (`--terse`). None of them fires on something the record has not seen. +made for a different motion (`--any-pose`), an action that is a label rather than a +motion (`--terse`), and a direction this motion already holds (`--again`). None of them +fires on something the record has not seen. `--start-pose` takes a state's id or a file and is the frame the motion starts on. Without it the animation starts from the neutral rotation and has to invent the pose and diff --git a/src/pixellab_cli/subject.py b/src/pixellab_cli/subject.py index e0ee25a..f10caae 100644 --- a/src/pixellab_cli/subject.py +++ b/src/pixellab_cli/subject.py @@ -285,9 +285,43 @@ def _animation(run: Run) -> dict[str, Any]: "run": run.run, "directory": run.directory, "files": run.files, + # Every run that fed this animation, because one motion can be several and + # the directory each wrote to is where its frames are. + "runs": [{"run": run.run, "directory": run.directory, "files": run.files}], } +def _motion(animation: dict[str, Any]) -> str: + """What an animation run was made for, which is how two of them are one. + + The name given to the call when there was one, the action otherwise. Empty for + neither, and an empty key joins nothing: two animations nobody named are two + animations, not one. + """ + return str(animation.get("name") or animation.get("action") or "") + + +def _join(animations: list[dict[str, Any]], animation: dict[str, Any]) -> None: + """Add one animation run to a character's animations, joining its motion. + + PixelLab starts a new animation for every call — see + `specs/characters-and-animation/design.md` — so a walk generated south on Monday + and east on Tuesday arrives here as two runs of one motion. They are one + animation in the record, over every direction they covered and every file they + wrote, which is what an atlas built from it is then built from. + """ + motion = _motion(animation) + held = next((e for e in animations if motion and _motion(e) == motion), None) + if held is None: + animations.append(animation) + return + held["directions"] = list(held["directions"] or []) + [ + name for name in animation["directions"] or [] if name not in (held["directions"] or []) + ] + held["files"] = held["files"] + animation["files"] + held["runs"] = held["runs"] + animation["runs"] + + def build(name: str, workspace: Workspace) -> Subject: """Assemble one subject from the runs under it. @@ -324,7 +358,7 @@ def build(name: str, workspace: Workspace) -> Subject: animation = _animation(run) owner = subject.character(str(animation["of"])) if animation["of"] else None if owner is not None: - owner["animations"].append(animation) + _join(owner["animations"], animation) continue subject.loose.append( { diff --git a/tests/test_commands_character.py b/tests/test_commands_character.py index 2d41dab..2965e38 100644 --- a/tests/test_commands_character.py +++ b/tests/test_commands_character.py @@ -17,7 +17,10 @@ from pixellab_cli.cli import app from pixellab_cli.commands.character import ( frame_for_reference, + gather_animations, known_templates, + motion_asked_for, + motion_of, ordered_rotations, ) from pixellab_cli.config import PIXELLAB_BASE_URL, PIXELLAB_SECRET_VAR @@ -3182,3 +3185,313 @@ def test_it_writes_no_ledger_line(self, tmp_path, monkeypatch): ) assert not (tmp_path / "out" / "ledger.jsonl").is_file() + + +class TestWhatTheCharacterAlreadyHolds: + """R2.39 refuses a direction the character already has for this motion, so the + motion the call would be known by and the motions the character holds have to be + named the same way.""" + + def test_a_name_given_is_the_motion_the_call_carries(self): + assert motion_asked_for("walk", "The character strides forward", None) == "walk" + + def test_without_a_name_an_action_carries_the_name_the_provider_derives(self): + action = "The character strides forward with a steady, even gait and swinging arms" + assert motion_asked_for(None, action, None) == "custom-The character strides forward" + + def test_a_template_carries_its_own_name(self): + assert motion_asked_for(None, None, "walk") == "walk" + + def test_a_display_name_names_the_motion_ahead_of_the_type(self): + assert motion_of({"display_name": "slash", "animation_type": "custom-The character"}) == ( + "slash" + ) + + def test_an_animation_with_no_display_name_is_named_by_its_type(self): + assert motion_of({"display_name": None, "animation_type": "walk"}) == "walk" + + def test_the_directions_of_one_motion_are_gathered_across_its_groups(self): + """Two calls, two groups on the provider, one animation here.""" + payload = { + "animations": [ + { + "animation_type": "custom-The character strides", + "animation_group_id": "group-1", + "directions": [{"direction": "south"}, {"direction": "east"}], + }, + { + "animation_type": "custom-The character strides", + "animation_group_id": "group-2", + "directions": [{"direction": "north"}], + }, + ] + } + + gathered = gather_animations(payload) + + assert gathered == [ + { + "motion": "custom-The character strides", + "directions": ["south", "east", "north"], + "groups": 2, + } + ] + + def test_a_direction_held_twice_is_named_once(self): + payload = { + "animations": [ + {"animation_type": "walk", "directions": [{"direction": "south"}]}, + {"animation_type": "walk", "directions": [{"direction": "south"}]}, + ] + } + + assert gather_animations(payload)[0]["directions"] == ["south"] + + def test_motions_stay_apart(self): + payload = { + "animations": [ + {"animation_type": "walk", "directions": [{"direction": "south"}]}, + {"animation_type": "run", "directions": [{"direction": "north"}]}, + ] + } + + assert [entry["motion"] for entry in gather_animations(payload)] == ["walk", "run"] + + def test_a_character_with_no_animations_holds_nothing(self): + assert gather_animations({"animations": None}) == [] + + +class TestADirectionThisMotionAlreadyHas: + """R2.39: a second call reaches the directions the first did not cover, so one it + did cover is a copy nobody asked for. R2.40: what it does reach joins the motion + here rather than in the animation PixelLab holds.""" + + def character(self, animations): + respx.get(f"{PIXELLAB_BASE_URL}/characters/char-9").respond( + json={ + "id": "char-9", + "rotation_urls": rotation_urls(), + "animations": animations, + } + ) + + def walk(self, directions): + return [ + { + "animation_type": "custom-a full walk cycle, legs altern", + "directions": [{"direction": name} for name in directions], + } + ] + + def animate(self, tmp_path, monkeypatch, *extra): + return invoke( + ["character", "animate", "char-9", "-a", WALK_CYCLE, *extra], + tmp_path, + monkeypatch, + ) + + @respx.mock + def test_it_refuses_and_names_what_the_motion_already_holds(self, tmp_path, monkeypatch): + self.character(self.walk(["south", "east"])) + route = respx.post(f"{PIXELLAB_BASE_URL}/characters/animations") + + result = self.animate(tmp_path, monkeypatch, "-d", "south") + + assert result.exit_code != 0 + assert "already animated" in result.output + assert "south, east" in result.output + assert not route.called + + @respx.mock + def test_the_refusal_names_the_directions_the_motion_lacks(self, tmp_path, monkeypatch): + self.character(self.walk(["south", "east"])) + + result = self.animate(tmp_path, monkeypatch, "-d", "south") + + assert "north" in result.output + assert "--again" in result.output + + @respx.mock + def test_a_direction_the_motion_lacks_is_animated(self, tmp_path, monkeypatch): + """The whole point: the second call covers what the first one did not.""" + self.character(self.walk(["south"])) + route = respx.post(f"{PIXELLAB_BASE_URL}/characters/animations").respond( + json={"background_job_ids": ["job-2"], "status": "processing"} + ) + respx.get(f"{PIXELLAB_BASE_URL}/background-jobs/job-2").respond( + json={"status": "completed", "last_response": {"images": [image_payload()]}} + ) + + result = self.animate(tmp_path, monkeypatch, "-d", "north") + + assert result.exit_code == 0 + assert json.loads(route.calls.last.request.content)["directions"] == ["north"] + + @respx.mock + def test_animating_what_it_lacks_says_where_the_frames_join(self, tmp_path, monkeypatch): + self.character(self.walk(["south"])) + respx.post(f"{PIXELLAB_BASE_URL}/characters/animations").respond( + json={"background_job_ids": ["job-2"], "status": "processing"} + ) + respx.get(f"{PIXELLAB_BASE_URL}/background-jobs/job-2").respond( + json={"status": "completed", "last_response": {"images": [image_payload()]}} + ) + + result = self.animate(tmp_path, monkeypatch, "-d", "north") + + assert "already animated over south" in result.output + assert "animation of their own" in result.output + + @respx.mock + def test_asked_again_it_generates_the_direction_a_second_time(self, tmp_path, monkeypatch): + self.character(self.walk(["south"])) + route = respx.post(f"{PIXELLAB_BASE_URL}/characters/animations").respond( + json={"background_job_ids": ["job-2"], "status": "processing"} + ) + respx.get(f"{PIXELLAB_BASE_URL}/background-jobs/job-2").respond( + json={"status": "completed", "last_response": {"images": [image_payload()]}} + ) + + result = self.animate(tmp_path, monkeypatch, "-d", "south", "--again") + + assert result.exit_code == 0 + assert route.called + + @respx.mock + def test_another_motion_is_not_this_one(self, tmp_path, monkeypatch): + """A character holding a walk over south is free to be attacked facing south.""" + self.character(self.walk(["south"])) + route = respx.post(f"{PIXELLAB_BASE_URL}/characters/animations").respond( + json={"background_job_ids": ["job-2"], "status": "processing"} + ) + respx.get(f"{PIXELLAB_BASE_URL}/background-jobs/job-2").respond( + json={"status": "completed", "last_response": {"images": [image_payload()]}} + ) + + result = invoke( + [ + "character", + "animate", + "char-9", + "-a", + "the character swings a heavy blade downward, shoulders turning with the cut", + "-d", + "south", + ], + tmp_path, + monkeypatch, + ) + + assert result.exit_code == 0 + assert route.called + + @respx.mock + def test_a_name_given_is_what_the_motion_is_matched_by(self, tmp_path, monkeypatch): + self.character( + [ + { + "display_name": "walk", + "directions": [{"direction": "south"}], + } + ] + ) + route = respx.post(f"{PIXELLAB_BASE_URL}/characters/animations") + + result = self.animate(tmp_path, monkeypatch, "-d", "south", "--name", "walk") + + assert result.exit_code != 0 + assert not route.called + + @respx.mock + def test_a_dry_run_is_refused_too(self, tmp_path, monkeypatch): + """A preview of a call that would be refused is a preview of nothing.""" + self.character(self.walk(["south"])) + + result = invoke( + ["--dry-run", "character", "animate", "char-9", "-a", WALK_CYCLE, "-d", "south"], + tmp_path, + monkeypatch, + ) + + assert result.exit_code != 0 + assert "already animated" in result.output + + +class TestShowingOneMotionHeldTwice: + """R4.5: PixelLab starts a new animation per call, so a motion generated over two + calls is two animations there and one motion here.""" + + def character(self, animations): + respx.get(f"{PIXELLAB_BASE_URL}/characters/char-9").respond( + json={ + "id": "char-9", + "name": "a knight", + "status": "completed", + "rotation_urls": rotation_urls(), + "animations": animations, + } + ) + + @respx.mock + def test_the_directions_of_one_motion_are_shown_on_one_line(self, tmp_path, monkeypatch): + self.character( + [ + {"animation_type": "walk", "directions": [{"direction": "south"}]}, + {"animation_type": "walk", "directions": [{"direction": "north"}]}, + ] + ) + + result = invoke(["character", "show", "char-9"], tmp_path, monkeypatch) + + assert result.exit_code == 0 + assert "walk south, north" in result.output + + @respx.mock + def test_it_says_how_many_the_provider_holds(self, tmp_path, monkeypatch): + self.character( + [ + {"animation_type": "walk", "directions": [{"direction": "south"}]}, + {"animation_type": "walk", "directions": [{"direction": "north"}]}, + ] + ) + + result = invoke(["character", "show", "char-9"], tmp_path, monkeypatch) + + assert "animations: 1 (2 on PixelLab)" in result.output + assert "[2 animations]" in result.output + + @respx.mock + def test_one_call_per_motion_is_counted_plainly(self, tmp_path, monkeypatch): + self.character( + [ + {"animation_type": "walk", "directions": [{"direction": "south"}]}, + {"animation_type": "run", "directions": [{"direction": "south"}]}, + ] + ) + + result = invoke(["character", "show", "char-9"], tmp_path, monkeypatch) + + assert "animations: 2" in result.output + assert "on PixelLab" not in result.output + + +class TestAProviderStringDoesNotRewriteTheTerminal: + """An animation's name is whatever was last typed into it, and it now reaches a + refusal and the terminal. The record already takes control characters out of a + manifest for this reason; a payload is the same boundary.""" + + ESCAPE = chr(27) + RETURN = chr(13) + + def test_an_escape_sequence_in_a_name_is_taken_out(self): + animation = {"display_name": "walk" + self.ESCAPE + "[2Kpaid in full"} + + assert motion_of(animation) == "walk[2Kpaid in full" + + def test_a_direction_carrying_one_is_taken_out_too(self): + direction = "south" + self.RETURN + self.ESCAPE + "[A" + payload = { + "animations": [{"animation_type": "walk", "directions": [{"direction": direction}]}] + } + + assert gather_animations(payload)[0]["directions"] == ["south[A"] diff --git a/tests/test_subject.py b/tests/test_subject.py index ae000f0..91f99f8 100644 --- a/tests/test_subject.py +++ b/tests/test_subject.py @@ -58,7 +58,9 @@ def a_state(home, version=2, state_id=POSE, of=CHARACTER, pose="mid-stride walki ) -def an_animation(home, version=1, of=CHARACTER, start_pose=POSE, name="walking"): +def an_animation( + home, version=1, of=CHARACTER, start_pose=POSE, name="walking", directions=("south",) +): return manifest( home, "animations", @@ -69,7 +71,7 @@ def an_animation(home, version=1, of=CHARACTER, start_pose=POSE, name="walking") "links": { "character_id": of, "start_pose": start_pose, - "directions": ["south"], + "directions": list(directions), "name": name, }, "ids": {}, @@ -357,3 +359,63 @@ def test_an_escape_sequence_never_reaches_the_reader(self, tmp_path): built = subjects.build("warrior", Workspace(root=tmp_path)) assert built.characters[0]["description"] == "a knight[2Kand a lie" + + +class TestOneMotionAnimatedOverSeveralCalls: + """R7.5: PixelLab starts a new animation per call, so the directions of one motion + arrive as separate runs. The record holds one animation over all of them.""" + + def test_two_runs_of_one_motion_are_one_animation(self, tmp_path): + home = tmp_path / "warrior" + a_character(home) + an_animation(home, version=1, directions=("south",)) + an_animation(home, version=2, directions=("north",)) + + subject = subjects.load(Workspace(tmp_path), "warrior") + + animations = subject.characters[0]["animations"] + assert len(animations) == 1 + assert animations[0]["directions"] == ["south", "north"] + + def test_it_holds_every_file_both_runs_wrote(self, tmp_path): + home = tmp_path / "warrior" + a_character(home) + an_animation(home, version=1, directions=("south",)) + an_animation(home, version=2, directions=("north",)) + + subject = subjects.load(Workspace(tmp_path), "warrior") + + animation = subject.characters[0]["animations"][0] + assert len(animation["files"]) == 18 + assert len(animation["runs"]) == 2 + + def test_each_run_keeps_the_directory_it_wrote_to(self, tmp_path): + home = tmp_path / "warrior" + a_character(home) + an_animation(home, version=1) + an_animation(home, version=2, directions=("north",)) + + subject = subjects.load(Workspace(tmp_path), "warrior") + + directories = [run["directory"] for run in subject.characters[0]["animations"][0]["runs"]] + assert len(set(directories)) == 2 + + def test_another_motion_stays_its_own_animation(self, tmp_path): + home = tmp_path / "warrior" + a_character(home) + an_animation(home, version=1, name="walking") + an_animation(home, version=2, name="attacking") + + subject = subjects.load(Workspace(tmp_path), "warrior") + + assert len(subject.characters[0]["animations"]) == 2 + + def test_a_direction_animated_twice_is_named_once(self, tmp_path): + home = tmp_path / "warrior" + a_character(home) + an_animation(home, version=1) + an_animation(home, version=2) + + subject = subjects.load(Workspace(tmp_path), "warrior") + + assert subject.characters[0]["animations"][0]["directions"] == ["south"] From 4876fb9644d9e11269e7890c1a55c6693bf5960f Mon Sep 17 00:00:00 2001 From: prode Date: Sun, 20 Sep 2026 17:29:59 -0300 Subject: [PATCH 2/2] chore(specs): record the branch and the pull request --- specs/asset-workspace/requirements.md | 6 +++--- specs/characters-and-animation/requirements.md | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/specs/asset-workspace/requirements.md b/specs/asset-workspace/requirements.md index 3f9a76c..cbfc04f 100644 --- a/specs/asset-workspace/requirements.md +++ b/specs/asset-workspace/requirements.md @@ -1,9 +1,9 @@ --- autonomy: auto ci: wait -branch: feat/the-subject-manifest -delivery: merged -pr: 60 +branch: feat/one-animation-many-directions +delivery: in-review +pr: 78 --- # Asset workspace — requirements diff --git a/specs/characters-and-animation/requirements.md b/specs/characters-and-animation/requirements.md index 67b3b8b..9ac146f 100644 --- a/specs/characters-and-animation/requirements.md +++ b/specs/characters-and-animation/requirements.md @@ -2,8 +2,8 @@ autonomy: auto ci: wait branch: feat/one-animation-many-directions -delivery: in-progress -pr: 68 +delivery: in-review +pr: 78 --- # Characters and animation — requirements