musescore / MuseScore

MuseScore is an open source and free music notation software. For support, contribution, bug reports, visit MuseScore.org. Fork and make pull requests!
https://musescore.org
Other
11.85k stars 2.57k forks source link

[MU4 Issue] Velocity changes from MS3 aren't converted to the new system #14768

Closed erinic04 closed 1 week ago

erinic04 commented 1 year ago

Describe the bug Whenever you open a score in MS4 that had offset velocity changes on certain notes, the values in the velocity box stay the same, even though they mean entirely different things now than they used to. For example, a value of 16 in MS3 increases the volume by a dynamic level, but in MS4, that decreases it by 3 (I think).

To Reproduce Steps to reproduce the behavior:

  1. Create score in MS3
  2. Add velocity of 16 to note
  3. Open the score in MS4
  4. Hear that the note is significantly quieter than the written dynamic now, rather than being a bit louder than the written dynamic.

Expected behavior The values should ideally be converted to the equivalent values in the new system, but if that is too unattainable, they should at least reset to use the written dynamics instead.

Platform information

Additional context Took me a while to figure out why the certain staff was just not really playing the notes

LuisLiszt commented 1 year ago

@Tantacrul to add to this issue: Changing the velocity setting of an individual note will have no effect with MuseSounds regardless of being an import by MS3 or written in MS4...

exegetor commented 1 year ago

I understand that the velocity system is WIP, and we don't know yet what coming down the road... but in the meantime, it's important that files imported from MS3 translate note velocities correctly to MS4, whether it be the current stop-gap system, or the eventual "final" system.

Jojo-Schmitz commented 8 months ago

What about the velocity change on hairpins? Those too get lost.

Jojo-Schmitz commented 3 months ago

Came up again in https://musescore.org/en/node/362850

mercuree commented 2 months ago

Came up again https://musescore.org/en/node/363812

Jojo-Schmitz commented 1 month ago

And once again in https://musescore.org/en/node/364287

scorster commented 1 month ago

Jojo wrote > And once again in https://musescore.org/en/node/364287

The .org description specifically reports that MS3 offset values are imported as Absolute values ... rather than being summed with whatever MS4 wants as its default absolute value. This point of view and its urgency is underscored by HildeK's .org post.

A pseudo code fix seems as simple as:

    MS4_absolute_velocity == MS4_absolute_velocity_default + imported_Offset

... rather than the assignment that's apparently happening:

   MS4_absolute_velocity == imported_Offset

... but it seems that the MS4 sound engine isn't ready to pair with velocity, so then that's a larger conumdrum.

MuseScore 4 ... for me, just too many gotcha's, missing features and regressions

Playback bugs like this are the main reasons I don't use MS4 ... except for testing and commentary ... also to tour the UI and try out new features, like the amazing Partial Capo.

MS4 was planned and launched as a great improvement to engraving, UI and playback.

If "playback improvement" referred only to the quality of new libraries, that doesn't take us very far— particularly if we lose control of basic matters like velocity and pairing those with dynamics. Additionally the LEN property is ignored on playback, and the Piano Roll Editor is MIA—and those too provide ways that I impart expression and realism into scores.

I hear there's a greater playback plan in store, but it's been a long wait since the release of MuseScore 4. And because most of my MS3 scores rely on Offset velocities and custom velocity dynamics, none of them sound good in MS4.I can only hope that my MS3 scores with offset velocities will make sense to the new system and import to MS4 in useable, even if converted form.

Request

Is there a chance for an accelerated fix for MS3 Offset velocity import and the Len Property? Currently the Len issue is in the 4.xSHORTLIST, at this card.

An example of expression (Velocity) and realism (Len) on a lute piece

Consider this audio production of a de Visée Courante which I started and refined in MuseScore 3.7

My Courante MS3 source score relies on Offset velocity, and Len property settings (generated by the TAB Ring plugin). The result sounds plausible right in MuseScore 3 with MSBasic's Mellow Grand Piano.

To generated the audio I exported to MIDI, imported it to Apple Logic Pro X, chose NI Plucked Nylon, and added bass and effects. I'd MUCH rather complete projects in MS4 but that's not possible, not at this level of quality, until Velocity and Len are functioning in MS4. Additionally, Velocity with have to play with VSTs like those from Native Instruments.

Here's my MS3 Courante score imported to MS4 ... and sounding quite awful due to the velocity bugs and Len properties entirely ignored. And for anyone wondering, MuseScore's Pedal (aka MIDI sustain and Let Ring) are not reasonable alternatives to the TAB Ring plugin.

scorster

bkunda commented 1 week ago

Hi everyone,

I've given this issue some thought, and have also discussed it with @RomanPudashkin.

I fully recognise this one is a sore point for many users converting from MS3.6 (and earlier) to v.4.0 and above.

Unfortunately, we're in a situation where we have only one developer assigned to working on the playback engine (our incredibly skilled @RomanPudashkin). And I have it on his authority that the fix for this issue is non-trivial.

It's therefore extremely hard to justify prioritising it ahead of other playback issues at the moment, especially where we still have playback issues for things that simply don't work (such as all the tied-note interactions in the 4.4 project. FWIW, velocity does work in v.4.0 and above; it's the backwards compatibility that's at issue here).

So this issue will unfortunately need to be put on the backlog for the time being. What I can say is that we are introducing independent treatment of dynamics for individual voices and staves in v.4.4, which will render the need to use per-note velocity less relevant for multi-voice and multi-stave contexts (so the problem of writing different dynamics for each hand at the piano now has a proper solution). We also have plans to introduce automation of velocity in the near future, which will further empower the use of this playback parameter.

Apologies that we can't do anything more about this at the moment.

MarcSabatella commented 1 week ago

Hmm, I wonder if there is some misunderstanding here about what we are asking for here? I think the discussion is confusing a lot of different and perhaps somewhat related issues, but the original issue is extremely simple, and it does have a very trivial fix. We just need, right here where the velocity value is being read:

https://github.com/musescore/MuseScore/blob/a654c722e5c1d63ad28abe0bd80b725f375d2c6f/src/engraving/rw/read400/tread.cpp#L3134-L3135

to add code to scale it if n->score()->mscVersion() is less than 400. The scaling code divide the given value by 2 and add 64.

I can submit a PR myself for that.

Jojo-Schmitz commented 1 week ago

That indeed seems the simplest yet sufficient way.

MarcSabatella commented 1 week ago

It is more complex than that, because this is only relevant for "offset" values. For "user" values, it's non-trivial indeed, since we'd need to know the current dynamic to rebase those properly. But, offset is the default velocity type in MU3 and earlier, and it's also way simpler for most people to deal with, so it's what is normally used. So I propose just ignoring user velocity values. Scaling offset values and ignoring user values is going to give a far better starting point for import than the mess that gets created currently.

MarcSabatella commented 1 week ago

OK, I need to take back some of what I've said and apologize for doubting @RomanPudashkin!

I admit I have completely misunderstood just what the "offset" was actually doing all these years. I think my assumption was probably common, and people probably have been using this field expecting to work one way (especially if they listened to me!) and not realizing it actually works another, but anyhow, I now see my naive assumption about how to do the scaling was wrong, and doing it correctly is indeed probably non-trivial.

It's not that it's hard to find the place to make the correction; it's that the correction itself is not necessarily simply a matter of rebasing the offset from 0 to 64. In MU3 and earlier, the offset is not simply added to the velocity value from the current dynamic as I had always assumed. Instead, the offset value specifies a percentage of the current dynamic. That means you need to know the current dynamic value in order to apply the offset correctly, and getting that while reading the file is probably impossible, at least to do in one pass.

On the other hand, I also have no idea how velocity values different than 64 are handled currently. I naively assumed these were also just plain offsets but from a base of 64 instead of 0. But probably this is being scaled in some non-obvious way too. Maybe it just so happens that the dependencies on the current dynamic cancel each other out and there is a simple formula that doesn't require knowing the dynamic but does successfully convert from the old percentage-based offset to whatever is being used now. But I wouldn't know how to derive it.

Bottom line: I am now thinking that rather than try to scale offset values and ignore user values, we should ignore all velocity settings in pre-MU4 scores. It is still infinitely better than importing garbage. This is the same approach we've used over and over with respect to engraving. When we know the default positions of elements have changed such that vertical or horizontal offsets applied in pre-MU4 scores just won't make sense anymore, we simply clear them.

BTW, current builds ignore velocity settings completely, even in MS Basic, as reported in https://github.com/musescore/MuseScore/issues/23470. That makes empirical testing of any proposed conversion formula impossible (and using 4.3.2 isn't good as things definitely seem to have changed since then). But I may test a bit more once that is fixed to see if it just so happens that some "trivial" calculation will produce results that are good enough.

cbjeukendrup commented 1 week ago

Hm, that makes me doubt my understanding of the situation as well. My understanding was:

But now it looks like my understanding of MS4 was not correct. Looking at the code, it looks more like:

Does that match your findings, @MarcSabatella?

Conclusion: it seems like USER velocity from MS3 needs no conversion, and OFFSET velocity would need conversion, but that conversion is indeed very difficult to do (in an acceptable way). So I agree with the proposal to just discard OFFSET velocities.

Even if we converted these values properly, they would be useless, because they were based on the MS3 sound font, and most MS4 users will use Muse Sounds which behaves of course completely differently.

For that reason, I wonder if it makes sense to discard USER velocity as well, even though it would take little effort to preserve it.

@bkunda TL;DR: What do you think about completely ignoring velocity from MS3 when importing in MS4? That will lead to better results most of the time, and as Marc says, fits well with the policy we have applied to engraving settings that were either no longer applicable or most-likely-redundant because of the improvements to the default situation.

cbjeukendrup commented 1 week ago

But wait, it's still weird. By default, all notes get velocity 64. So MS4's velocity property were absolute, all notes would sound equally loud (namely with a velocity of 64), independent of dynamics. But that is obviously not the case. So perhaps MS4's system is still "relative, centred around 64". I'm getting a bit lost...

cbjeukendrup commented 1 week ago

Oh no... there is another layer of weirdness to this.

So, judging purely from the code, MS4's system is absolute, from 0 to 127. But there is one exception: a value of 0 means "use the default". So actually, the scale goes from 1 to 127. But now the jank: look at NotePlaybackModel::loadProperties(). When the actual value is 0 (so "use the default"), it is displayed as 64!

So now the number 64 has two meanings: If you didn't enter it manually, it means: use the default. If you did enter it manually, it means: use 64. And if you enter 0, then you are saying "use the default", but there is no indication that this is the case. And when you select something else and then select the note again and look in the inspector again, it says 64 again (but in the meaning "use the default").

Okay, so now you'd think: if I set it explicitly to some number other than 0, it will use that value, and ignore the current dynamic, because it's an absolute scale. Right? No! The dynamics still have influence. So, if you have notes with the same explicit velocity but different dynamics, they will sound different, and if you have notes with the same dynamics but different explicit velocity, they sound different too.

Judging from the code, this is unexpected, but my theory is that is has to do with the fact that besides velocity, dynamics also affect the volume parameter. I'm now going to try to confirm this using a synth where I map the velocity to something audible like pitch (assuming I can figure out how to do that).

MarcSabatella commented 1 week ago

First, your characterization of the MU3 system is correct. User is taken literally. For offset, negative numbers mean softer than default, positive mean louder. I previously thought it was a purely linear scale, where an often of 42 means add 42 to the default velocity but now I know it's not that simple. Still, yes, definitely 127 is going to be the loudest you can achieve via offset, -127 will be the softest.

For MU4, I never fully understood and still don't, plus it may have changed between 4.3.2 and master but that's hard to test because it doesn't work at all in master. My assumption has been 64 means default, larger number mean louder, and smaller numbers mean quieter. This much does seem to be true in 4.3.2. But it's really hard to say if the scale is "linear" or "percentage-based" or something else entirely.

I can definitely confirm that 127 doesn't mean the same thing in a measure with "p" dynamic as it does in a measure with "mf" or "ff", so it does seem it's not anything like a literal 127, at least for actual volume. But that "p" measure with 127 velocity, while still relatively quiet, definitely seems to have the 127 velocity sample. So, a "p" note with velocity 127 is just as "harsh" as an "ff" note with velocity 127 - it's just quieter. Again, though, I can only easily test 4.3.2 here, and I know the handling of velocity has changed (at least that's what the descriptions of various recent PR's suggest).

cbjeukendrup commented 1 week ago

Okay, so let's collect what we know now about MS4's system:

And we can't really test that hypothesis because it seems broken anyway in master.

Anyway, back to the original topic: I'm still in favour of completely ignoring velocity data from MS3, because the sounds are different anyway. And, because not importing the values at all is better than importing in a way that results in garbage. @MarcSabatella If it's not much work, I even think you could already draft a PR to do this, if you want. But only if it's not much work, because we aren't certain yet whether everyone agrees.

Tantacrul commented 1 week ago

@bkunda @cbjeukendrup - I had a plan in place for velocity back when we created MS4. When we're ready to tackle the overall subject of velocity properly, lemme know. We always planned to tackle it properly when we got to a piano roll or automation. Both were delayed behind other priorities. However, there are simple improvements we can make now, especially in terms of discoverability.

In MS4, velocity was working with soundfonts at one point, though we didn't ever work on migrating velocity from MS3. Has it stopped working?

At this stage, I agree trying to migrate velocity data from MS3 is too low impact. We'd be better off just improving the whole system and forging towards automation.

Keep me in the loop on those conversations! It'll be vital for Audacity also 😀

cbjeukendrup commented 1 week ago

@Tantacrul currently, it is apparently broken in the latest nightly, but there is a separate issue about that and we'll fix it before the release.

About MS3 migration: let's remove it completely then, because what we currently have is too broken.

Apart from that, there is some confusion about what the velocity property is supposed to do at all in MS4. The current behaviour is quite janky, see my comments above.

What is the expected behaviour?

Which of the two is expected?

MarcSabatella commented 1 week ago

My recollection of the originally-discussed interim design is that these values are indeed meant to be "relative" (so 64 means "no change from current dynamic"), and that much appeared to work up until now. The long-term plan was for this to be replaced by an "absolute" system (so the system would automatically assign the correct absolute values to each note, and you wouldn't just see 64 listed for everything). Which would of course make much more sense to work in conjunction with automation lanes, etc.

As for handling MU3 import - yesterday I made a draft PR to do the conversion that I originally thought would be appropriate (just adjusting the 0-based offsets to 64-based offsets). It was while testing the draft PR that I realized MU3 did not in fact work the way I always assumed - that it was actually interpreting the offset as a percentage of the dynamic instead of a literal offset. Modifying that PR to remove the offset scaling I was trying to apply and just set the userVelocity to 0 (which is the default even though it displays as 64) is trivial. So I will update and link the PR appropriately.

Meanwhile, though, if there are simple improvements to the current system that could be made, that sounds good too. For instance, I could imagine a slider instead of a spinbox, so the "center" point would be more obvious (and could be labeled "default"). But people would probably still want a way to set the value numerically even though no one actually knows exactly what the values mean exactly. In particular, they'll want a way to set one note to the same value as another.

Anyhow, I assume no proposed short-term changes would alter the basic fact that it's not viable to correctly honor MU3 velocity values, and that clearing them will remain the best approach. But if it does turn out there is a simple formula to convert, I could go back in and add that.