Fix #17793: Apply swing to chord symbol playback#32942
Fix #17793: Apply swing to chord symbol playback#32942dpad46 wants to merge 2 commits intomusescore:masterfrom
Conversation
📝 WalkthroughWalkthroughThe playback chord-symbol event generation in 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/engraving/playback/playbackeventsrenderer.cpp`:
- Around line 77-83: The code adjusts eventTimestamp using
Swing::applySwing(...).remainingDurationMultiplier but ignores
Swing::ChordDurationAdjustment::durationMultiplier, so on-beat swung chords
never lengthen and later events can desync; update the block that computes
swingOffset and modifies eventTimestamp/duration (referencing
Swing::ChordDurationAdjustment, swingDurationAdjustment,
remainingDurationMultiplier, durationMultiplier, nominalDuration,
eventTimestamp, duration, timestampFromTicks, positionTickWithOffset,
chord->actualTicks().ticks()) to both shift the eventTimestamp by swingOffset
and also apply the durationMultiplier to the event's duration (e.g., scale or
add the extra portion derived from nominalDuration * (durationMultiplier - 1))
so the event lengthens when durationMultiplier > 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97b38878-f10f-4b2c-b741-118ebfb3424f
📒 Files selected for processing (1)
src/engraving/playback/playbackeventsrenderer.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/engraving/playback/playbackeventsrenderer.cpp`:
- Around line 61-84: Add regression tests covering the swing timing code paths
exercised by applySwingIfNeed: (1) an offbeat swung-eighth case that verifies
eventTimestamp shifts for offbeat alignment, (2) an on-beat case that verifies
duration uses Swing::ChordDurationAdjustment.durationMultiplier (and
remainingDurationMultiplier behavior), and (3) early-return cases when
findChordAtHarmony returns null and when chord->tuplet() is true. Use fixtures
to create a Score/Harmony/Chord and staff swing parameters (staff()->swing(...))
and assert both timestamp and duration outputs match expected values; include
equivalent vtests for the same scenarios so the timing behavior is covered after
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a246a06-b62f-43fb-a701-421a6542b18f
📒 Files selected for processing (1)
src/engraving/playback/playbackeventsrenderer.cpp
| static void applySwingIfNeed(const Harmony* chordSymbol, | ||
| const Score* score, | ||
| int positionTickWithOffset, | ||
| timestamp_t& eventTimestamp, | ||
| duration_t& duration) | ||
| { | ||
| const SwingParameters swing = chordSymbol->staff()->swing(chordSymbol->tick()); | ||
| if (!swing.isOn()) { | ||
| return; | ||
| } | ||
|
|
||
| const Chord* chord = findChordAtHarmony(chordSymbol); | ||
| if (!chord || chord->tuplet()) { | ||
| return; | ||
| } | ||
|
|
||
| const Swing::ChordDurationAdjustment swingDurationAdjustment = Swing::applySwing(chord, swing); | ||
| const duration_t nominalDuration = timestampFromTicks(score, positionTickWithOffset + chord->actualTicks().ticks()) - eventTimestamp; | ||
| const duration_t additionalDuration = duration - nominalDuration; | ||
| const timestamp_t swingOffset = static_cast<timestamp_t>(nominalDuration * swingDurationAdjustment.remainingDurationMultiplier); | ||
|
|
||
| eventTimestamp += swingOffset; | ||
| duration = static_cast<duration_t>(nominalDuration * swingDurationAdjustment.durationMultiplier) + additionalDuration; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Please add regression coverage for swing chord-symbol timing paths.
This is a timing-sensitive change; add tests/vtests for: offbeat swung eighth alignment, on-beat duration-multiplier behavior, and tuplet/no-chord early-return paths.
I can draft a compact test matrix and expected timestamp/duration assertions if helpful.
Also applies to: 188-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/engraving/playback/playbackeventsrenderer.cpp` around lines 61 - 84, Add
regression tests covering the swing timing code paths exercised by
applySwingIfNeed: (1) an offbeat swung-eighth case that verifies eventTimestamp
shifts for offbeat alignment, (2) an on-beat case that verifies duration uses
Swing::ChordDurationAdjustment.durationMultiplier (and
remainingDurationMultiplier behavior), and (3) early-return cases when
findChordAtHarmony returns null and when chord->tuplet() is true. Use fixtures
to create a Score/Harmony/Chord and staff swing parameters (staff()->swing(...))
and assert both timestamp and duration outputs match expected values; include
equivalent vtests for the same scenarios so the timing behavior is covered after
the change.
|
Would it be possible for chords that are not attached to notes to swing as well? I'm also experiencing some inconsistencies with the swing text switching to straight at the start, will keep investigating 2026-04-14.13-23-25.mp4 |
4 participants
Resolves: #17793, where when swing is enabled, chord symbols placed on offbeat eighth notes are played back straight, causing them to be played out of sync with the notes they are attached to (notes are correctly swung but chord symbols are not).
Fixed by updating
PlaybackEventsRenderer::renderChordSymbolto call a new helperapplySwingIfNeed, which adjusts the event timestamp and duration using logic based on the existing NoteRenderer::applySwingIfNeed.Fixed playback:
8mb.video-v5m-ohvLGyaz.mp4
Before (master):
8mb.video-VNH-Cp8vVe6R.mp4