Skip to content

Old MIDI drum block support, featuring approximations via pitch shift #1613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from

Conversation

towerofnix
Copy link
Contributor

Resolves

Fixes #1424. Sort of a follow up to #1329 and #355.

This also (not) accidentally allows the "set (audio) effect" block's "pitch" to have an effect on drums. I figure this is okay since the pan left/right effect already worked; and it's necessary to have the MIDI drum approximations.

Proposed Changes

Adds support for the obsolete drum:duration:elapsed:from: block, including very similar MIDI drum approximations to the ones originally created for Scratch 2.0.

Reason for Changes

Compatibility with old Scratch projects (and new ones which use that block).

Test Coverage

Tested manually by confirming that:

This is probably done as is. There is data for decay, but there is no
audio decay effect yet, so I don't think I can do anything with it yet.
@@ -608,6 +608,62 @@ class Scratch3MusicBlocks {
];
}

/**
* An array that is a mapping from MIDI drum numbers in range (35..81) to Scratch drum numbers.
* @type {Array[Array]} an array of information about the drums, in the format [drumNum, pitch, decay].

This comment was marked as abuse.

blockType: BlockType.COMMAND,
text: formatMessage({
id: 'music.midiPlayDrumForBeats',
default: 'play drum [DRUM] for [BEATS] beats',

This comment was marked as abuse.

drumNum = midiDescription[0];
pitchShift = midiDescription[1] * 10; // 10 = one semitone
} else {
drumNum = 2;

This comment was marked as abuse.

const pitchEffect = chain._effects.find(effect => effect.name === 'pitch')
if (pitchEffect) {
pitchEffect.updatePlayer(player);
}

This comment was marked as abuse.

@ericrosenbaum
Copy link
Contributor

Thanks for this @towerofnix ! I'm excited to give this a look- due to other work it may have to wait a week or more.

@ericrosenbaum
Copy link
Contributor

ericrosenbaum commented Nov 19, 2018

Okay this is working well, but I'd like to make some structural changes, and maybe remove the use of the pitch effect (for simplicity). If it's okay with you I will push some changes directly to the PR branch (assuming I can figure out how!).

update: I seem to have got into some git trouble. Looking into it..

ericrosenbaum and others added 5 commits November 19, 2018 16:44
This is probably done as is. There is data for decay, but there is no
audio decay effect yet, so I don't think I can do anything with it yet.
…h-vm into midi-play-drum

# Conflicts:
#	src/extensions/scratch3_music/index.js
@towerofnix
Copy link
Contributor Author

@ericrosenbaum Okay! That's fine with me. Good luck fixing the git issues.. always a pain, of course :P

(In the uber-worst case scenario that this PR is utterly irredeemable, I have the commits separately on my computer - so it's easy to re-push/PR that if necessary.)

@ericrosenbaum
Copy link
Contributor

@towerofnix Okay, @paulkaplan helped me fix the branch. Phew! But now that it has been force-pushed from my side, you won't be able to push correctly to it from your existing local branch. I can take over from here (just moving a few things around). If you do want to push any changes, I think you would just have to rename your local branch, then pull this one down, make the changes there, and push from it. Sorry again about the confusion (I learned something!).

@towerofnix
Copy link
Contributor Author

towerofnix commented Nov 20, 2018

@ericrosenbaum Alright, sounds good! And no problem! (:stars: ~The more you know~ :stars:)

Good to know - I've fixed up my own branch so it'll push/pull fine. I'm cool with you taking over from here. Just one thought:

I'd like to [..] maybe remove the use of the pitch effect (for simplicity).

I'm okay with this, but I think it is a pretty important part of simulating the sounds - several sounds just don't sound the same when their pitches aren't right. It's a lot of constant numbers which we can't be 100% sure are all exactly correct, but I think some effort will make them sound a lot better than none. There's not a lot of pressure to get them perfect either, since 1.4 MIDI-drum projects are relatively rare. Thoughts?

Aside - there are a few "decay" values which can be removed from that constants array, since I don't have any code for the feature and it would probably take a significant amount of effort to implement.

@as-com
Copy link

as-com commented Nov 20, 2018

Aside - there are a few "decay" values which can be removed from that constants array, since I don't have any code for the feature and it would probably take a significant amount of effort to implement.

I think decay is very important; if it's not a ridiculous amount of effort to implement, it should be done (since some of my projects just don't sound right without the shorter decays of some instruments).

@ericrosenbaum
Copy link
Contributor

Okay, I've now made the changes. The main thing is to keep the legacy Scratch 1.x-related code more separate from the rest of the code- so I just moved the drum number mapping into midiPlayDrumForBeats.

I recently fixed a couple of other bugs in the music extension by removing the use of the effects chain (#1735). This prevents us from using the pitch effect, so we will not be able to get the sound variations between the different 1.4 drums. I appreciate the clever work on this, but going forward we probably won't be able to use it.

Thanks for your help @towerofnix !

@ericrosenbaum
Copy link
Contributor

Hm. In another hilarious github-related plot-twist, the diffs are not showing up correctly (apparently this is sometimes an issue on branches that have been force-pushed). I can't figure out a way to fix this PR, so I'm just going to create a new one.

@ericrosenbaum
Copy link
Contributor

Replaced by #1790

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants