Add per-note playback start/duration offset properties - #34546
Add per-note playback start/duration offset properties#34546tharos-devs wants to merge 9 commits into
Conversation
Adds two new per-note properties (playbackStartOffset, playbackDurationOffset) that let users nudge a note's audio playback timing independently of its notated position and duration. Exposed in the Properties panel's Play section and on the plugins API. Resolves musescore#34545
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds integer playback start and duration offsets to 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped 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. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engraving/dom/note.cpp`:
- Around line 4216-4222: Update Note::effectivePlaybackDuration() to compute
duration from ch->ticks().ticks() plus playbackDurationOffset(), without
subtracting playbackStartOffset(). Ensure nonpositive results are clamped or
skipped before rendering, while preserving the existing zero result when no
chord is available.
In `@src/engraving/dom/note.h`:
- Around line 574-575: Update the manual copy constructor Note::Note(const
Note&, bool) to copy both m_playbackStartOffset and m_playbackDurationOffset
from the source note, preserving their values when notes are cloned.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 3265a300-c3b4-48a4-9e3f-80ba5bba1bc7
📒 Files selected for processing (11)
src/engraving/api/v1/elements.hsrc/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/dom/property.cppsrc/engraving/dom/property.hsrc/engraving/playback/renderers/noterenderer.cppsrc/engraving/rw/read460/tread.cppsrc/engraving/rw/write/twrite.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/general/playback/internal/NoteExpandableBlank.qmlsrc/propertiespanel/qml/MuseScore/PropertiesPanel/general/playback/internal/noteplaybackmodel.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/general/playback/internal/noteplaybackmodel.h
| int Note::effectivePlaybackDuration() const | ||
| { | ||
| const Chord* ch = chord(); | ||
| if (!ch) { | ||
| return 0; | ||
| } | ||
| return ch->ticks().ticks() - playbackStartOffset() + playbackDurationOffset(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep duration independent from the start offset.
Line 4222 subtracts playbackStartOffset() from the duration. A note with a 480-tick duration and a 120-tick start offset plays for 360 ticks even when playbackDurationOffset is zero. This violates the independent duration contract.
The current formula can also produce a negative duration. The UI permits offsets from -1920 to 1920, and XML or plugins can supply larger values. Use the nominal chord duration plus playbackDurationOffset(), then clamp or skip nonpositive durations before rendering.
Proposed fix
- return ch->ticks().ticks() - playbackStartOffset() + playbackDurationOffset();
+ return std::max(0, ch->ticks().ticks() + playbackDurationOffset());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int Note::effectivePlaybackDuration() const | |
| { | |
| const Chord* ch = chord(); | |
| if (!ch) { | |
| return 0; | |
| } | |
| return ch->ticks().ticks() - playbackStartOffset() + playbackDurationOffset(); | |
| int Note::effectivePlaybackDuration() const | |
| { | |
| const Chord* ch = chord(); | |
| if (!ch) { | |
| return 0; | |
| } | |
| return std::max(0, ch->ticks().ticks() + playbackDurationOffset()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/engraving/dom/note.cpp` around lines 4216 - 4222, Update
Note::effectivePlaybackDuration() to compute duration from ch->ticks().ticks()
plus playbackDurationOffset(), without subtracting playbackStartOffset(). Ensure
nonpositive results are clamped or skipped before rendering, while preserving
the existing zero result when no chord is available.
| TieJumpPointList m_jumpPoints { this }; | ||
|
|
||
| int m_playbackStartOffset = 0; // offset in ticks to add to chord's tick for playback start | ||
| int m_playbackDurationOffset = 0; // offset in ticks to add to chord's ticks for playback duration |
There was a problem hiding this comment.
Good catch — fixed in 87d97ba. Note::Note(const Note&, bool) now also copies m_playbackStartOffset and m_playbackDurationOffset.
| if (!ch) { | ||
| return 0; | ||
| } | ||
| return std::max(1, ch->ticks().ticks() - playbackStartOffset() + playbackDurationOffset()); |
There was a problem hiding this comment.
The risk of a non-positive effective duration is real, but I'd like to keep the - playbackStartOffset() term rather than drop it.
The intent is for playbackStartOffset and playbackDurationOffset to move the effective start and end of playback independently: with the current formula, effectiveEnd = effectiveStart + effectiveDuration simplifies to chordTick + chordTicks + durationOffset — moving the start offset never shifts the end point, and vice versa. This is the anchor for an upcoming edit UI that lets users adjust the start and end of playback as two separate handles.
Dropping - playbackStartOffset() (as suggested) would make effectiveEnd also depend on startOffset, so moving the start would drag the whole playback window along with it rather than just resizing it — not the intended behavior.
I've addressed the actual risk you raised — a bad combination of independently-set offsets producing a non-positive duration — by clamping at the computation itself instead, in 87d97ba:
return std::max(1, ch->ticks().ticks() - playbackStartOffset() + playbackDurationOffset());
and similarly clamped effectivePlaybackStartTime() to std::max(0, ...). This keeps every caller (Properties panel, plugins, file loading) protected without changing the start/duration independence.
- Note::Note(const Note&, bool) did not copy m_playbackStartOffset / m_playbackDurationOffset, so cloning a note (copy-paste, duplication, linked parts) silently reset both offsets to 0. - effectivePlaybackStartTime()/effectivePlaybackDuration() had no lower bound. Since the two offsets are set independently (Properties panel spinboxes each range -1920..1920 with no cross-validation), an inconsistent combination could produce a negative effective start tick or a non-positive effective duration, both unguarded downstream in NoteRenderer. The duration formula intentionally keeps "- playbackStartOffset()" so that the effective end time (chordTick + chordTicks + durationOffset) does not depend on the start offset - this keeps start/duration independently adjustable, which upcoming UI work relies on. Clamping is applied at the computation itself rather than changing the formula, so every caller (Properties panel, plugins, future UI) is protected centrally.
87d97ba to
756caf5
Compare
timestampAndDurationFromStartAndDurationTicks() was called with a hardcoded tick-position offset of 0 instead of ctx.positionTickOffset, so every note (not just ones with a non-zero playback offset) played at first-playthrough timing on repeat/volta/D.C. passes. effectivePlaybackDuration() also independently recomputed from the raw, unclamped playbackStartOffset() instead of the same (possibly clamped) start effectivePlaybackStartTime() returns, so the two could disagree once the start clamp kicked in, making the note play longer than its clamped start implied.
This branch doesn't depend on the (not yet merged) MuseSampler velocity fix, so pin it to a commit that's actually on musescore/muse_framework:main - the check_muse_framework CI check rejects fork-only commits.
Note::effectivePlaybackStartTime()/effectivePlaybackDuration() derived the note's playback window from the chord's own tick()/ticks(), which is correct for an ordinary note but not for a grace note, an arpeggio note, or a note inside a repeated section: their actual playback window is computed separately (see GraceChordCtx::buildCtx and the repeat-aware positionTickOffset handling) and can differ substantially from the chord's notated tick/duration. Recomputing from chord tick/ticks discarded that and collapsed grace notes back onto their principal note's timing, breaking 16 unit tests. Apply playbackStartOffset()/playbackDurationOffset() directly in NoteRenderer::render() instead, on top of the RenderingContext's already-correct nominal tick range, and only when an offset is actually set (so unedited notes take the exact same code path as before this feature existed).
Continuation-line indentation was off by one space, flagged by the codestyle CI check.
|
Note that there is already I would be particularly interested if a note can ever have more than one entry in |
|
Git hint: it looks like this PR contains a merge commit ("Merge remote-tracking branch 'origin/main' into feature/note-offsets". It would be good to rebase it ( |
Note::playbackStartOffset()/playbackDurationOffset() were only wired into read460/tread.cpp's XML reader, but files saved by this app go through read500 (the current format version, dispatched by RWRegister::reader() for version >= 500) - read460 is only ever used to open older 4.60-4.99 files, which can never contain this property in the first place since it didn't exist yet. The property was written correctly (twrite.cpp) but silently dropped on reload because the reader that actually matters never looked for the tag, resetting both values to 0 every time a file was saved and reopened. Moved the read hooks to read500/tread.cpp, removed the dead ones from read460, and added a save/reload regression test (writeReadElement round-trip) to Engraving_NoteTests.note.
6 participants
Resolves: #34545
Adds two new per-note properties (
playbackStartOffset,playbackDurationOffset) that let users nudge a note's audio playback timing independently of its notated position and duration. Exposed in the Properties panel's Play section and on the plugins API. See #34545 for the full motivation and scope.Note: no unit test/vtest yet for this playback-timing change — happy to add one if reviewers think it's warranted.