Move tile_accent / tile_style rendering helpers into a render module - #259
Merged
Merged
Conversation
Create scripts/tile_render.gd with pure (no scene reference) functions: - TileRender.tile_style() — returns StyleBoxFlat using theme dict - TileRender.tile_accent() — returns accent Color using context dict Pass pending_build_kind, hover_pos, stockpile_pos, and can_place_fn via a context dict parameter so tile rendering is testable without scene tree instantiation. Constants (RESOURCE_COLORS, STRUCTURE_COLORS, TILE_BACKDROPS) are passed via a theme dict from main.gd. Add 18 regression tests in tests/test_tile_render.gd covering all tile types (ground, tree, rock, berries, foundation, structures), build placement highlights, stockpile accents, and integration with real constants. Fixes #235 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
![its-saffron[bot]](https://avatars.githubusercontent.com/in/3901073?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The main.gd script already delegated tile_style() and tile_accent() calls to TileRender.tile_style() and TileRender.tile_accent(), but the TileRender class was never preloaded, causing a runtime error. Added the missing preload statement to wire up the rendering module. Fixes #235 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add explicit type annotations to variables assigned from Dictionary.get() calls, which return Variant and trigger Godot's strict type inference warnings-as-errors. This resolves the headless smoke test failure. - Added explicit Vector2i casts for hover_pos and stockpile_pos - Added explicit Dictionary type annotations for resource_colors and structure_colors Fixes #235 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
its-saffron
Bot
dismissed
their stale review
July 8, 2026 08:38
Superseded by a newer automated review for this pull request.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Godot's strict type inference was treating implicit type warnings as errors in CI, causing the headless smoke test to fail. Added explicit type annotations to all variables assigned from Dictionary.get() calls: - pending_build_kind: String - hover_pos: Vector2i - stockpile_pos: Vector2i - resource_colors: Dictionary - structure_colors: Dictionary Fixes #235 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
its-saffron
Bot
dismissed
their stale review
July 8, 2026 11:11
Superseded by a newer automated review for this pull request.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
Author
Reason: PR review: CHANGES_REQUESTED Posted automatically by Dispatch on 2026-07-08T11:26:01.360Z |
Godot requires libfontconfig.so.1 for text rendering, which is missing on the Ubuntu CI runner. This caused the headless smoke test to fail with: ERROR: The dependent shared library 'libfontconfig.so.1' could not be loaded Add an apt-get install step for libfontconfig1 before running the smoke test. Fixes #235 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
joryirving
added
ai-review
its-saffron
Bot
dismissed
their stale review
July 8, 2026 18:03
Superseded by a newer automated review for this pull request.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
its-saffron
Bot
dismissed
their stale review
July 8, 2026 18:04
Superseded by a newer automated review for this pull request.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Move pure tile rendering helpers out of scripts/main.gd into a dedicated TileRender class so tile rendering can be tested without a scene tree. - scripts/tile_render.gd: pure TileRender class with static tile_style() and tile_accent() methods. Both accept plain data (tile dict, pos, theme dict, context dict) and read constants from scripts/constants.gd (RESOURCE_COLORS, STRUCTURE_COLORS, TILE_BACKDROPS) — no scene refs. - scripts/main.gd: preload TileRender and delegate the two render helpers, passing pending_build_kind, hovered_tile_pos, stockpile_pos, and can_place_at via a context dict built from own state. - tests/test_tile_render.gd: 19 SceneTree-based tests covering tile_style (border color, stockpile override, unknown-kind fallback, corner radius, shadow, return type) and tile_accent (build placement valid/invalid, stockpile, resource/structure colors, foundation, hover mismatch, empty context, real-constant integration). Fixes #235 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
There was a problem hiding this comment.
AI Automated Review (incremental)
Incremental review: reviewed the changes since the last managed review; unresolved findings from that review are carried forward.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)
Recommendation: Approve
This PR extracts tile_style and tile_accent rendering helpers into a dedicated TileRender module (scripts/tile_render.gd) with 19 comprehensive tests, following established repository patterns for testability.
Change Summary
New Module: scripts/tile_render.gd
class_name TileRenderwith pure static functionstile_style(): CreatesStyleBoxFlatwith border colors, backdrop colors, corner radius, shadowtile_accent(): ReturnsColorfor overlay accents based on tile kind, build state, hover state, stockpile state, and resource/structure colors- No scene references — fully testable without scene tree
Test Suite: tests/test_tile_render.gd
- 19 regression tests covering:
tile_style: StyleBoxFlat type, border color, background from TILE_BACKDROPS, stockpile override, unknown kind default, corner radius, shadowtile_accent: build placement valid/invalid, stockpile gold, resource color, structure color, foundation, hover mismatch, empty context- Integration: uses real
RESOURCE_COLORSandSTRUCTURE_COLORSfrom constants.gd
Integration: scripts/main.gd
- Delegates
tile_style()andtile_accent()toTileRender.tile_style()andTileRender.tile_accent() - Passes
render_themedict containingTILE_BACKDROPS,RESOURCE_COLORS,STRUCTURE_COLORS
CI: .github/workflows/test.yml
- Added
libfontconfig1dependency installation, fixing the fontconfig error shown in smoke.log
Standards Compliance
Verified:
- ✅ Follows repository extraction pattern:
tile_render.gdmirrorsworker_renderer.gd(issue PR 232),goal_progression.gd,worker_cap_logic.gd - ✅ Pure functions with no scene references
- ✅ Class-based module with
class_namedeclaration - ✅ Comprehensive test coverage with regression tests
- ✅ Uses existing constants (
RESOURCE_COLORS,STRUCTURE_COLORS,TILE_BACKDROPS) - ✅ Smoke test CI updated to fix dependency issue
Linked Issue Fit
Issue PR 235 calls for extracting tile rendering helpers into a testable module. This PR delivers:
TileRenderclass with pure rendering functions- 19 comprehensive tests validating tile_style and tile_accent behavior
- Integration with main.gd via delegation pattern
No acceptance criteria document was provided in context, but the PR body commits to "19 comprehensive tests" — verified as delivered.
Evidence Provider Findings
No evidence providers configured.
Tool Harness Findings
- ✅ Test file
tests/test_tile_render.gdread successfully: 19 tests defined covering both functions - ✅ Workflow file
.github/workflows/test.ymlread successfully: libfontconfig1 dependency added - ✅ RESOURCE_COLORS usage verified across constants.gd, main.gd, tile_render.gd, and tests
CI Results
| Check | Status |
|---|---|
| macOS validation | ✅ success |
| Script test suite | ✅ success |
| Headless smoke test | ✅ success |
Unknowns / Needs Verification
None — all relevant evidence was available and verified.
Conclusion
This PR is a clean extraction refactor that:
- Improves testability by moving rendering logic out of main.gd
- Adds comprehensive regression tests (19 tests)
- Fixes a CI dependency issue (libfontconfig1)
- Follows established repository patterns
- Passes all CI checks including macOS validation
No blockers identified. The implementation is ready to merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracted tile_style and tile_accent into a pure TileRender class with 19 comprehensive tests.
Fixes #235
Opened by foreman on review GO (workload wl-misospace-windowstead-235).