Skip to content

Move Twos grid state and rules into a TwosGrid class - #2477

Open
xhon-pelushi wants to merge 1 commit into
InfiniTimeOrg:mainfrom
xhon-pelushi:refactor/1668-twos-grid-class
Open

Move Twos grid state and rules into a TwosGrid class#2477
xhon-pelushi wants to merge 1 commit into
InfiniTimeOrg:mainfrom
xhon-pelushi:refactor/1668-twos-grid-class

Conversation

@xhon-pelushi

Copy link
Copy Markdown

Fixes #1668.

Per the request in the issue, the grid and the functions that manipulate it move out of the Twos screen and into a new TwosGrid class. TwosGrid owns the tiles and the score and knows nothing about LVGL; Twos keeps the styles, the table and the label and just draws whatever the grid currently holds.

While moving the code I also collapsed the four swipe branches. They were the same algorithm written out once per direction, differing only in which axis they walked and in which order. TwosGrid::Slide(rowStep, colStep) takes the direction as a step pair ({0, -1} for left, {1, 0} for down, and so on), so the sliding and merging rules exist in one place instead of four. That is what removes most of the line count here.

Two small things came along with the move:

  • updateGridDisplayUpdateGridDisplay, and the moved functions are PascalCase, matching the naming used by the rest of the screens. The score label update is split out into UpdateScoreDisplay because TryMerge can no longer touch LVGL.
  • The digit buffer in UpdateGridDisplay goes from char[7] to char[11]. Reading the tile value into a local made GCC's range analysis visible across the snprintf, which surfaced a -Wformat-truncation warning that the old repeated-member-access form hid. 11 bytes holds any unsigned int, so the warning goes away rather than being suppressed. Without this the file would have gained a warning it did not have before.

No behaviour change is intended.

Verification

I do not have the ARM toolchain here, so rather than reason about it I tested the extracted logic directly. Because TwosGrid no longer depends on LVGL, I could compile the old implementation and the new one side by side on the desktop and compare them:

  • Exhaustive over 65536 seeded grid patterns × 4 directions, plus 400000 randomised grids using the full range of real tile values, each played 6 moves deep so state carries across swipes.
  • After every swipe I compared the full grid, the score, and the returned "did anything move" flag.

2662144 swipes compared, 0 mismatches. Loop order was the part I most expected to have broken, since the original walks columns-outer for horizontal swipes and rows-outer for vertical ones; rows and columns are independent for their respective directions, and the fuzzing agrees.

Also checked:

  • Compiles clean against the real lvgl submodule and generated Apps.h (-Wall -Wextra), producing an object file.
  • Warning count compared against the unmodified file: 0 before, 0 after (this is what caught the char[7] issue, which I fixed rather than shipped).
  • clang-format-14, the version CI installs, reports both files clean.

What I could not verify is the app running on real hardware or in the simulator, so the LVGL side of Twos is unexercised beyond compiling. Worth a quick look on a watch or in InfiniSim before merging.

The Twos screen held both the game state and the rules that manipulate
it alongside its LVGL objects. Move the grid, the score and the
functions that act on them into a TwosGrid class, leaving the screen
responsible only for drawing.

The four swipe branches of OnTouchEvent were the same algorithm written
out once per direction. They are replaced by TwosGrid::Slide, which
takes the direction as a row/column step, so the sliding and merging
rules now exist in one place.

Fixes InfiniTimeOrg#1668
@github-actions

Copy link
Copy Markdown

Build size and comparison to main:

Section Size Difference
text 385088B -144B
data 944B 0B
bss 22640B 0B

Run in InfiniEmu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Twos grid should be a class

1 participant