search

chipweinberger / dart_melty_soundfont

A port of Melty Synth by Nobuaki Tanaka (C#) to Dart
Other
33 stars 8 forks source link

Rewrite to match C# more closely #20

Closed rodydavis closed 1 year ago

rodydavis commented 1 year ago
chipweinberger commented 1 year ago

appreciate the PR!

The super enums do look like a nice improvement!

realistically, it's much too large for me to be comfortable merging. This code base is pretty much "done" so it would be hard to justify such a large change.

Ill need to look at it in more detail later.

rodydavis commented 1 year ago

Rewrite is a strong word, but really I should have put the title as refactor.

The api should be non breaking.

Also a lot of the changes are due to formatting too and moving files into a src folder to make them private.

rodydavis commented 1 year ago

~~Also forgot to mention the reason I'm doing it is the sound font I tested does not play the correct key for middle C.

So I was going to check the math in the port and discovered that I could port the tests from the parent project.~~

Turns out I had a mismatch on buffer sizes. Regardless feel free to review the PR if you want.

I also ported the midi file classes from the other fork and updated the enums.

Also wanted to mention I split out the dart:io support into separate files so it can be imported on web

chipweinberger commented 1 year ago

@rodydavis just want to reiterate, this pull request is orders of magnitude too big.

I'm open to a lot of these changes. But there is too much in here.

If you want to change the file structure, that should be a single PR.

If you want to change the enums, that should be a single PR.

Commits should have clear messages. "updating" is not very useful.

as-is, it is too much for me to review. I can barely load it all on github!

etc.

chipweinberger commented 1 year ago

A couple other things, we don't need to match C#. We should use dart conventions.

Enums use lower case values in dart. i.e. GeneratorType.keyRange, not GeneratorType.KeyRange

It's hard to tell what else has changed because the diff shows as you deleting every file and rewriting it. That is not useful for review. File structure changes should be in their own PR, so git can notice that the file has been renamed and just say "renamed X -> Y", making it easy to review.