Hydrogen: loading drumkit does not clean up previous drumkit

[thijz]

load a drumkit that has less instruments that the currently loaded drumkit

expected:
all instruments of the first drumkit are cleaned up and only the instruments of the new drumkit are available

what really happens :
only the overlapping instruments get updated

so if the first drumkit has 10 instruments and the second has 5 instruments, only the first 5 are updated and the other 5 just remain as they were

but this only happens for some songs !?
example song here : https://drive.google.com/file/d/0B1eVS8jzLhY9cUoxbF9yYnpuc2s/view?usp=sharing

[trebmuh]

confirmed

[blablack]

@thijz does this only happen when working with songs?
I just tried with drumkits and couldn't reproduce the problem.

[pvint]

I've fixed this locally - I just need to fix up my fork before I can create a pull request (I accidentally did all of my changes for the Drumkit Images in my Master rather than in a branch, not it makes it a PITA to create a branch for this). Above is a pull request on _my_ fork to show what I've done.

[thijz]

thanks for the input @pvint !
@blablack : i only have it on some songs. did you try it with the song i shared in my previous post?

[pvint]

Submitted a pull request #338 for this.
Paul

[trebmuh]

Hi all,

@pvint : I just tried to compile a fresh git + adding you patch ( https://github.com/pvint/hydrogen/commit/7421adc2094fd71351d243b2ba64f09671f6dc89.patch from https://github.com/hydrogen-music/hydrogen/pull/338 ) and am afraid that it is not working.

What I did:
1) menu -> Project -> Open demo -> GM kit demo 1 (there is 16 instruments in this drumkit)
2) select "pattern" play mode
3) select pattern "main 3-2" (this pattern has a wide use of the instruments)
4) push "play"
5) Go to "sound library"
6) select the beck drumkit (there is only 8 instruments in this drumkit : https://github.com/becks/becks-drumkit )
Result: on the instruments list, I've got the first 8 which are from the beck drumkit, and the next 8 which are still from the previous (GMkit) drumkit as the picture shows:

Note that if I'm going to: menu -> Project -> new, then load the beck drumkit, the drumkit loads fine:

Tell me if I can do some other tries useful for you.
Hope that helps

[pvint]

Good catch @trebmuh thanks!
I took a quick look and was able to reproduce what you mentioned.
I'll try to look at this over the next couple days.
Paul

[pvint]

Just took a quick look, at it appears that this is what is happening:

The new kit is loaded fine replacing the existing instruments, but then when Hydrogen tries to delete the unused instruments it checks to see if there are notes used by those instruments, and if there are notes for it, it does not delete them.

This is the original behavior, and while it is a bit confusing, I would call it "correct" - maybe it could be improved by warning the user that "The following instruments were kept" or something similar?

It does log in the console but most users will not see that:
(D) Hydrogen::removeInstrument Keeping instrument #15
(D) Hydrogen::removeInstrument Keeping instrument #14
(D) Hydrogen::removeInstrument Keeping instrument #13
(D) Hydrogen::removeInstrument Keeping instrument #12
(D) Hydrogen::removeInstrument Keeping instrument #11
(D) Hydrogen::removeInstrument Keeping instrument #10
(D) Hydrogen::removeInstrument Keeping instrument #9

This is at Debug level - likely should be changed to Info level (and maybe add the instrument name would be helpful)

[trebmuh]

Hi @pvint : no worries. Glad it is helping.
Would that be possible to ask the user with a pop up window like this:

The drumkit you want to load contains less instruments than the previous one. Then, some notes will not be kept.
a) That's OK, delete them!
b) Please keep them and keep the old instrument(s) matching those notes.
c) Wahou, I don't want doing this at all. Cancel action.

What are your thought?

[pvint]

@trebmuh - That makes sense to me.

Anyone else have thoughts on it?

[pvint]

How about something like this:

The wording is a bit funny I think (I think it should say something like "Unused instruments will be deleted, but what should we do with instruments that have note?"), but I wanted to get a second opinion.

[trebmuh]

Looks good.

Proposing slightly modified words here, feel free to pick them up if you want:

---

The current kit has 32 instruments but the one you want to load has only 9.
The first 9 instruments will be replaced. If any of the remaining 23 have notes filled in, they can be saved or discarded. What do you want to do ?

[Save] [Discard] [Cancel]

[thijz]

Would it be possible to first check if there actually is a 'problem' ?
IMO If there is no problem the user should not even see this popup

Other than that i have no remarks
Thanks guys!
Op 15-feb.-2016 22:58 schreef "Olivier Humbert" [email protected]:

Looks good.
Proposing slightly modified words here, feel free to pick them up if you
want:

The current kit has 32 instruments but the one you want to load has only 9.
The first 9 instruments will be replaced. If any of the remaining 23 have
notes filled in, they can be saved or discarded. What do you want to do ?
[Save] [Discard] [Cancel]

—
Reply to this email directly or view it on GitHub
https://github.com/hydrogen-music/hydrogen/issues/332#issuecomment-184412622
.

[trebmuh]

@thijz : +1, good catch.
@pvint : what is your thought about thijz proposition ?

[trebmuh]

@pvint: as said here : https://github.com/hydrogen-music/hydrogen/pull/338#issuecomment-189955437 it does work great!

[pvint]

@thijz @trebmuh

I agree - if there's no issue there shouldn't be a warning (mistake on my behalf - and good catch!)

No time tonight, but I'll make that change this week and let you know.

Paul

[trebmuh]

OK. Let me know when you need testing.

[pvint]

I got the "checking for notes before launching the warning dialog" thing pretty much done tonight but got pulled away - with a little luck I can get time to finish it tomorrow night.

Cheers
Paul

[pvint]

I added a test to see if instruments that might be removed have active notes or not before prompting the user about removing or keeping them. See #338

[pvint]

Also improved (I think) the wording on the dialog that will get presented to the user when there are more instruments in the old kit than the one being loaded (and one or more of them has notes)

[thijz]

looking good, but i have 1 question:
suppose i actually get the popup as shown in the screenshot above,
then what will happen if i select save ?
will i get empty instruments (10 to 32) with notes in there, but with no samples ?
i'm not sure if there is a valid usecase for the save option

edit: maybe 'keep' would be better than 'save' ?
(save makes me think of saving to disk)

[pvint]

I initially had changed it to something other than "Save", I think, but put it back to be consistent. I figured with the wording at "some instruments have notes, would you them to be saved...?" it would likely be clear, but the evidence seems to be that it is not clear... and now I can see from my wording it is not clear whether I mean the notes will be kept, or the instruments, or both...

Maybe it would be better to say:
"Would you like to keep or discard the remaining instruments and notes?"

The dialog in general is too wordy, but I'm not sure how to shorten it....

[trebmuh]

@pvint : I would say that your last proposition is good with "Would you like to keep or discard the remaining instruments and notes?" and then, the tree buttons: [Keep] [Discard] [Cancel].

[mauser]

The pull-request from @pvint has been merged now, so i'm going to close the issue..

[trebmuh]

Added to : https://github.com/hydrogen-music/hydrogen/wiki/New-features-in-Hydrogen-0.9.7#all-platforms