Move standard drumset to instruments.xml #25759
Conversation
The standard drumset was only used by the Large Drum Kit and the Percussion Synthesizer; the only unpitched percussion instruments that didn't specify any <Drum> elements in instruments.xml.
shoogle
force-pushed
the
instruments-standard-drumset
branch
from
f096593 to
5962d46
Compare
|
In Mu3 Open High Conga and Low Conga were also part of an SM drumkit, not in Mu4, on purpose or by mistake? |
| <voice>0</voice> | ||
| <name>Acoustic Snare</name> | ||
| <stem>1</stem> | ||
| <shortcut>A</shortcut> |
There was a problem hiding this comment.
shouldn't the shortcut here be at the electric snare?
| @@ -604,6 +604,9 @@ void InstrumentTemplate::read(XmlReader& e) | |||
| } | |||
| if (id.empty()) { | |||
| id = trackName.toLower().replace(u' ', u'-'); | |||
| } else if (id == "drumset") { | |||
There was a problem hiding this comment.
Shouldn't this check for smDrumset to not be a nullptr?
There was a problem hiding this comment.
No. Although I removed all the drums from Drumset::initDrumset(), the function itself still exists, so smDrumset will never be nullptr when this line is reached. Instead, it will be a valid (albeit empty) drumset, which is why I delete it on the next line.
Initializing an empty drumset is useful in case instruments.xml isn't found for some reason, but we don't care about it being empty because the user would have bigger problems if instruments.xml wasn't found.
Note: Even if smDrumset was nullptr, it's still safe to call delete on a nullptr. It's only unsafe to call delete on a void pointer, or a pointer that has already been deleted. However, you can't check for a deleted pointer, so whenever you delete a pointer you should immediately set it to nullptr (or another value, as I do here), so it's safe to delete it again in future (i.e. in case another instrument is loaded with id == "drumset").
There was a problem hiding this comment.
The delete is not the problem, the assignment in the next line might be though
There was a problem hiding this comment.
But you may have a point though. If a second instrument was loaded with id == "drumset" then calling delete smDrumset would mean that the first instrument's drumset would now be broken, so I guess I do need to check for that case specifically.
There was a problem hiding this comment.
Crossed posts. What's wrong with the assignment? Are you referring to the issue where two instruments have id == "drumset" that I just mentioned?
There was a problem hiding this comment.
No. when smDrumset is a nullptr, then smDrumset = drumset; crashes
There was a problem hiding this comment.
This doesn't crash. Why would it?
smDrumset = nullptr;
smDrumset = drumset;This also does not crash (at least not immediately, it may crash later if subsequent code doesn't check for nullptr).
drumset = nullptr;
smDrumset = drumset;I think you're confused about something.
There was a problem hiding this comment.
I guess I am...
In my attempt to backport this to Mu3 it does crash there, when not checking
else if (id == "drumset" && !smDrumset) {
delete smDrumset;
smDrumset = drumset;
}There was a problem hiding this comment.
Perhaps MU3 has another pointer to the same object, so when it is deleted here and then assigned to something else, MU3's other pointer still points to the original object, which is now deleted.
In any case, I recommend that you don't backport this to MU3, at least not yet because:
- This is really just a proof of concept. We might decide not to merge it.
- It doesn't add any new features. If you manage to backport it properly, at best nothing will change.
If we do merge it, we might then make some changes to the drumset in the spreadsheet, at which point it might be worth your while backporting it to MU3. But until then, there's no point backporting it.
There was a problem hiding this comment.
Yes, I'll wait for those, thanks
|
When I open this MIDI file (made with MS3 drum kit) in this PR build, the instrument does not load in the mixer and there is no playback. It works as expected in Master |
|
This PR is likely to be superseded by PR #26082. |
The standard drumset was only used by the Large Drum Kit and the Percussion Synthesizer; the only unpitched percussion instruments that didn't specify any
<Drum>elements ininstruments.xml.