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

refactor(ui): stance icons with tint shader (#787) #118

Closed
jpmschweitzer wants to merge 0 commits from sprint-32/client into main
pull from: sprint-32/client
merge into: :main
Conversation 3 Commits - Files Changed -
+119 -18
jpmschweitzer commented 2026-04-06 16:34:27 +02:00
Owner
Copy Link
Copy Source

Summary

  • Stance indicator: replace text-only labels with SVG icons + alpha-mask tint shader per D-086
  • New icon_tint.gdshader — samples texture alpha, applies runtime tint_color parameter
  • Icons: walk, crouch, careful, sprint at 18px inside ImplantPanel row
  • Fix stale UILayer/HUD test path in test_ui_framework_sprint15.gd

Files changed

  • client/shaders/icon_tint.gdshader — new alpha-mask tint shader
  • client/ui/stance_indicator.gd — TextureRect + ShaderMaterial replaces text label
  • client/tests/test_ui_framework_sprint15.gd — stale path fix
  • CHANGELOG.md — updated

Decisions referenced

  • D-086 (insert icon system)
  • D-169 (implant UI component library)

Test plan

  • Verify all 4 stance icons display correctly (walk, crouch, careful, sprint)
  • Verify icon tint color matches stance color (white-blue, amber, green, red)
  • Verify icon fits within 44px ImplantPanel row without clipping
  • Run make test-client for unit test pass
## Summary - Stance indicator: replace text-only labels with SVG icons + alpha-mask tint shader per D-086 - New `icon_tint.gdshader` — samples texture alpha, applies runtime `tint_color` parameter - Icons: walk, crouch, careful, sprint at 18px inside ImplantPanel row - Fix stale `UILayer/HUD` test path in `test_ui_framework_sprint15.gd` ## Files changed - `client/shaders/icon_tint.gdshader` — new alpha-mask tint shader - `client/ui/stance_indicator.gd` — TextureRect + ShaderMaterial replaces text label - `client/tests/test_ui_framework_sprint15.gd` — stale path fix - `CHANGELOG.md` — updated ## Decisions referenced - D-086 (insert icon system) - D-169 (implant UI component library) ## Test plan - [ ] Verify all 4 stance icons display correctly (walk, crouch, careful, sprint) - [ ] Verify icon tint color matches stance color (white-blue, amber, green, red) - [ ] Verify icon fits within 44px ImplantPanel row without clipping - [ ] Run `make test-client` for unit test pass
jpmschweitzer added 2 commits 2026-04-06 16:34:28 +02:00
refactor(ui): add stance icons to implant badge (#787) a14828e02a 
Replace text-only stance labels with SVG icons + tint shader per D-086.
Alpha-mask shader swaps color at runtime via ShaderMaterial parameter.
Icons: walk, crouch, careful, sprint at 18px inside ImplantPanel row.
Also fix stale UILayer/HUD test path in test_ui_framework_sprint15.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog bb61d860d6 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-04-06 16:39:12 +02:00
Author
Owner
Copy Link
Copy Source

Review: sprint-32/client → main (PR #118, type: code)

Reviewers: Hoshe (code quality), Tyre (architecture)
Lint: gdlint clean.

Hoshe (Code Quality): REQUEST_CHANGES

# Sev File Issue
1 High stance_indicator.gd:34,48 icon_shader not null-checked. Missing shader = broken material + runtime errors.
2 High stance_indicator.gd:73 _icon_mat.set_shader_parameter fires even when texture load fails. Move inside if tex: or add guard.
3 Med stance_indicator.gd:45 Icon size 18×18 but D-086 specifies 20×20.
4 Med Tests No test for _apply_stance — unknown stance fallback, null-texture, no-op guard.

Tyre (Architecture): REQUEST_CHANGES

# Sev File Issue
1 High stance_indicator.gd _apply_stance() load() per transition — preload 4 icons at _ready() into Dictionary.
2 Med stance_indicator.gd _ready() No null guard on theme_res or icon_shader. Add push_error() + early return.

Verdict: CHANGES REQUESTED

5 deduplicated issues:

  1. Preload icons at _ready() (Tyre)
  2. Null-guard shader + theme loads with push_error() (both)
  3. Guard _icon_mat.set_shader_parameter consistency (Hoshe)
  4. Icon size 18→20 per D-086 (Hoshe)
  5. Add _apply_stance test coverage (Hoshe)
## Review: sprint-32/client → main (PR #118, type: code) **Reviewers:** Hoshe (code quality), Tyre (architecture) **Lint:** gdlint clean. --- ### Hoshe (Code Quality): REQUEST_CHANGES | # | Sev | File | Issue | |---|-----|------|-------| | 1 | High | `stance_indicator.gd:34,48` | `icon_shader` not null-checked. Missing shader = broken material + runtime errors. | | 2 | High | `stance_indicator.gd:73` | `_icon_mat.set_shader_parameter` fires even when texture load fails. Move inside `if tex:` or add guard. | | 3 | Med | `stance_indicator.gd:45` | Icon size 18×18 but D-086 specifies 20×20. | | 4 | Med | Tests | No test for `_apply_stance` — unknown stance fallback, null-texture, no-op guard. | ### Tyre (Architecture): REQUEST_CHANGES | # | Sev | File | Issue | |---|-----|------|-------| | 1 | High | `stance_indicator.gd` `_apply_stance()` | `load()` per transition — preload 4 icons at `_ready()` into Dictionary. | | 2 | Med | `stance_indicator.gd` `_ready()` | No null guard on `theme_res` or `icon_shader`. Add `push_error()` + early return. | --- ### Verdict: CHANGES REQUESTED **5 deduplicated issues:** 1. Preload icons at `_ready()` (Tyre) 2. Null-guard shader + theme loads with `push_error()` (both) 3. Guard `_icon_mat.set_shader_parameter` consistency (Hoshe) 4. Icon size 18→20 per D-086 (Hoshe) 5. Add `_apply_stance` test coverage (Hoshe)
jpmschweitzer added 1 commit 2026-04-06 16:41:37 +02:00
fix(ui): address PR #118 review — 5 items 746ed05e2d 
- Preload stance icons at _ready() into cache Dictionary
- Null-guard shader + theme loads with push_error() + early return
- Guard _icon_mat.set_shader_parameter inside if _icon_mat:
- Icon size 18→20px per D-086, container height 44→46px to fit
- Add _apply_stance test coverage (known + unknown stance)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-04-06 16:43:01 +02:00
Author
Owner
Copy Link
Copy Source

Re-review: sprint-32/client → main (PR #118, round 2)

Commit: 746ed05e — fix(ui): address PR #118 review — 5 items

Round 1 issues (5/5 addressed):

  1. Preload icons at _ready() — Fixed (icon cache Dictionary)
  2. Null-guard shader + theme — Fixed (push_error + early return)
  3. _icon_mat guard — Fixed (if _icon_mat:)
  4. Icon size 18→20 — Fixed (D-086 compliant)
  5. Test coverage — Fixed (known + unknown stance tests)

New blocker found:

# Sev File Issue
1 BLOCKER stance_indicator.gd:33-34 STANCE_ICONS constant is referenced in the preload loop but was removed from the file during the refactor. Will crash on startup. Restore the const STANCE_ICONS dictionary or inline the paths.

Verdict: CHANGES REQUESTED — 1 blocker (startup crash)

## Re-review: sprint-32/client → main (PR #118, round 2) **Commit:** `746ed05e` — fix(ui): address PR #118 review — 5 items ### Round 1 issues (5/5 addressed): 1. Preload icons at `_ready()` — **Fixed** (icon cache Dictionary) 2. Null-guard shader + theme — **Fixed** (push_error + early return) 3. `_icon_mat` guard — **Fixed** (if _icon_mat:) 4. Icon size 18→20 — **Fixed** (D-086 compliant) 5. Test coverage — **Fixed** (known + unknown stance tests) ### New blocker found: | # | Sev | File | Issue | |---|-----|------|-------| | 1 | BLOCKER | `stance_indicator.gd:33-34` | `STANCE_ICONS` constant is referenced in the preload loop but was **removed from the file** during the refactor. Will crash on startup. Restore the `const STANCE_ICONS` dictionary or inline the paths. | ### Verdict: CHANGES REQUESTED — 1 blocker (startup crash)
jpmschweitzer added 1 commit 2026-04-06 16:44:00 +02:00
fix(ui): restore STANCE_ICONS constant removed during refactor 168c5e0d9b 
Startup crash — preload loop referenced STANCE_ICONS but the constant
was accidentally dropped when rewriting the icon cache logic.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-04-06 16:44:53 +02:00
Author
Owner
Copy Link
Copy Source

Re-review: sprint-32/client → main (PR #118, round 3)

Commit: 168c5e0d — fix(ui): restore STANCE_ICONS constant removed during refactor

Round 2 blocker resolved: STANCE_ICONS constant restored. The preload loop at _ready() now has the dictionary it references.

All 6 issues across 2 review rounds are fixed:

  1. Preload icons at _ready() into cache Dictionary
  2. Null-guard shader + theme loads with push_error()
  3. _icon_mat.set_shader_parameter inside if _icon_mat: guard
  4. Icon size 20×20 per D-086
  5. _apply_stance test coverage (known + unknown stance)
  6. STANCE_ICONS constant restored

Verdict: APPROVED

No new issues. Ready to merge.

## Re-review: sprint-32/client → main (PR #118, round 3) **Commit:** `168c5e0d` — fix(ui): restore STANCE_ICONS constant removed during refactor Round 2 blocker resolved: `STANCE_ICONS` constant restored. The preload loop at `_ready()` now has the dictionary it references. All 6 issues across 2 review rounds are fixed: 1. Preload icons at `_ready()` into cache Dictionary 2. Null-guard shader + theme loads with `push_error()` 3. `_icon_mat.set_shader_parameter` inside `if _icon_mat:` guard 4. Icon size 20×20 per D-086 5. `_apply_stance` test coverage (known + unknown stance) 6. `STANCE_ICONS` constant restored ### Verdict: APPROVED No new issues. Ready to merge.
jpmschweitzer closed this pull request 2026-04-06 16:45:25 +02: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#118