-
Notifications
You must be signed in to change notification settings - Fork 0
Move sprite generation state writes from web-ui to editor package #234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ui-sprite-grid-writes
- Modified web-ui to receive sprite data as parameter instead of generating it - Changed reloadSpriteSheet to return sprite data instead of mutating state - Updated editor to generate sprite data and update state before initializing web-ui - Updated spriteSheetManager to handle new return type and update state - Removed direct state writes for spriteLookups, hGrid, and vGrid from web-ui Co-authored-by: andorthehood <[email protected]>
Co-authored-by: andorthehood <[email protected]>
Co-authored-by: andorthehood <[email protected]>
Add comments to explain the potentially confusing naming: - hGrid = horizontal grid lines = vertical spacing = character height - vGrid = vertical grid lines = horizontal spacing = character width Co-authored-by: andorthehood <[email protected]>
| await init(mockState, canvas, memoryViews); | ||
| const spriteData = createMockSpriteData(mockState); | ||
|
|
||
| // Update state with sprite data before init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Please add this to the createMockStateWithColors, I don't want to repeat this in every single test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 7cce872. Moved the sprite data generation and state updates into createMockStateWithColors to eliminate the repetition across all test files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors sprite generation to enforce proper architectural boundaries by moving state mutation responsibilities from the web-ui package (rendering layer) to the editor package (orchestration layer). The web-ui now accepts pre-generated sprite data as a parameter and returns new sprite data without mutating state directly.
Key Changes
- Web-ui's
init()andreloadSpriteSheet()now use dependency injection pattern for sprite data instead of internally generating and mutating state - Editor package now owns sprite generation and state updates for sprite-related fields (
spriteLookups,hGrid,vGrid) - All screenshot tests updated to generate sprite data before web-ui initialization
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/editor/src/spriteSheetManager.ts | Updated to capture reloadSpriteSheet() return value and update state with new sprite data |
| packages/editor/src/index.ts | Generates sprite data before web-ui initialization and updates state accordingly |
| packages/editor/packages/web-ui/src/index.ts | Removed state writes; init() accepts spriteData parameter, reloadSpriteSheet() returns SpriteData |
| packages/editor/packages/web-ui/screenshot-tests/utils/createMockSpriteData.ts | New helper utility to generate mock sprite data for tests |
| packages/editor/packages/web-ui/screenshot-tests/*.test.ts | Updated four test files to generate sprite data and update state before initialization |
| packages/editor/packages/web-ui/README.md | Added comprehensive API documentation explaining the new initialization pattern and state usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Update state with sprite data before init | ||
| mockState.graphicHelper.spriteLookups = spriteData.spriteLookups; | ||
| mockState.graphicHelper.viewport.hGrid = spriteData.characterHeight; | ||
| mockState.graphicHelper.viewport.vGrid = spriteData.characterWidth; |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state update logic here is duplicated in three places:
- packages/editor/src/index.ts (lines 70-72)
- packages/editor/src/spriteSheetManager.ts (lines 26-28)
- All test files (lines 18-20 in each)
Consider extracting this logic into a shared helper function to reduce duplication and ensure consistency. For example:
function updateStateWithSpriteData(state: State, spriteData: SpriteData): void {
state.graphicHelper.spriteLookups = spriteData.spriteLookups;
state.graphicHelper.viewport.hGrid = spriteData.characterHeight;
state.graphicHelper.viewport.vGrid = spriteData.characterWidth;
}This would make the code more maintainable and reduce the risk of inconsistencies if this logic needs to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied in ea6d1d2. Created updateStateWithSpriteData helper function to eliminate the duplication across editor initialization, spriteSheetManager, and test utilities.
Move the repetitive sprite data generation and state updates from individual test files into the createMockStateWithColors helper function to reduce code duplication across all screenshot tests. Co-authored-by: andorthehood <[email protected]>
Created updateStateWithSpriteData helper to eliminate duplication of the state update logic across editor initialization, spriteSheetManager, and test utilities. This ensures consistency and makes future changes easier to maintain. Co-authored-by: andorthehood <[email protected]>
Decouple Web-UI Sprite/Grid Writes
Goal
Move ownership of sprite generation outputs (
spriteLookups) and grid sizing (hGrid/vGrid) from@8f4e/web-uito the editor package, so that web-ui becomes read-only with respect to these state fields.Implementation Complete
✅ All Success Criteria Met:
state.graphicHelper.spriteLookups,state.graphicHelper.viewport.hGrid, orstate.graphicHelper.viewport.vGridChanges Summary
Web-UI Package:
initto acceptspriteDataparameterreloadSpriteSheetto returnSpriteDataEditor Package:
spriteSheetManagerhandles sprite reload state updatesupdateStateWithSpriteDatahelper to eliminate duplication of state update logicTests:
createMockStateWithColorshelperDocumentation:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.