Skip to content

Skip all sectors after a subcode desync instead of just the first#341

Merged
superg merged 1 commit intosuperg:mainfrom
hiddenpalaceorg:fix/subcode-desync-skip
Feb 4, 2026
Merged

Skip all sectors after a subcode desync instead of just the first#341
superg merged 1 commit intosuperg:mainfrom
hiddenpalaceorg:fix/subcode-desync-skip

Conversation

@Sazpaimon
Copy link
Contributor

@Sazpaimon Sazpaimon commented Feb 4, 2026 •
edited
Loading

The original function would only skip one sector after a subcode desync. The intent is to skip all sectors untl the desync ends.

Summary by CodeRabbit

  • Refactor
    • Internal logic improvements to subcode shift handling for better code maintainability and consistency.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 4, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

The check_subcode_shift function's skip flag logic was refactored to move its condition check outside the shift-detection branch. Skip is now set unconditionally whenever subcode_shift is nonzero and the skip_subcode_desync option is enabled, rather than only within specific control flow branches.

Changes

Cohort / File(s) Summary
Skip Flag Logic Refactoring
cd/cd_dump.ixx		 Moved skip flag condition check to unconditional block after subcode processing, changing when skip is set based on subcode_shift state and options rather than shift detection branches.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A rabbit hops through logic gates,
Rearranging skip's fates,
When subcode shifts, now skip will know,
In cleaner paths the bits shall flow! 🐰✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: refactoring the skip logic to apply to all sectors after a subcode desync rather than just the first one.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

superg
superg approved these changes Feb 4, 2026
View reviewed changes
Copy link
Owner

@superg superg left a comment

Choose a reason for hiding this comment

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

Nice catch!

@superg superg merged commit 7f48611 into superg:main Feb 4, 2026
11 checks passed
Deterous added a commit to Deterous/redumper that referenced this pull request Feb 5, 2026
@Deterous @Sazpaimon
@superg @coderabbitai
superg changes (#17)
3afe9a9 
* Skip all sectors after a subcode desync instead of just the first (superg#341)

* Initial OmniDrive support (superg#336) (superg#340)

* Initial OmniDrive support (superg#336)

* Initial OmniDrive support

* Count SCSI errors in dumpable range during refine

* Initial Xbox support

* Raw BD support

* revert bd

* revert one more

* Code review

* changed dvd scrambler table init to construction (temporary?)

* revert code that will come in other PRs

* refactoring

* rework drive info output

* recording/data frame conversion, dvd dump loop refactored

* Apply suggestions from code review

CR feedback

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* added parity generation

* commonize overread count, dvd overread configuration, PSN refactoring

* chunk refactoring, addressed CR review

* removed unnecessary code

* addressed CR issue

---------

Co-authored-by: Deterous <138427222+Deterous@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* reverted to --rings option, removed desync stats (superg#342)

---------

Co-authored-by: Sazpaimon <sazpaimon@x-cult.org>
Co-authored-by: Hennadiy Brych <gennadiy.brich@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@superg superg superg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Sazpaimon @superg