Start adding merge strategy, closes #57#58
Conversation
CodeDoctorDE
force-pushed
the
feature/merge
branch
from
dbe5a5d to
dbfbde2
Compare
CodeDoctorDE
force-pushed
the
develop
branch
from
40efadc to
c0b44db
Compare
CodeDoctorDE
force-pushed
the
develop
branch
2 times, most recently
from
66015d5 to
0de8a21
Compare
There was a problem hiding this comment.
Pull request overview
This pull request implements a merge cells feature (closes #57) that allows users to merge cells in lines with different display strategies. The feature adds two merge modes: "Stacked" (cards stack with a visible percentage offset) and "Distribute" (cards spread across the merged cell space).
Changes:
- Adds new merge strategy data models (CellMergeStrategy hierarchy) with support for horizontal/vertical directions, reversing, and strategy-specific parameters
- Implements merge dialog UI for configuring merge strategies including span, direction, and mode-specific settings
- Updates cell rendering logic to handle merged cells with dynamic sizing and positioning based on the selected strategy
Reviewed changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| api/lib/src/models/cell.dart | Defines new merge strategy classes (CellMergeStrategy, StackedCellMergeStrategy, DistributeCellMergeStrategy, MergedCellStrategy) and CellMergeDirection enum |
| api/lib/src/models/cell.mapper.dart | Generated mapper code for merge strategy serialization/deserialization |
| api/lib/src/event/state.dart | Adds calculateSpan and getParentCell utility methods for merge logic |
| api/lib/src/event/hybrid.dart | Defines CellMergeStrategyChanged event for merge operations |
| api/lib/src/event/process/server.dart | Implements event processing logic for merging/unmerging cells and moving objects between cells |
| api/lib/src/event/event.mapper.dart | Generated mapper code for the new event type |
| app/lib/pages/game/merge.dart | Implements merge configuration dialog with sliders and controls for all merge parameters |
| app/lib/l10n/app_en.arb | Adds localization strings for merge feature UI elements |
| app/lib/board/cell.dart | Updates cell rendering to support dynamic sizing, multiple object positioning strategies, and merged cell visibility |
| app/lib/board/grid.dart | Modifies grid to use parent cell positions for merged cells and force updates on table changes |
| app/lib/board/background.dart | Refactors background rendering to use paintImage with repeating pattern and cell size scaling |
| app/lib/bloc/world/state.dart | Adds isCellSelected helper that accounts for merged cells |
| server/pubspec.lock, plugin/pubspec.lock | Updates lw_file_system_api dependency to newer commit |
| Multiple .mapper.dart files | Adds ignore_for_file: invalid_use_of_protected_member to generated files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 38 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ic, and enhance safety
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 39 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final allObjects = (cells[current] ?? TableCell()).objects | ||
| .toList(); | ||
|
|
||
| for (var i = 1; i < span; i++) { | ||
| current = direction == CellMergeDirection.horizontal | ||
| ? VectorDefinition(current.x + 1, current.y) | ||
| : VectorDefinition(current.x, current.y + 1); | ||
|
|
||
| final neighbor = cells[current] ?? TableCell(); | ||
|
|
||
| if (neighbor.objects.isNotEmpty) { | ||
| allObjects.addAll(neighbor.objects); | ||
| } | ||
|
|
||
| cells[current] = neighbor.copyWith( | ||
| merge: MergedCellStrategy(direction), | ||
|
|
||
| objects: [], | ||
| ); | ||
| } | ||
|
|
||
| cells[event.cell.position] = cells[event.cell.position]!.copyWith( | ||
| objects: allObjects, | ||
| ); |
There was a problem hiding this comment.
This merge implementation mutates game state by aggregating neighbor objects into the start cell and clearing neighbor cells (objects: []). If a user later removes or changes the merge, there is no logic to restore objects to their original cells, so cell contents can be permanently re-partitioned (and ordering semantics may change). If merge is intended to be a rendering/layout concern, avoid moving objects here and persist only merge metadata; if it is intended to physically merge stacks, add explicit unmerge/split behavior (and tests) to prevent data loss.
| final neighbor = cells[current] ?? TableCell(); | ||
|
|
||
| if (neighbor.objects.isNotEmpty) { | ||
| allObjects.addAll(neighbor.objects); | ||
| } | ||
|
|
||
| cells[current] = neighbor.copyWith( | ||
| merge: MergedCellStrategy(direction), | ||
|
|
||
| objects: [], | ||
| ); | ||
| } |
There was a problem hiding this comment.
When expanding neighbors you clear objects but leave tiles untouched. On the client, cells with MergedCellStrategy are hidden, so any tiles on those neighbor cells will effectively disappear while merged. Consider either migrating/merging tiles into the parent cell as well, or preventing merges across cells that contain tiles, or rendering tiles from merged children in the parent.
| // Optimization: Break if we are moving away from bounds | ||
| bool decreasing = true; | ||
| if (strategy is LayoutCellMergeStrategy && reverse) { | ||
| decreasing = false; | ||
| } |
There was a problem hiding this comment.
The early-break optimization sets decreasing = false whenever reverse is true, but for StackedCellMergeStrategy the computed positions still move toward the start as i decreases (because you iterate renderObjects in reverse order). This means the optimization won't kick in for reversed stacked merges and can do unnecessary sprite loads/position math for offscreen cards. Consider deriving the break direction from the actual iteration order/position delta (or handle stacked vs distribute separately).
1 participant
closes #57