add new extension MIDI Parser#2035
Conversation
github-actions
Bot
added
the
pr: new extension
|
The code doesn't even work |
|
But the code works fine on my end.How are you using it? |
|
oh wait nevermind, github mobile is just really messing up the display |
Can this code run normally on your side? |
|
it really does, in fact, not work (at least when pasting the source code). you are defining MidiLibrary twice, for some reason also ideally you should define that string inside the main function to not pollute global scope |
But the code for the extension works perfectly on TurboWarp. |
i tried it on turbowarp |
|
i also tried cloning your repository and running it locally. still errors |
But this issue doesn't cause much trouble.The extension can output JSON properly. |
the issue quite literally PREVENTS THE EXTENSION FROM LOADING |
Oh, sorry, it was my mistake. I duplicated "const MidiLibrary". It was my carelessness. I'll fix it right away. |
I have fixed the issue of duplicate declaration of "const MidiLibrary". |
|
@CHCAT1320 I added a pull request to your repo with a suggested change on how you import the tonejs/midi code. Do you think it'd make sense to merge this PR and my WebMidi one into the same extension? |
| return 'MIDI library failed to load'; | ||
| } | ||
|
|
||
| const dataUrl = args.DATA_URL.trim(); |
There was a problem hiding this comment.
You should use cast: Scratch.Cast.toString(args.DATA_URL).trim()
| midi.tracks.forEach((track, trackIndex) => { | ||
| track.notes.forEach(note => { | ||
| this.notesData.push({ | ||
| track: trackIndex, |
There was a problem hiding this comment.
Suggestion: consider outputting this as trackIndex + 1 since scratch indexes start at 1 instead of 0
| noteName: note.name, | ||
| startTime: note.time, | ||
| duration: note.duration, | ||
| velocity: note.velocity |
There was a problem hiding this comment.
Suggestion: @tonejs/midi outputs a normalized value of 0-1. Consider scaling this to 0-100, since that seems to be standard scaling for scratch. (I have no preference personally)
There was a problem hiding this comment.
When I finished writing,I found there were no blocks for controlling note velocity,and the volume-control blocks were global.I intended to delete the velocity key.
Sure, it's my first time writing an extension, so there are many shortcomings. Thanks for your suggestions. Feel free to modify or reference it. |
| // License: MIT | ||
|
|
||
|
|
||
| // tonejs/midi |
There was a problem hiding this comment.
This should probably be a full comment block including link to the original code and maybe license text as well - check other extensions for examples
|
|
||
| const dataUrl = args.DATA_URL.trim(); | ||
| if (!dataUrl) { | ||
| return 'MIDI Data URL is empty'; |
There was a problem hiding this comment.
These error messages should probably be wrapped in Scratch.translate()
lselden
left a comment
lselden
left a comment
There was a problem hiding this comment.
Added some suggestions / tips. See also the PR I passed in as well, which has fixes for some of the linting errors you're getting
Glad you're open to it. I can manage merging the changes together. Please don't hesitate to contribute to it! |
4 participants
Parse MIDI files into JSON.