Add drag-handle overlay to edit per-note velocities - #34582
Draft
tharos-devs wants to merge 56 commits into
Draft
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::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.
Adds an on-canvas alternative to the Properties panel spinboxes for editing
the playbackStartOffset/playbackDurationOffset introduced in
"Add per-note playback start/duration offset properties": a toggleable
overlay ("Note offsets" toolbar action, alongside Automation) draws a small
rounded rectangle above each note, anchored on the note's own position.
Dragging the rectangle's left/right edge adjusts the start/duration offset
respectively, with a live preview during the drag and a single undo/redo
step on release. Dragging a note that is part of a multi-note selection
applies the same tick delta to every selected note. The notation dims while
the mode is active, matching the existing Automation overlay's behavior.
New files:
- INotationNoteOffsets / NotationNoteOffsets: edit-mode toggle state,
mirroring INotationAutomation.
- NotationNoteOffsetController: builds/positions one overlay per staff from
the actual note layout (not a fixed lane), converts drag input to ticks
via segment interpolation, and commits offset changes through the
existing undoChangeProperty/Pid machinery (no new UndoableCommand needed).
- NoteOffsetOverlay: QQuickPaintedItem drawing the rectangles/handles and
doing its own hit-testing, independent of the Grip/NotationInteraction
edit path - the same pattern PolylinePlot uses for automation.
- segmentcanvasinterpolation.{h,cpp}: canvasX<->tick interpolation shared
between the automation and note-offset controllers (previously
duplicated).
Wiring follows the existing "toggle-automation" action end to end
(notationuiactions.cpp, notationactioncontroller.cpp,
notationcommandsregister.cpp, notationcommandsstate.cpp,
notationtoolbarmodel.cpp) and reuses AbstractNotationPaintView's existing
per-view-matrix redraw/dim hooks.
Live-dragging a style value (e.g. "Staff space (sp)" in Page Settings) relayouts the score without going through changesChannel(), so the overlay's cached note positions went stale and stopped tracking the rescaled notation in real time.
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.
- Compare SysStaffKey's System pointer by address only, never dereferencing it - a stale key still held from a previous rebuild had a dangling System* once Page/Continuous view mode switching destroyed and recreated every System, crashing on MeasureBase::index(). - Skip tied-continuation notes when building overlay handles: their own playback offset is ignored by NoteRenderer::shouldRender() in most cases, so a handle there could never actually do anything. - Disconnect from a document's viewModeChanged/styleChanged the moment a different document becomes current, instead of leaking one subscription per document ever opened in the session. - Don't delete an overlay that's mid-drag (holding the mouse grab) when a rebuild is triggered; defer until the drag finishes. - Handle mouseUngrabEvent so a stolen mouse grab mid-drag doesn't leave the overlay stuck thinking a drag is still in progress. - Consolidate the three parallel per-staff maps (overlay/notes/band rect) into one map to a per-staff struct, and reuse an existing overlay item in place instead of destroying and recreating every overlay on every rebuild. - Mutate a single rect in place during a drag instead of copying the whole staff's rect vector out and back on every mouse-move.
Mirrors the note-offset drag-handle overlay's architecture with a NoteVelocityOverlay/NotationNoteVelocityController pair: a draggable vertical bar per note maps its position between the staff's bottom line (velocity 0) and where a 5th line would sit if the staff had one (velocity 127), so it works the same way on non-5-line staves (e.g. 1-line percussion) as on a standard staff. Chord notes stack their bars by pitch (lowest-pitched note painted frontmost) instead of offsetting horizontally, matching how DAW piano rolls stack overlapping velocity lanes. Dragging one note in a multi-note selection applies the same delta to every selected note, including ones hidden behind others in the same chord's stack - only what's selected moves. Unedited notes display the velocity their dynamics marking/hairpin context alone would produce at that exact tick (via new PlaybackModel::appliableDynamicLevel(), exposed through INotationPlayback and converted with a shared muse::mpe::dynamicLevelToVelocityRatio() curve) rather than a flat default, so nudging one starts from a musically coherent baseline. VeloType::OFFSET_VAL notes (a percentage nudge on the dynamics baseline, distinct from an absolute VeloType::USER_VAL override) are read correctly rather than treated as an absolute value. Bars are colored to show state at a glance: green for the dynamics-derived default, orange for a user-modified velocity, blue when selected. Requires the companion musescore/muse_framework fix that forwards per-note velocity overrides to MuseSampler's main playback stream (without it, per-note velocity is audible via the legacy/FluidSynth sound library but not via MuseSampler-hosted MuseSounds).
Picks up the fix for per-note velocity overrides not reaching MuseSampler's main playback stream, so edits made with the note velocity drag-handle overlay are actually audible.
…ocity-drag-handles
Continuation-line indentation was off by one space, flagged by the codestyle CI check.
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.
…ocity-drag-handles
The merge from feature/note-offset-drag-handles reset the submodule pointer back to upstream main; this branch actually needs the MuseSampler velocity fix (musescore/muse_framework#221), so re-pin it to fix/musesampler-note-velocity's tip. Also fixes an extra blank line flagged by the codestyle CI check in NotationNoteVelocityController.
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).
…ocity-drag-handles
Continuation-line indentation was off by one space, flagged by the codestyle CI check.
…ocity-drag-handles
… handle Displays the velocity value (0-127) in a small chip next to the bar's top edge, updated in real time as the bar is dragged, so the exact new value is visible without guessing from bar height alone - matches the convention used by Dorico's own velocity lane. Only the actively dragged bar shows the readout, to keep the staff uncluttered the rest of the time. The chip's colors are picked from the score's current background color (INotationConfiguration::backgroundColor(), already theme-aware: light/dark/high-contrast paper, or a user-customized color) rather than hardcoded, so it stays legible against light or dark paper alike instead of only working for the default white background.
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.
…ocity-drag-handles
createOverlayForStaff() anchored the nominal (zero-offset) right edge on "the next ChordRest segment", but that segment is shared across every voice/track on the staff. If a different voice had a shorter simultaneous note (e.g. an eighth note under a quarter note), its segment became the "next" one for every voice, cutting the longer note's rectangle down to the shorter note's end tick instead of its own. Anchor the right edge on this chord's own end tick via canvasXFromTick instead, which already correctly interpolates/snaps to the real segment at that tick regardless of which voice created it.
…ocity-drag-handles
Dragging a note's velocity only updated that note's own USER_VELOCITY/ VELO_TYPE properties. A tied-continuation note is usually skipped entirely by playback rendering, but in some configurations (tremolo across the tie, partial ties across a repeat, multi-note articulations, a trill ending on the tie's start chord) it is still rendered as its own independent event using its own, never-touched velocity, causing an audible volume jump. Mirror the dragged velocity onto the whole forward tie chain so every note in it stays in sync.
VELO_TYPE was read from XML on load but never included in the note property write list, so it was silently dropped on save. Harmless for the common USER_VAL case (the in-class default happens to match), but loses an OFFSET_VAL (percentage nudge) override on save/reload.
Match the note-velocity overlay's convention: green when both playback offsets are at their default (0), orange as soon as either the start or duration offset has been user-modified, blue when the note is selected (taking priority over the modified color, same as velocity). Both offsets already round-trip correctly through save/reload, so the color - computed live from those persisted values on every rebuild - does too.
…ocity-drag-handles
Mirrors the existing Automation context-menu entry: only appears when note-offset edit mode is enabled (NotationContextMenuModel::loadItems), and resets both playback offsets to 0 for every selected note via the undo stack, so notes revert to the system-default (unmodified) offset.
…ocity-drag-handles
Mirrors the existing "Reset note offsets" entry: only appears when note-velocity edit mode is enabled (NotationContextMenuModel::loadItems), and resets USER_VELOCITY to 0 for every selected note via the undo stack, so notes revert to the system-default (dynamics-derived) velocity - matching the userModified check already used by the velocity overlay's own color-coding.
Bars are painted back-to-front by pitch (lowest note frontmost) to mimic a piano-roll velocity lane, but that meant selecting a non-frontmost chord note left its bar visible only where a taller neighbor didn't cover it - and often not clickable at all, since hit-testing only exposed the portion of a back bar poking out above the front one. Selected bars are now redrawn on top of every other bar in their column, and hit-tested first, ignoring stacking-order occlusion, so picking a note (however it's selected) always makes its velocity bar fully visible and draggable.
The edge-drag handle nubs were always a fixed gray regardless of whether the pill body was showing default/modified/selected color, making them visually disconnected from the rectangle they belong to. Handles now use a darker shade of the same state color (green/orange/ blue) as the pill body they're attached to.
…ocity-drag-handles
Previously a tied-into note (note->tieBack()) got no overlay entry at all, so the rectangle stopped at the first note's own duration and left a visual gap over the rest of the tied chain, even though playback already correctly sounds through the whole chain. NoteEntry now tracks headNote/tailNote/anchorNote instead of a single note. A chain-head's rectangle extends to the tail's own end tick (walked via Note::lastTiedNote(), playback-accurate to match NoteRenderer::renderNormalTie()), with the right/duration handle shown only if the tail resolves within the same System. A new branch handles a tied-continuation note whose *predecessor* lives in a different System: it gets its own continuation fragment starting at that System's own left edge, which is what actually closes the gap for a tie crossing a system or page break - each System still only knows its own coordinate space, so a single rectangle can't literally span two of them, but a fragment per System reads as continuous. Only the chain's first note owns the left/start handle and only its last note owns the right/duration handle; everything in between has neither, matching the fact that dragging duration from the middle of a tie chain (or start from past its own end) has no sensible meaning. Property writes always target the chain's head note regardless of which fragment/handle was actually dragged, since only the head's own offset is ever honored during playback. Also added two one-sided drag clamps, both only active for a real tie (tailNote != headNote): the duration handle can't shrink the total span to end before the tail note's own start, and the start handle can't push the start past the head note's own end.
The "Start offset"/"Duration offset" spinboxes read and wrote Pid::PLAYBACK_START_OFFSET/DURATION_OFFSET on the exact selected note with no tie-awareness at all: selecting a tied-continuation note let you type a value that was silently ignored during playback (only the chain's head note's own offset is ever honored) and, now that the on-canvas overlay is tie-aware too, disagreed with what it shows for that same chain. NotePlaybackModel::headNoteElements() now remaps each selected note to its firstTiedNote(false) for both directions, via a custom onPropertyChangedCallBack passed to buildPropertyItem - ties into an extension point PropertiesPanelAbstractModel already supported, no base-class changes needed. A note that is neither its chain's head nor its tail (a middle link in a 3+-note chain) owns no handle at all in the overlay, so it's dropped from the list entirely rather than redirected, leaving both spinboxes disabled for it instead of quietly editing a value it has no visual handle for. Also fixed a separate, pre-existing refresh bug found while testing this: committing a property change (e.g. releasing a drag) never live-updated these spinboxes without leaving and re-entering the Properties tab. GeneralSettingsModel::onNotationChanged() only ever reloaded its own four Pids and never forwarded the notification down into m_playbackProxyModel (unlike onCurrentNotationChanged() right below it, which does forward to both nested models), and NotePlaybackModel never overrode onNotationChanged() to react even if it had been forwarded. Both gaps are now closed for NotePlaybackModel; PlaybackProxyModel's other sibling models (arpeggio/fermata/breath/ glissando/gradual tempo change) have the same gap and are left as a known, separate follow-up.
…ocity-drag-handles
The "Velocity" spinbox hardcoded a flat 64 whenever a note had no explicit userVelocity() (0), completely ignoring any dynamic (piano, forte...) actually in effect at that note - unlike the on-canvas velocity-bar overlay, which already falls back to the real dynamics-derived value (NotationNoteVelocityController:: displayedVelocity()/contextVelocity(), via INotationPlayback::appliableDynamicLevel() + muse::mpe::dynamicLevelToVelocityRatio()). A forte note that had never been dragged showed a bar at ~96 but a spinbox stuck at 64. NotePlaybackModel::effectiveVelocity() mirrors that same fallback so both surfaces agree. loadPropertyItem()'s convertElementPropertyValueFunc only ever sees the already-read value, not the element it came from - not enough to compute a per-note contextual fallback - so the velocity spinbox is now loaded through a dedicated loadVelocityProperty() instead of the generic path. Also fixes a related, more subtle bug found while testing this: since an unset note's displayed value is now a computed fallback rather than a fixed constant, dragging such a note to a value that happens to match its own displayed fallback (e.g. dragging a forte note to exactly 96) left the spinbox showing the same number both before and after, even though the note genuinely went from "following the dynamic" to "explicit user velocity" underneath - PropertyItem:: updateCurrentValue() only notifies when the displayed value itself changes, which can't tell those two states apart when they coincide numerically. Gave updateCurrentValue() an optional forceNotify parameter (defaults to false, so every other call site is unaffected) and pass it whenever isModified is about to flip, so the spinbox never silently disagrees with the (always-correct) isModified-driven color in that situation.
Contributor
|
As I said on Discord too: when I was still part of the team, it was tentatively decided to remove the velocity property completely, once dynamics automation is in place. It is namely unclear how velocity should interact with dynamics: velocity is namely a representation of dynamics, used to communicate dynamics to FluidSynth, VST, and MIDI devices. But for MuseSounds, I believe it's meaningless, and mapped to the same parameter as dynamics. The property existed as a way to override dynamics per note, but now there is a better way for that, namely dynamics automation. (One might want to customise how exactly the dynamics values are mapped to velocity for specific VST plugins; that sort of parameter mapping is indeed desirable, but it is a separate feature.) |
Author
|
Thanks for the added context. |
Qt Quick's per-item cursor arbitration follows whichever topmost item has ever called setCursor(), independent of hover event accept/ignore. NoteOffsetOverlay unconditionally declares a cursor on every hover move, so its east-west edge cursor was winning even where a velocity bar - painted on top, and already capturing mouse presses there - visually covered one of its drag handles. NoteVelocityOverlay now claims/releases its own cursor declaratively so hover matches what a click there actually does.
Clicking anywhere on a velocity bar and dragging used to snap the velocity to whatever absolute value the click's Y position corresponded to, which felt wrong for a click that landed mid-bar rather than exactly on its top edge. The whole bar now acts as a drag handle: barDragged reports the mouse's own displacement since the press instead of an absolute position, and the controller nudges the note's pre-drag velocity by that amount rather than computing an absolute target.
Code review of the two preceding commits found: the drag delta compared a press-time position already normalized by height() against a move/release-time position normalized by a height() read later - if the overlay's height changes mid-drag (window resize, view zoom/pan), the two ends of the subtraction used different scales. Now stores the raw pixel press position and divides once by the current height(). Also made hoverMoveEvent skip redundant setCursor()/unsetCursor() calls when the hovered/not-hovered state hasn't changed, matching the cached-state pattern NoteOffsetOverlay::updateCursor() already uses.
Draw the offset rectangle body with plain square corners instead of a fully-rounded pill shape, per user preference.
Lets the user hear the effect of a velocity edit before committing it, mirroring the existing pitch-drag audition pattern via IPlaybackController::playNotes() with an ad-hoc velocityOverride on a throwaway NoteVal - the real Note is never touched until the drag completes. Also plays once on a plain click with no movement, since the overlay otherwise swallows the click MuseScore would normally give audible feedback for on note selection. Throttled to at most one retrigger per 200ms during the drag (untriggered mouse-move events fire far more often than that, which sounded like a machine gun without a minimum interval), always auditions the exact value that ends up committed on release regardless of the throttle window, copies headGroup so cross/diamond noteheads audition with their own articulation, skips entirely while real transport playback is running so it doesn't fight the transport for the track, and resets its throttle state if the drag's mouse grab is stolen mid-gesture (e.g. by a popup) rather than only on a normal release.
With both note-offset and note-velocity edit modes active, a velocity bar visually covering an offset edge handle also always won clicks and hover there, since it's stacked on top - making that handle both invisible and unreachable whenever a bar happened to cover it. A standalone Cmd (macOS) / Ctrl (Windows, Linux) tap - pressed and released with nothing else happening in between - now swaps which of the two overlay containers paints, and is hit-tested, on top of the other, persisting until tapped again. Committing only on release, and only if nothing else used the modifier in the meantime, keeps this from firing as a side effect of every other Cmd/Ctrl interaction (copy, undo, Ctrl-click to extend a selection, Ctrl-wheel zoom, passive hover in note-input mode, ...): a single general check in event() cancels the pending toggle for any QInputEvent that carries the modifier and isn't the Control key's own press/release, rather than reproducing that check in every individual handler. Also fixes a pre-existing gap surfaced while reviewing this: a velocity bar drag interrupted by its mouse grab being stolen mid-gesture (e.g. a popup opening) only reset the audition throttle, leaving the bar's live-preview height on screen indefinitely instead of snapping back to the note's actual velocity.
A plain click (press+release without moving past a small threshold) now sets the note's velocity directly to whatever value the clicked position corresponds to, instead of being a no-op. Pressing and dragging past that threshold keeps today's existing relative-nudge behavior unchanged - the two are distinguished by tracking whether the mouse ever moved past CLICK_MOVE_THRESHOLD_PX before release. Implemented without a second code path: a click's delta is expressed as (clicked position - the bar's current top edge), which resolves through the same linear canvasY-to-velocity mapping used for drags to exactly the velocity at the clicked position, regardless of what that delta happens to be measured from.
…vert A genuinely zero-delta gesture (a plain click landing back on the bar's own current position, or a drag that ends up where it started) was still run through the [MIN_DRAGGABLE_VELOCITY, MAX_DRAGGABLE_VELOCITY] clamp, silently flooring a note whose dynamics-derived velocity is legitimately 0 (e.g. under ppppppppp) to 1 and pinning it to an explicit VeloType::USER_VAL it never asked for - same issue for any other co-selected note whose own displayed velocity was 0. Now skips the clamp (and the property write entirely, for any note whose target value already matches what's displayed) whenever the actual delta is zero, and skips the whole undo entry if nothing ends up changing. Also, onDragCancelled() (mouse grab stolen mid-drag, e.g. by a popup) only reverted the one bar that owned the grab - if the dragged note was part of a multi-selection, every other selected note's bar (and their tie chains) had been live-previewed too and stayed stuck at that uncommitted height indefinitely. Now reverts the whole affected set, mirroring onBarDragged()'s own selection/tie-chain expansion.
Which overlay's cursor is displayed over an overlap is only re-evaluated by Qt on the next hover event (see the cursor-priority handling in notevelocityoverlay.cpp/noteoffsetoverlay.cpp). Without this, swapping which of the note-offset/note-velocity overlays is on top left a stationary mouse showing the previous top overlay's cursor until it happened to move even a pixel, even though a click there would already route to the new top overlay - a visible mismatch between the cursor and what a click would actually do. Synthesizes a button-less mouse-move at the current pointer position right after the swap, forcing Qt Quick's normal hover-delivery path to run again immediately, the same as a real (zero-distance) move would.
effectiveVelocity() always treated a nonzero userVelocity() as an absolute value, but for VeloType::OFFSET_VAL notes it's actually a percentage nudge on top of the dynamics-derived context velocity (see Note::customizeVelocity()) - the spinbox showed a raw, meaningless number instead of either the percentage or the actual playing velocity, disagreeing with the on-canvas velocity-bar overlay this was meant to mirror (NotationNoteVelocityController:: displayedVelocity()). Now shares the same VeloType-aware logic, factored into a new contextVelocity() helper mirroring the controller's own. Editing the spinbox had the matching write-side bug: it went through the default single-Pid write path, which never touched VELO_TYPE, so typing an absolute value into an OFFSET_VAL note's velocity field silently got reinterpreted as a percentage the next time it was read. A dedicated callback now forces VELO_TYPE to USER_VAL first, matching what dragging the on-canvas bar already does.
…ocity-drag-handles # Conflicts: # muse
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft — opened for visibility only, not meant to be merged yet. This follows the same on-canvas drag-handle overlay pattern as #34569 (note offsets), applied to per-note velocity.
Summary
ExpressionContext::velocityOverridewas already forwarded to FluidSynth/VST, but silently dropped in MuseSampler's main playback stream.Stack
This branch builds directly on #34569 (
feature/note-offset-drag-handles), which itself depends on #34546 (feature/note-offsets, data model). Its diff currently includes those commits too, sincemaindoesn't have them yet - it will shrink automatically as those merge. Merge order:Test plan