jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(client): character visual pipeline — compositor, descriptor, assets #95

Closed
jpmschweitzer wants to merge 0 commits from client into main
pull from: client
merge into: :main
Conversation 2 Commits - Files Changed -
jpmschweitzer commented 2026-03-20 07:49:32 +01:00
Owner
Copy Link
Copy Source

Summary

Sprint 28 client deliverables — character visual pipeline foundation:

  • #702: Port spike shaders (toon, toon_masked, outline), skin tone textures (9 PNGs), and GLTF import setting to production client
  • #703: CharacterVisualDescriptor GDScript class — 11-variant BodyType enum, wire format encode/decode, body_type_key mapping for asset path resolution
  • #704: CharacterVisual compositor — runtime 3D character assembler with slot architecture (body segments, head, hair, facial hair, eyebrows, clothing, accessories), BoneAttachment3D, clothing coverage via coverage.json, recolor mask loading, skin tone tinting, bone validation
  • #706: Import 65-bone armature and Universal Animation Libraries (UAL1 + UAL2, 88 animations total)
  • Docs: Camera presets confirmed (frontal -5°, dramatic -30°, top-down 0°), bone names corrected to match Quaternius rig, accessory slots expanded (wrist_l/r, earring_l/r, necklace), eyebrow naming fixed

64 unit tests covering descriptor round-trips and compositor API.

Test plan

  • Verify shaders, skin tones, skeleton, animations present at client/assets/characters/
  • Verify gltf/embedded_image_handling=3 in client/project.godot
  • GDScript parse check on character_visual.gd and character_visual_descriptor.gd
  • Unit tests pass (descriptor wire format, compositor API)
  • Compositor gracefully handles missing body segment GLBs (warns, no crash)

🤖 Generated with Claude Code

## Summary Sprint 28 client deliverables — character visual pipeline foundation: - **#702:** Port spike shaders (toon, toon_masked, outline), skin tone textures (9 PNGs), and GLTF import setting to production client - **#703:** CharacterVisualDescriptor GDScript class — 11-variant BodyType enum, wire format encode/decode, body_type_key mapping for asset path resolution - **#704:** CharacterVisual compositor — runtime 3D character assembler with slot architecture (body segments, head, hair, facial hair, eyebrows, clothing, accessories), BoneAttachment3D, clothing coverage via coverage.json, recolor mask loading, skin tone tinting, bone validation - **#706:** Import 65-bone armature and Universal Animation Libraries (UAL1 + UAL2, 88 animations total) - **Docs:** Camera presets confirmed (frontal -5°, dramatic -30°, top-down 0°), bone names corrected to match Quaternius rig, accessory slots expanded (wrist_l/r, earring_l/r, necklace), eyebrow naming fixed 64 unit tests covering descriptor round-trips and compositor API. ## Test plan - [ ] Verify shaders, skin tones, skeleton, animations present at `client/assets/characters/` - [ ] Verify `gltf/embedded_image_handling=3` in `client/project.godot` - [ ] GDScript parse check on `character_visual.gd` and `character_visual_descriptor.gd` - [ ] Unit tests pass (descriptor wire format, compositor API) - [ ] Compositor gracefully handles missing body segment GLBs (warns, no crash) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 3 commits 2026-03-20 07:49:32 +01:00
feat(client): port character shaders, skin tones, skeleton, and animations (#702, #706) ff3a8e95e4 
Port proven shaders (toon, toon_masked, outline) and skin tone textures
from quaternius-aesthetic spike to production client. Import 65-bone
armature and Universal Animation Libraries (UAL1 + UAL2). Set
gltf/embedded_image_handling=3 in project.godot (critical import setting).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
feat(client): CharacterVisualDescriptor and compositor (#703, #704) 23ec1ddebd 
Add CharacterVisualDescriptor (11-variant BodyType enum, wire format
encode/decode, body_type_key mapping) and CharacterVisual compositor
(runtime 3D character assembler with slot architecture, BoneAttachment3D,
clothing coverage, recolor mask loading, skin tone tinting, bone
validation). Includes 64 unit tests and Blender utility scripts.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
docs(decisions): update camera presets, bone names, accessory slots f34e6362c4 
D-148/D-158: confirm three camera presets (frontal -5°, dramatic -30°,
top-down 0°), supersede 45° isometric. Update asset organization doc
with verified Quaternius bone names, add wrist_l/r and earring_l/r
accessory slots, fix eyebrow naming convention (short names, no prefix).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jpmschweitzer commented 2026-03-20 07:53:33 +01:00
Author
Owner
Copy Link
Copy Source

Review: client -> main (type: code)

Hoshe (Code Quality): REQUEST_CHANGES

Well-structured compositor with thorough test-first coverage. Skip-with-warning pattern is right for this stage.

# File Severity Issue
1 character_visual.gd:225 warning _clear() calls _rebuild_outlines() before mesh lists are cleared — rebuilds against about-to-be-freed meshes
2 character_visual.gd:509 warning _load_accessories() uses _bone_attachments.back() — fragile implicit coupling to _attach_to_bone() internals
3 character_visual.gd:57-66 warning SLOT_TO_BONE missing 6 of 12 accessory slots from architecture doc
4 toon_masked.gdshader:2 warning cull_disabled without ALPHA output — renders interior faces of all toon_masked geometry
5 toon_masked.gdshader:31 warning tint_color alpha silently discarded (inconsistent with toon.gdshader which does write ALPHA)
6 character_visual_descriptor.gd:132 suggestion skin_tone from from_dict() not range-validated (deferred to use-time clamp)
7 blender_list_bones.py suggestion No docstring, no -- argv guard (inconsistent with other Blender scripts)
8 test_character_visual_sprint28.gd:162 suggestion Reload test sentinel <= count * 2 is too loose — should be == count

Tyre (Architecture): REQUEST_CHANGES

Compositor matches the approved asset organization proposal well. Path derivation is purely convention-based, descriptor contract is well-typed. Two contract issues need fixing.

# File Severity Issue
1 character_visual.gd:57-66 warning SLOT_TO_BONE missing 4 slots from spec (earring_l/r, necklace, wrist_l/r); earring vs earring_l/r naming mismatch creates silent descriptor drops
2 decisions/architecture.md D-160 warning D-160 says "15 base segments" but implementation and org doc both use 17; test comment at line 19 propagates stale figure
3 character_visual.gd:553 suggestion Shared outline ShaderMaterial will need per-instance duplication when D-152 LOD tiers arrive
4 character_visual.gd:225 suggestion _clear() call ordering for outline teardown is non-obvious — needs dependency comment
5 blender_list_bones.py suggestion No docstring, no __main__ guard, crashes without -- separator

Verdict: CHANGES REQUESTED

Overlap: Both reviewers flag the SLOT_TO_BONE gap (Hoshe #3 / Tyre #1) and _clear() ordering (Hoshe #1 / Tyre #4). Unique warnings: Hoshe catches the _bone_attachments.back() fragility and the shader cull_disabled/ALPHA issues; Tyre catches the D-160 stale segment count.

Priority fixes:

  1. SLOT_TO_BONE — sync with architecture doc (12 slots, L/R earring split)
  2. _clear() — fix ordering or extract teardown-only method
  3. toon_masked.gdshader — either remove cull_disabled or justify with comment + add ALPHA output for consistency
  4. D-160 — update from "15" to "17" base segments
  5. _attach_to_bone() — return the BoneAttachment3D node instead of bool

Reviewed by Hoshe (code quality) and Tyre (architecture) via /pr-review

## Review: client -> main (type: code) ### Hoshe (Code Quality): REQUEST_CHANGES Well-structured compositor with thorough test-first coverage. Skip-with-warning pattern is right for this stage. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `character_visual.gd:225` | warning | `_clear()` calls `_rebuild_outlines()` before mesh lists are cleared — rebuilds against about-to-be-freed meshes | | 2 | `character_visual.gd:509` | warning | `_load_accessories()` uses `_bone_attachments.back()` — fragile implicit coupling to `_attach_to_bone()` internals | | 3 | `character_visual.gd:57-66` | warning | `SLOT_TO_BONE` missing 6 of 12 accessory slots from architecture doc | | 4 | `toon_masked.gdshader:2` | warning | `cull_disabled` without `ALPHA` output — renders interior faces of all toon_masked geometry | | 5 | `toon_masked.gdshader:31` | warning | `tint_color` alpha silently discarded (inconsistent with `toon.gdshader` which does write `ALPHA`) | | 6 | `character_visual_descriptor.gd:132` | suggestion | `skin_tone` from `from_dict()` not range-validated (deferred to use-time clamp) | | 7 | `blender_list_bones.py` | suggestion | No docstring, no `--` argv guard (inconsistent with other Blender scripts) | | 8 | `test_character_visual_sprint28.gd:162` | suggestion | Reload test sentinel `<= count * 2` is too loose — should be `== count` | ### Tyre (Architecture): REQUEST_CHANGES Compositor matches the approved asset organization proposal well. Path derivation is purely convention-based, descriptor contract is well-typed. Two contract issues need fixing. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `character_visual.gd:57-66` | warning | `SLOT_TO_BONE` missing 4 slots from spec (`earring_l/r`, `necklace`, `wrist_l/r`); `earring` vs `earring_l/r` naming mismatch creates silent descriptor drops | | 2 | `decisions/architecture.md` D-160 | warning | D-160 says "15 base segments" but implementation and org doc both use 17; test comment at line 19 propagates stale figure | | 3 | `character_visual.gd:553` | suggestion | Shared outline `ShaderMaterial` will need per-instance duplication when D-152 LOD tiers arrive | | 4 | `character_visual.gd:225` | suggestion | `_clear()` call ordering for outline teardown is non-obvious — needs dependency comment | | 5 | `blender_list_bones.py` | suggestion | No docstring, no `__main__` guard, crashes without `--` separator | ### Verdict: CHANGES REQUESTED **Overlap:** Both reviewers flag the `SLOT_TO_BONE` gap (Hoshe #3 / Tyre #1) and `_clear()` ordering (Hoshe #1 / Tyre #4). Unique warnings: Hoshe catches the `_bone_attachments.back()` fragility and the shader `cull_disabled`/`ALPHA` issues; Tyre catches the D-160 stale segment count. **Priority fixes:** 1. `SLOT_TO_BONE` — sync with architecture doc (12 slots, L/R earring split) 2. `_clear()` — fix ordering or extract teardown-only method 3. `toon_masked.gdshader` — either remove `cull_disabled` or justify with comment + add `ALPHA` output for consistency 4. D-160 — update from "15" to "17" base segments 5. `_attach_to_bone()` — return the `BoneAttachment3D` node instead of `bool` --- *Reviewed by Hoshe (code quality) and Tyre (architecture) via `/pr-review`*
jpmschweitzer added 1 commit 2026-03-20 07:59:27 +01:00
fix(client): address PR #95 review — 8 items bcd5887b82 
SLOT_TO_BONE expanded to 12 slots (earring_l/r, necklace, wrist_l/r).
_clear() outline teardown ordering fixed. toon_masked.gdshader: removed
cull_disabled, added ALPHA output. _attach_to_bone() returns node instead
of bool. skin_tone clamped in from_dict(). D-160 segment count 15→17.
Blender scripts: docstrings + __main__ guards. Reload test tightened.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jpmschweitzer commented 2026-03-20 09:05:15 +01:00
Author
Owner
Copy Link
Copy Source

Review: client -> main (type: code) — Round 2

Hoshe (Code Quality): APPROVE

All 8 fixes from round 1 verified. Three suggestions for follow-up:

# File Severity Issue
1 toon_masked.gdshader suggestion No depth_draw_alpha_prepass — cutout alpha on hair/accessories may sort incorrectly
2 character_visual.gd:229 suggestion _accessory_attachments / _bone_attachments coupling is implicit — add comment
3 test_character_visual_sprint28.gd:143 suggestion Reload test checks child count, not internal array sizes — gap in regression coverage

Tyre (Architecture): APPROVE

All 5 fixes from round 1 verified. One counting comment inconsistency and doc drift:

# File Severity Issue
1 character_visual.gd:37 warning Comment says "17 base" but array has 18 entries (torso_upper counted in array). Suggest: "18 segments (16 body + eyes + eyebrows; torso_upper hidden by default)"
2 test_character_visual_sprint28.gd:19 suggestion Test docstring says "17 bone-group regions" — same counting confusion
3 character_visual.gd:583 suggestion _rebuild_outlines() needs comment "must be called after segment visibility is finalized"
4 character_visual_descriptor.gd:5 suggestion No mechanism to detect wire format drift from server
5 character-asset-organization.md Section 3.3 suggestion Accessory slot list (9 entries) doesn't match code (12 entries with earring l/r, wrist l/r)

Verdict: APPROVED

Both reviewers approve. Segment count comment is the only warning-level item — it's a comment issue, not a code bug (the array itself is correct). Doc drift on accessory slots should be fixed as a follow-up.

Re-reviewed by Hoshe (code quality) and Tyre (architecture) via /pr-review

## Review: client -> main (type: code) — Round 2 ### Hoshe (Code Quality): APPROVE All 8 fixes from round 1 verified. Three suggestions for follow-up: | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `toon_masked.gdshader` | suggestion | No `depth_draw_alpha_prepass` — cutout alpha on hair/accessories may sort incorrectly | | 2 | `character_visual.gd:229` | suggestion | `_accessory_attachments` / `_bone_attachments` coupling is implicit — add comment | | 3 | `test_character_visual_sprint28.gd:143` | suggestion | Reload test checks child count, not internal array sizes — gap in regression coverage | ### Tyre (Architecture): APPROVE All 5 fixes from round 1 verified. One counting comment inconsistency and doc drift: | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `character_visual.gd:37` | warning | Comment says "17 base" but array has 18 entries (torso_upper counted in array). Suggest: "18 segments (16 body + eyes + eyebrows; torso_upper hidden by default)" | | 2 | `test_character_visual_sprint28.gd:19` | suggestion | Test docstring says "17 bone-group regions" — same counting confusion | | 3 | `character_visual.gd:583` | suggestion | `_rebuild_outlines()` needs comment "must be called after segment visibility is finalized" | | 4 | `character_visual_descriptor.gd:5` | suggestion | No mechanism to detect wire format drift from server | | 5 | `character-asset-organization.md` Section 3.3 | suggestion | Accessory slot list (9 entries) doesn't match code (12 entries with earring l/r, wrist l/r) | ### Verdict: APPROVED Both reviewers approve. Segment count comment is the only warning-level item — it's a comment issue, not a code bug (the array itself is correct). Doc drift on accessory slots should be fixed as a follow-up. --- *Re-reviewed by Hoshe (code quality) and Tyre (architecture) via `/pr-review`*
jpmschweitzer closed this pull request 2026-03-20 09:07:12 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#95