Skip to content
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

Merge improvements (pitch bend, parseSong(), ...) from webaudiofont #34

Open
page200 opened this issue Jul 28, 2023 · 10 comments
Open

Merge improvements (pitch bend, parseSong(), ...) from webaudiofont #34

page200 opened this issue Jul 28, 2023 · 10 comments

Comments

@page200
Copy link

page200 commented Jul 28, 2023

There are various improvements in a copy of MIDIFile.js in https://github.com/surikov/webaudiofont/blame/master/examples/MIDIFile.js such as support for pitch bends (slides).

It might be good to merge those improvements into this repo, and replace MIDIFile.js in that repo by a link to this repo, as discussed at #33 (comment)

@mk-pmb
Copy link
Collaborator

mk-pmb commented Jul 28, 2023

Unfortunately they seem to use a stricter license (GNU GPL v3) than we do (MIT), so we cannot take code from their repo. We'd have to track down the relevant patch author(s) and request explicit permission from them.

Most of the code lines that have the word "slide" seem to come from this commit by user @surikov but I cannot find a PR for that. Also the commit title "test" looks like the patch might not be a single feature, so we'd need to figure out whether we need other patches as well to support slides.

If your patch from surikov/webaudiofont#95 works without slides, feel free to submit it as PR here under MIT license. (Assuming you do have all required copyrights, of course.)

@page200
Copy link
Author

page200 commented Jul 30, 2023

@surikov's patches introduce pitch bend etc., and my patch surikov/webaudiofont#95 adds support for MIDI files with custom pitch-bend range, so that the pitch bend is correct for them.

What form of permission is needed, as comments on GitHub? You have my permission, and I think @surikov also suggests here that @nfroidure's repos should be modified accordingly (and that @surikov's patches to MIDIFile.js aren't the focus of his repo anyway, but are merely a small example).

@mk-pmb
Copy link
Collaborator

mk-pmb commented Jul 31, 2023

What form of permission is needed, as comments on GitHub?

Obligatory disclaimer: I'm not a lawyer, and the following is just how I as a layman understand (US) copyright and (European) authorship protection.

We need an MIT license grant. A comment stating the grant would work in theory, but we'd have to keep that around somehow. The easier way is to submit a PR for a branch that is

  • based on our master branch and
  • whose new commits do not modify any license or authorship statements in existing files.
    • (and do not introduce any new terms that contradict the current license.)

The 2nd condition acts as implied license grant under the existing license. It's only effective if you're legally capable of granting the license, but this is most usually the case for your own creations.

@mk-pmb
Copy link
Collaborator

mk-pmb commented Jul 31, 2023

I checked surikov/webaudiofont#95 and it looks like the patch there isn't directly compatible, even if I adjust the file paths. For example, it tries to modify a function addSlide but that code seems to not exist on our master branch. My quick attempt to trace where addSlide comes from, failed. I might try again later.

Edit: If with

surikov's patches introduce pitch bend etc.,

you mean he is the author of the patches for those features, we'd have to ask him to please PR his changes into our repo. However, they may require even more patches from other authors, can't currently check that.

@page200
Copy link
Author

page200 commented Jul 31, 2023

My patch fixes the pitch-bend range. @nfroidure's repos don't have pitch bend yet (as far as I know / in the form we're discussing). @surikov's repo does.

Here's the blame history for addSlide: https://github.com/surikov/webaudiofont/blame/8d3bbebcb1b3fa5642b931bf1eff9e7740795c51/examples/MIDIFile.js#L1009-L1025

The contributors (@surikov and me) are listed on the left side of the code and on the top right.

The code was added in subsequent commits after "forking" the original MIDIFile.js.

@mk-pmb
Copy link
Collaborator

mk-pmb commented Jul 31, 2023

Thanks! So it seems the other code all comes from the "test" commit. Back then, the repo seems to have been under MIT license still, so we might be in luck. I've started negotiations.

Once we have proper permissions on that code, we'll have to figure out stuff like what this magic number means:

if (track.notes[i].duration == 0.0000001

and what conparison operator to use instead, probably <= or ===.

Edit: Also there is a suspicious change from binary | to boolean || near the /127 division.

@mk-pmb
Copy link
Collaborator

mk-pmb commented Oct 8, 2023

The issue where I asked how to credit him was closed without comment, so I guess he doesn't care. My approach would be to use the "From:" header of the "test" commit as a patch file as the author attribution. Would you other maintainers agree?

@page200
Copy link
Author

page200 commented Jan 3, 2024

Yes, that sounds like an appropriate author attribution. And feel free to attribute me for the pitch-bend range functionality, if that makes things easier. I welcome a license that benefits the project and its widespread use. Thanks!

@mk-pmb
Copy link
Collaborator

mk-pmb commented Jan 3, 2024

And feel free to attribute me for the pitch-bend range functionality,

Are you refering to parts of the "test" commit? Are you the real author of part of that? If so, can you PR those parts first?

Edit: I see now that you probably meant the git-blame you linked above:

Here's the blame history for addSlide: […]

I'll take that into account.

@page200
Copy link
Author

page200 commented Jan 3, 2024

I meant surikov/webaudiofont#95.

I'll take that into account.

Whatever benefits a widespread use of the project.

Thanks!

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

No branches or pull requests

2 participants