New dynamics properties for voicing and stave positioning #22662

mike-spa · 2024-05-02T15:19:26Z

Resolves: #22175

The way that the "All voices" option is presented in the properties is just a placeholder. It functions correctly, but it looks different from the design. Given that implementing the required button+dropdown will take some more work, @cbjeukendrup and I have decided to do it later. Meanwhile, @avvvvve and @oktophonie please have fun with this, looking forward to your feedback 😄

avvvvve · 2024-05-02T20:50:34Z

@mike-spa First off, this is sooooo satisfying! Bravo. Testing it a bit just now and found a few small things:

I assume the "All..." labels aren't the same as in the spec for now just to fit them inside those buttons? So you don't have to go looking, they should be:
- All voices on instrument
- All voices on this staff only
When one of the "All..." voice options is selected, none of the voice buttons in the note input toolbar should be accented. Currently, voice 1 gets accented when you do this.
In Style > Dynamics & hairpins, when "Center on grand staff instruments automatically" is turned off, any dynamics with "Center between staves" set to Auto should no longer be centered. In other words, Auto should behave as if set to "Off" instead of "On".
When applying dynamics to the bottom staff, sometimes the vertical offset below the staff changes after changing from voice 1 or 3 to one of the "All..."s or voice 2 or 4 (see video)

dynamics-positioning-bottom.mov

mike-spa · 2024-05-03T12:04:44Z

@avvvvve

Yes the button labels are just as placeholders as the buttons themselves (the full labels don't fit). We'll fix them :)
Fixed ✅
Fixed ✅
Fixed ✅

cbjeukendrup · 2024-05-06T01:07:41Z

@mike-spa I created a PR to your repo to implement the design for the "All" button: mike-spa#1
You can merge that PR, and then the commits will appear here immediately (also make sure to pull them to your own computer). After doing that, the commits are on your branch and you can continue adding new commits or amending previous ones (including my ones), just as if the commits were created by you.

avvvvve · 2024-05-06T15:29:27Z

For @cbjeukendrup:
Nice work on the button! We should make the context menu show up if you right-click on the button too.
And one suggestion for an interaction feature, if it's not too much to implement...
Right now, the context menu only shows up after clicking and releasing the mouse on the "All" button. It would be cool if it appeared as soon as you click down, then if you kept holding, you could drag the mouse over one of the context menu options and then release to select it all in one click. But that's definitely extra credit.

Also, would it be possible to make this button a reusable component i.e. one that shows up in "UI Gallery" for future use? :)

For @mike-spa, cc @bkunda:
When copying and pasting a dynamic, its "Apply to voice" is getting set based on what is selected in the new Preference in Note Input, "When entered, dynamics and hairpins should affect:"

If it's set to "All voices on the instrument", then I copy a dynamic set to voice 3 and paste it somewhere else, it gets set to "All voices" and the "Position" and "Center between staves" get reset to "Auto", even if I had manually chosen other options.

I think dynamics should retain their properties when copied and pasted instead, regardless of what you've chosen in preferences.

cbjeukendrup · 2024-05-06T22:09:56Z

We should make the context menu show up if you right-click on the button too.

Done!

Also, would it be possible to make this button a reusable component i.e. one that shows up in "UI Gallery" for future use?

It is already reusable; it basically consists of three components: the FlatButton, the menu indicator triangle that is also seen in the note input bar, and the menu. But I've added the combination of the three to the DevTools page, because why not.

Right now, the context menu only shows up after clicking and releasing the mouse on the "All" button. It would be cool if it appeared as soon as you click down, then if you kept holding, you could drag the mouse over one of the context menu options and then release to select it all in one click. But that's definitely extra credit.

Unfortunately, that is difficult. Making the menu open on press (instead of release) is doable, but the problem is that you can't receive release events in the menu items if the initial press event was sent to anything other than the menu item (namely to the button). Also, when we implement this, we should implement it for all menu buttons throughout the app in one go. Feel free to open a new feature request for that.

mike-spa · 2024-05-09T09:41:52Z

I think dynamics should retain their properties when copied and pasted instead, regardless of what you've chosen in preferences.

@avvvvve Done! ✅

avvvvve · 2024-05-13T18:15:14Z

@mike-spa Looking good! The addition of the voice preference is great. I'd suggest a small change to the label:
"Place above staff on vocal instruments"
I know 'vocal instruments' sounds like not a real thing, but they are technically instruments in MSS.

A couple other style dialog things:

Can we make the reset button in the "Default positions..." section reset all three settings back to the default?
When 'Position' is set to 'Above', can we disable 'Place above staff on vocal instruments' (40% opacity) since it will have no bearing on the score?
The "Center on grand staff instruments automatically" checkbox currently centers dynamics automatically on any instrument with at least two staves. The original intent was that it would only do this for instruments defined as having more than one staff (i.e. piano/organ). So currently, if you put a dynamic on a violin part with two staves, it gets centered. Can we make it only apply to the correct instruments? Alternatively, we could make this a dropdown and provide three options: Never center automatically, Center on grand staff instruments, Center on any instrument with more than 1 staff @oktophonie thoughts on whether the latter is something we should even allow?

And one engraving thing:
4. When copying a dynamic with an individual voice selection (say, voice 3) and pasting onto a different note (say, voice 2), the dynamic takes the color of the voice it was pasted on. The "Apply to voice" shows the correct voice, from the copied dynamic, so the color just needs to be correct.

mike-spa · 2024-05-14T12:27:25Z

@avvvvve all done!

avvvvve · 2024-05-15T14:31:40Z

@mike-spa

Re: #1 above, the reset button only becomes active if you change the position setting. It should become active if you uncheck either of the other two options too.

The rest below we can address in follow-ups to this PR after merging!

I noticed some slightly strange behavior with the actual placement of centered dynamics. Check the video below. I added a ridiculously tall spacer to the staff to illustrate that the dynamic moves around when a note's boundaries extend beyond one of the staves even if it's still very far away from the dynamic.

Dynamics on the very first note of the system appear slightly misaligned with the rest.

mp generally feels vertically misaligned with mf. Personally, I'd expect their baselines to be the same. I don't think other in-score elements are causing this offset.

dynamics-centering-issues.mov

oktophonie · 2024-05-16T08:57:23Z

The behaviour shown in the video is as intended for now - pending a lot more testing in real-world situations; more nuance and possibly config options will be required.

We centre dynamics in the available vertical space (not between the staves only), and also do not align the baselines since p, mp etc (dynamics with descenders but no ascenders) then appear too low in isolation.

When dynamics are adjacent or close together then yes, the baselines should probably align (in most situations), but that matter is to be addressed in Phase II.

RomanPudashkin · 2024-05-24T12:11:46Z

src/engraving/dom/engravingitem.cpp

+
+void EngravingItem::setInitialTrackAndVoiceApplication(track_idx_t track)
+{
+    if (track == muse::nidx) {


Shouldn't this be IF_ASSERT_FAILED instead?

RomanPudashkin · 2024-05-24T12:13:31Z

src/engraving/dom/engravingitem.cpp

+    if (track == muse::nidx) {
+        track = m_track;
+    }
+    if (MScore::dynamicsApplyToAllVoices) {


I'm not a big fan of these global flags... Can we add it to the engraving configuration instead?

RomanPudashkin · 2024-05-24T12:21:46Z

src/engraving/dom/engravingitem.cpp

+        setProperty(Pid::APPLY_TO_VOICE, VoiceApplication::ALL_VOICE_IN_INSTRUMENT);
+    } else {
+        setTrack(track);
+        setProperty(Pid::APPLY_TO_VOICE, static_cast<VoiceApplication>(track2voice(track)));


This looks a bit odd... As if a dynamic can be added to one voice, but affect another. Maybe VoiceApplication should only have 3 values:
ALL_VOICE_IN_INSTRUMENT
ALL_VOICE_IN_STAFF
CURRENT_VOICE_ONLY

What do you think?

RomanPudashkin · 2024-05-24T12:27:47Z

src/engraving/rendering/dev/systemlayout.cpp

+
+    const Staff* thisStaff = element->staff();
+    const std::vector<Staff*>& partStaves = part->staves();
+    if (thisStaff == partStaves.front() && element->placeAbove() || thisStaff == partStaves.back() && element->placeBelow()) {


It's better to check if partStaves is empty

RomanPudashkin · 2024-05-24T12:29:29Z

src/engraving/rendering/dev/systemlayout.cpp

+    }
+
+    staff_idx_t thisIdx = thisStaff->idx();
+    staff_idx_t nextIdx = element->placeAbove() ? thisIdx - 1 : thisIdx + 1;


thisIdx - 1

It is better to check that thisIdx > 0

Same for centerElementBetweenStaves

RomanPudashkin · 2024-05-24T12:30:38Z

src/engraving/rendering/dev/systemlayout.cpp

+    staff_idx_t nextIdx = element->placeAbove() ? thisIdx - 1 : thisIdx + 1;
+
+    const SysStaff* thisSystemStaff = system->staff(thisIdx);
+    const SysStaff* nextSystemStaff = system->staff(nextIdx);


These pointers should also be checked for null

mike-spa requested review from oktophonie and avvvvve May 2, 2024 15:19

avvvvve mentioned this pull request May 2, 2024

Fix #22062: Add preferences for missing engraving UI colors #22361

Open

8 tasks

mike-spa force-pushed the newDynamicsProperties branch from e48de7c to 974f5fb Compare May 3, 2024 12:00

mike-spa force-pushed the newDynamicsProperties branch 2 times, most recently from a1c4776 to ca42264 Compare May 6, 2024 07:57

mike-spa force-pushed the newDynamicsProperties branch from 43d21b4 to 245aa9e Compare May 13, 2024 09:22

mike-spa force-pushed the newDynamicsProperties branch from 47e560a to 538cb3c Compare May 14, 2024 07:39

mike-spa force-pushed the newDynamicsProperties branch from 2331b19 to 28e0a2c Compare May 16, 2024 08:44

mike-spa mentioned this pull request May 16, 2024

Improve key signature shape and distances #22853

Merged

oktophonie approved these changes May 16, 2024

View reviewed changes

oktophonie added the vtests This PR produces approved changes to vtest results label May 16, 2024

avvvvve approved these changes May 16, 2024

View reviewed changes

mike-spa requested a review from RomanPudashkin May 17, 2024 13:41

avvvvve mentioned this pull request May 17, 2024

Implement "Flip direction" shortcut action for new dynamics & hairpins #22886

Open

mike-spa force-pushed the newDynamicsProperties branch 4 times, most recently from f30d202 to 67df2c7 Compare May 23, 2024 07:18

Introduce new options for dynamics voice and placement

0d998bb

mike-spa force-pushed the newDynamicsProperties branch from 67df2c7 to 0d998bb Compare May 23, 2024 15:35

RomanPudashkin reviewed May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New dynamics properties for voicing and stave positioning #22662

New dynamics properties for voicing and stave positioning #22662

mike-spa commented May 2, 2024

avvvvve commented May 2, 2024

mike-spa commented May 3, 2024

cbjeukendrup commented May 6, 2024

avvvvve commented May 6, 2024

cbjeukendrup commented May 6, 2024

mike-spa commented May 9, 2024

avvvvve commented May 13, 2024

mike-spa commented May 14, 2024

avvvvve commented May 15, 2024

oktophonie commented May 16, 2024 •

edited

RomanPudashkin May 24, 2024

RomanPudashkin May 24, 2024

RomanPudashkin May 24, 2024

RomanPudashkin May 24, 2024

RomanPudashkin May 24, 2024

RomanPudashkin May 24, 2024

RomanPudashkin May 24, 2024

New dynamics properties for voicing and stave positioning #22662

Are you sure you want to change the base?

New dynamics properties for voicing and stave positioning #22662

Conversation

mike-spa commented May 2, 2024

avvvvve commented May 2, 2024

mike-spa commented May 3, 2024

cbjeukendrup commented May 6, 2024

avvvvve commented May 6, 2024

cbjeukendrup commented May 6, 2024

mike-spa commented May 9, 2024

avvvvve commented May 13, 2024

mike-spa commented May 14, 2024

avvvvve commented May 15, 2024

oktophonie commented May 16, 2024 • edited

RomanPudashkin May 24, 2024

Choose a reason for hiding this comment

RomanPudashkin May 24, 2024

Choose a reason for hiding this comment

RomanPudashkin May 24, 2024

Choose a reason for hiding this comment

RomanPudashkin May 24, 2024

Choose a reason for hiding this comment

RomanPudashkin May 24, 2024

Choose a reason for hiding this comment

RomanPudashkin May 24, 2024

Choose a reason for hiding this comment

RomanPudashkin May 24, 2024

Choose a reason for hiding this comment

oktophonie commented May 16, 2024 •

edited