Add drag-handle overlay to edit per-note velocities - #2
Open
tharos-devs wants to merge 30 commits into
Open
Conversation
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
…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.
…ocity-drag-handles
…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.
…ocity-drag-handles
…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.
…ocity-drag-handles
…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.
…ocity-drag-handles
…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.
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
1 participant
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.
Summary
Stacked on
This PR is the third in a stack and targets
feature/note-offset-drag-handles(this fork's #1) rather thanmain, because the note-velocity overlay controller shares its core architecture with the note-offset overlay controller - notably theSysStaffKeymap-key pattern, the overlay-reuse-in-place optimization, and the View mode (Page/Continuous) crash fix that came out of reviewing that code. Merge order:feature/note-offsets, upstream) - data modelfeature/note-offset-drag-handles) - offset drag-handle UI, including the shared overlay-controller fixes this PR builds onfeature/note-velocity-drag-handles) - velocity drag-handle UITest plan