Skip to content

fix(tools): reject duplicate timeline buttons - #12040

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
Cankiee:fix/pad-timeline-duplicate-buttons
Oct 28, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
Cankiee:fix/pad-timeline-duplicate-buttons

Conversation

@Cankiee

@Cankiee Cankiee commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

What

Reject duplicate button names in tools/pad_timeline.py, consistent with the existing validation in pad_input.py.

Previously, timeline events such as ["cross", "cross"] were silently accepted and compiled into the same button bitmask as a single button.

This PR adds duplicate detection with an event-specific error message and regression tests covering repeated buttons, duplicate combinations, and repeated release entries.

Tested

  • Windows 11, Python 3.14.
  • python -m unittest discover -s tools/tests -p "test_pad_timeline.py" -v — 19 tests, 1 skipped (POSIX permissions).
  • python -m unittest discover -s tools/tests -p "test_pad_input.py" -v — 18 tests, 2 skipped (POSIX permissions).
  • python -m py_compile tools/pad_timeline.py tools/tests/test_pad_timeline.py
  • git diff --check
  • No emulator, game, or GPU testing performed.

Checklist

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 0fdce2faf4684703a07bdc2dddbc724305ddd33c. Duplicate button names previously collapsed silently into one bitmask; the new validation rejects them and the regression test covers duplicate ordinary buttons, combinations, and release entries.\n\nValidation here: all 19 test_pad_timeline.py tests passed from the exact-head worktree; py_compile and git diff --check passed. Hosted conventions, intake, Linux/Windows relinker, and sanitizer checks pass. macOS relinker jobs remain pending.

@THEMANnt THEMANnt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 0fdce2faf4684703a07bdc2dddbc724305ddd33c — approve. Duplicate button names are rejected before forming a controller timeline mask.

Windows/Python 3.11: head 18 passed, 1 skipped (19 total); author base 17 passed, 1 skipped (18 total). Current tests against old production fail three duplicate-button assertions. Normal multi-button combinations and release-only events retain their existing rules. No title or controller runtime was exercised.

The inspected hosted Windows relinker job merged this exact head and completed successfully. This is not a full product or game build.

@github-actions

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 10578 tools/tests/test_pad_timeline.py

@rooklz rooklz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced on macOS arm64, Python 3.13, at head 0fdce2faf4. The head merges cleanly onto main ccfb96757f.

The test discriminates. test_pad_timeline passes on head: 19 tests OK, 1 skipped. With only tools/pad_timeline.py reverted to main, all three subtests of test_duplicate_buttons_are_rejected fail (cross,cross, l1,r1,l1, release,release). main accepts each of them.

It's consistent with the existing rule. pad_input.py:57 already rejects len(set(names)) != len(names), so the two tools now agree on duplicates. The duplicate check comes before the release-combination check. So ["release","release"] reports the duplicate message, and that's what the test asserts. An empty buttons list is still accepted as before; this PR doesn't change that.

check_conventions --base upstream/main passes, and test_pad_input is still OK. pr_overlap lists only #10578 (tools/tests/test_pad_timeline.py, which conflicts with base). That PR is about advancing sequences past invalid timelines, not a duplicate of this one, so expect a trivial test-file rebase for whichever merges second.

AI-assisted: yes (Claude Code).

@oneandonlydean oneandonlydean self-assigned this Oct 28, 2026
@oneandonlydean
oneandonlydean merged commit e52c925 into boykopovar:main Oct 28, 2026
13 checks passed
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.

5 participants