Hi Martin, I reviewed your changed version on midi2ly with inclusion in official sources in mind, the patch that match your work (minus trailing whitespaces and indentation errors). If you want your future work on midi2ly to be considered for committing into official sources, it is highly recommended to send patches (instead of the full file) to this list, although you are free to send again the full script to user-list to make user testing easier. You can read more about submitting patches in out Contributor's guide.
I'm sorry to set higher requirements on code quality than the original
code, but not doing so would not arrange the bad maintenance state of
this script, and would not encourage future work on it.
> diff --git a/scripts/midi2ly.py b/scripts/midi2ly.py
> index c57c788..040dec5 100644
> --- a/scripts/midi2ly.py
> +++ b/scripts/midi2ly.py
> @@ -11,7 +11,7 @@
> '''
> TODO:
> * test on weird and unquantised midi input (lily-devel)
> - * update doc and manpage
> + * update doc and manpage and translations
Updating translations is just a matter of updating lilypond.pot and
sending it to the Translation Project, it doesn't require changing this
script again, so you needn't change this line. However, you could add
something like
* move each global variable used in the conversion process to
global_options or per-file variable
(see just below)
> @@ -497,19 +505,28 @@ def events_on_channel (channel):
> seconds_per_1 = us_per_4 * 4 / 1e6
> events.append ((t, Tempo (seconds_per_1)))
> elif e[1][1] == midi.TIME_SIGNATURE:
> + global num, den
Global variables are evil when multiple files can be processed, better
define these as per-files variables or attributes of global_options.
> (num, dur, clocks4, count32) = map (ord, e[1][2])
> den = 2 ** dur
> + if global_options.mytime:
> + num =
> int(global_options.mytime[:global_options.mytime.index('/')])
> + den =
> int(global_options.mytime[global_options.mytime.index('/')+1:])
Could you rewrite this using something more concise and clear like
num, den = [int (s) for s in global_options.mytime.split ('/')]
after having checked at the options parsing stage that mytime has a correct
form?
> events.append ((t, Time (num, den)))
> elif e[1][1] == midi.KEY_SIGNATURE:
> (alterations, minor) = map (ord, e[1][2])
> sharps = 0
> flats = 0
> +
> if alterations < 127:
> sharps = alterations
> else:
> flats = 256 - alterations
>
> - k = Key (sharps, flats, minor)
> + if mysharps == myflats:
> + k = Key (sharps, flats, minor)
> + else:
> + k = Key (mysharps, myflats, myminor)
> +
Why do you use variable names starting with "my"? If it means some
values can be overriden by the user, this should not appear in the
variable names but the variables should be initalized with the values
properties of global_options in case this override is forced by some
option or makes sense in all situations.
> +def mykey (sharps, flats, minor):
> + ks = [ 'g' , 'd' , 'a' , 'e' , 'b' , 'fis' , 'cis', 'gis', 'dis' ]
> + kf = [ 'd', 'g', 'c' , 'f' , 'bes' , 'es' , 'as' , 'des' , 'ges' , 'ces'
> ]
> + if sharps > 0:
> + mykey = ks [ sharps - 1 + 3*minor ]
> + elif flats > 0:
> + mykey = kf [ 2 + flats - 3*minor ]
> + else:
> + if minor == 1:
> + mykey = 'a'
> + else:
> + mykey = 'c'
Nice, but why don't you handle the case sharps == flats == 0 by
prepending 'a' to kf, replacing "elif flats...:" with "else:" and adding
1 to kf index? You could even have only one list named fifth_helix and
use -flats instead of +flats.
By the way:
- this list (or these two lists) should be tuples,
- this duplicates lists at the beginning of class Key,
- full names (with underscores for compound names) would be better than
initialisms like kf, ks;
- we use no space at the inside side of brackets and parentheses, e.g.
"[sharps - 1]" and not "[ sharps -1 ]", and list and dictionary
subscripts should not be separated by space, e.g.
"ks[sharps - 1]" not "ks [sharps - 1]".
Oh, and did you notice your function duplicates the functionality of
Key.dump? Please consider replacing Key.dump with your function, which
I find clearer.
> def convert_midi (in_file, out_file):
> global clocks_per_1, clocks_per_4, key
> global start_quant_clocks
> - global duration_quant_clocks
> + global duration_quant_clocks
> global allowed_tuplet_clocks
> + global mysharps, myflats, myminor, use_mykey
Same thing as above: prefer global options, or variables specific to
each file, over global variables.
> str = open (in_file).read ()
> midi_dump = midi.parse (str)
>
> @@ -804,33 +848,63 @@ def convert_midi (in_file, out_file):
> for (dur, num, den) in global_options.allowed_tuplets:
> allowed_tuplet_clocks.append (clocks_per_1 / den)
>
> + if global_options.key.sharps + global_options.key.flats +
> global_options.key.minor !=0:
>
> + mysharps = global_options.key.sharps
> + myflats = global_options.key.flats
> + myminor = global_options.key.minor
> + use_mykey = 1
> + else:
> + mysharps = global_options.key.sharps
> + myflats = global_options.key.flats
> + myminor = global_options.key.minor
> + use_mykey = 0
I don't see the point of defining global variables mysharps, myflats,
myminor if you can retrieve their values as well through global_options;
use_mykey should be defined as an attribute of global_options.
> + s = s + '\\header {\n'
"s +=" is as clear but shorter.
> i = 0
> + A = 0
Could A be a boolean and be given a more explicit name?
> + p.add_option ('-P', '--partial',
> + metavar =_ ("DUR"),
> + action = "store",
> + dest = "mypartial",
> + help =_ ("insert global \"\\partial DUR\" upbeat"))
> + p.add_option('-s', '--start-quant',
> + help= _ ("quantise note starts on DUR"),
> + default = '16',
> + metavar= _ ("DUR"))
> + p.add_option ('--skip',
> + action = "store_true",
> + help =_ ("use s instead of r for rests"))
> + p.add_option('-t', '--allow-tuplet',
> metavar=_ ("DUR*NUM/DEN"),
> action = "append",
> dest="allowed_tuplets",
> - help=_ ("allow tuplet durations DUR*NUM/DEN"),
> + help= _ ("allow tuplet durations DUR*NUM/DEN"),
> default=[])
> + p.add_option ('-T', '--time',
> + metavar =_ ("NUM/DEN"),
> + action = "store",
> + dest = "mytime",
> + help =_ ("insert global \"\\time NUM/DEN\" signature"))
> p.add_option ('-V', '--verbose', help=_ ("be verbose"),
> - action='store_true'
> - ),
> - p.version = "midi2ly (LilyPond) @TOPLEVEL_VERSION@"
> + action='store_true')
> + p.version = "midi2ly (LilyPond) %s" % ( program_version )
Spaces usage is incorrect in some places and not consistent with
previous spacing in some other places. I don't see why variable names
are prefixed with "my" (as above).
> @@ -913,11 +1007,8 @@ def do_options ():
> if options.duration_quant:
> options.duration_quant = int (options.duration_quant)
>
> - if options.warranty:
> - warranty ()
> - sys.exit (0)
> if 1:
> - (alterations, minor) = map (int, (options.key + ':0').split
> (':'))[0:2]
> + (alterations, minor) = map (int, (options.mykey + ':0').split
> (':'))[0:2]
Again, I'm not fond prefixing variable names with "my".
I'm looking forward to a new patch against current master branch (or
equally 2.13.6 sources) that solves these nitpicks.
Best,
John
diff --git a/scripts/midi2ly.py b/scripts/midi2ly.py
index c57c788..040dec5 100644
--- a/scripts/midi2ly.py
+++ b/scripts/midi2ly.py
@@ -11,7 +11,7 @@
'''
TODO:
* test on weird and unquantised midi input (lily-devel)
- * update doc and manpage
+ * update doc and manpage and translations
* simply insert clef changes whenever too many ledger lines
[to avoid tex capacity exceeded]
@@ -19,6 +19,12 @@ TODO:
* better lyrics handling
* [see if it is feasible to] move ly-classes to library for use in
other converters, while leaving midi specific stuff here
+
+ * split notes that cross barlines, use ties
+ * better handling of polyphony in one staff.
+ * make barcheck and barcount work even after meter changes
+ * split midifile format-0 files into separate tracks ??
+ * test and debug using more, complex example MIDI files
'''
import os
@@ -50,7 +56,6 @@ start_quant_clocks = 0
duration_quant_clocks = 0
allowed_tuplet_clocks = []
-
################################################################
@@ -285,7 +290,7 @@ class Time:
def dump (self):
global time
time = self
- return '\n ' + '\\time %d/%d ' % (self.num, self.den) + '\n '
+ return '\n ' + '\\time %d/%d ' % (self.num, self.den) + ' '
class Tempo:
def __init__ (self, seconds_per_1):
@@ -299,7 +304,7 @@ class Tempo:
return 4 * 60 / self.seconds_per_1
def dump (self):
- return '\n ' + '\\tempo 4 = %d ' % (self.bpm()) + '\n '
+ return '\n ' + '\\tempo 4 = %d ' % (self.bpm()) + '\n '
class Clef:
clefs = ('"bass_8"', 'bass', 'violin', '"violin^8"')
@@ -310,7 +315,7 @@ class Clef:
return 'Clef(%s)' % self.clefs[self.type]
def dump (self):
- return '\n \\clef %s\n ' % self.clefs[self.type]
+ return ' \\clef %s\n' % self.clefs[self.type]
class Key:
key_sharps = ('c', 'g', 'd', 'a', 'e', 'b', 'fis')
@@ -360,8 +365,10 @@ class Key:
else:
s = s + ' \\major'
- return '\n\n ' + s + '\n '
-
+ if use_mykey == 0:
+ return '\n ' + s + ' '
+ else:
+ return ''
class Text:
text_types = (
@@ -388,6 +395,8 @@ class Text:
or d.compare (reference_note.duration):
s = s + Duration (self.clocks).dump ()
s = s + ' '
+ elif self.type == midi.SEQUENCE_TRACK_NAME:
+ s = '\\set Staff.instrumentName="%s"' % self.text
else:
s = '\n % [' + self.text_types[self.type] + '] ' + self.text + '\n '
return s
@@ -459,14 +468,13 @@ def end_note (pitches, notes, t, e):
def events_on_channel (channel):
pitches = {}
-
notes = []
events = []
last_lyric = 0
last_time = 0
+
for e in channel:
t = e[0]
-
if start_quant_clocks:
t = quantise_clocks (t, start_quant_clocks)
@@ -497,19 +505,28 @@ def events_on_channel (channel):
seconds_per_1 = us_per_4 * 4 / 1e6
events.append ((t, Tempo (seconds_per_1)))
elif e[1][1] == midi.TIME_SIGNATURE:
+ global num, den
(num, dur, clocks4, count32) = map (ord, e[1][2])
den = 2 ** dur
+ if global_options.mytime:
+ num = int(global_options.mytime[:global_options.mytime.index('/')])
+ den = int(global_options.mytime[global_options.mytime.index('/')+1:])
events.append ((t, Time (num, den)))
elif e[1][1] == midi.KEY_SIGNATURE:
(alterations, minor) = map (ord, e[1][2])
sharps = 0
flats = 0
+
if alterations < 127:
sharps = alterations
else:
flats = 256 - alterations
- k = Key (sharps, flats, minor)
+ if mysharps == myflats:
+ k = Key (sharps, flats, minor)
+ else:
+ k = Key (mysharps, myflats, myminor)
+
events.append ((t, k))
# ugh, must set key while parsing
@@ -639,11 +656,12 @@ def dump_bar_line (last_bar_t, t, bar_count):
def dump_channel (thread, skip):
global reference_note, time
- global_options.key = Key (0, 0, 0)
- time = Time (4, 4)
+ time = Time (num, den)
+
# urg LilyPond doesn't start at c4, but
# remembers from previous tracks!
# reference_note = Note (clocks_per_4, 4*12, 0)
+
reference_note = Note (0, 4*12, 0)
last_e = None
chs = []
@@ -667,7 +685,8 @@ def dump_channel (thread, skip):
last_bar_t = 0
bar_count = 1
- lines = ['']
+ lines = ['\mytime ']
+
for ch in chs:
t = ch[0]
@@ -682,6 +701,7 @@ def dump_channel (thread, skip):
(s, last_bar_t, bar_count) = dump_bar_line (last_bar_t,
t, bar_count)
+
lines[-1] = lines[-1] + s
lines[-1] = lines[-1] + dump_chord (ch[1])
@@ -715,7 +735,11 @@ def dump_track (channels, n):
item = thread_first_item (channels[i])
if item and item.__class__ == Note:
- skip = 's'
+ if global_options.skip:
+ skip = 's'
+ else:
+ skip = 'r'
+
s = s + '%s = ' % (track + channel)
if not global_options.absolute_pitches:
s = s + '\\relative c '
@@ -723,8 +747,9 @@ def dump_track (channels, n):
skip = '" "'
s = s + '%s = \\lyricmode ' % (track + channel)
else:
- skip = '\\skip '
+ skip = ' \\skip '
s = s + '%s = ' % (track + channel)
+
s = s + '{\n'
s = s + ' ' + dump_channel (channels[i][0], skip)
s = s + '}\n\n'
@@ -732,7 +757,7 @@ def dump_track (channels, n):
s = s + '%s = <<\n' % track
if clef.type != 2:
- s = s + clef.dump () + '\n'
+ s = s + clef.dump ()
for i in range (len (channels)):
channel = channel_name (i)
@@ -781,12 +806,31 @@ def guess_clef (track):
else:
return Clef (2)
+def mykey (sharps, flats, minor):
+ ks = [ 'g' , 'd' , 'a' , 'e' , 'b' , 'fis' , 'cis', 'gis', 'dis' ]
+ kf = [ 'd', 'g', 'c' , 'f' , 'bes' , 'es' , 'as' , 'des' , 'ges' , 'ces' ]
+ if sharps > 0:
+ mykey = ks [ sharps - 1 + 3*minor ]
+ elif flats > 0:
+ mykey = kf [ 2 + flats - 3*minor ]
+ else:
+ if minor == 1:
+ mykey = 'a'
+ else:
+ mykey = 'c'
+
+ if minor == 1:
+ mykey = mykey + ' \\minor'
+ else:
+ mykey = mykey + ' \\major'
+ return mykey
def convert_midi (in_file, out_file):
global clocks_per_1, clocks_per_4, key
global start_quant_clocks
- global duration_quant_clocks
+ global duration_quant_clocks
global allowed_tuplet_clocks
+ global mysharps, myflats, myminor, use_mykey
str = open (in_file).read ()
midi_dump = midi.parse (str)
@@ -804,33 +848,63 @@ def convert_midi (in_file, out_file):
for (dur, num, den) in global_options.allowed_tuplets:
allowed_tuplet_clocks.append (clocks_per_1 / den)
+ if global_options.key.sharps + global_options.key.flats + global_options.key.minor !=0:
+ mysharps = global_options.key.sharps
+ myflats = global_options.key.flats
+ myminor = global_options.key.minor
+ use_mykey = 1
+ else:
+ mysharps = global_options.key.sharps
+ myflats = global_options.key.flats
+ myminor = global_options.key.minor
+ use_mykey = 0
+
tracks = []
for t in midi_dump[1]:
- global_options.key = Key (0, 0, 0)
tracks.append (split_track (t))
- tag = '%% Lily was here -- automatically converted by %s from %s' % ( program_name, in_file)
+ tag = 'Automatically converted by %s (Lilypond v.%s) from %s' % ( program_name, program_version, in_file)
+
+ s = '\\version "%s"\n\n' % ( program_version )
+
+ s = s + '\\header {\n'
+ s = s + ' title = "%s"\n' % ( in_file[:in_file.index('.')] )
+ s = s + ' tagline = "%s"\n' % ( tag )
+ s = s + '}\n\n'
+ s = s + 'mytime = {\n'
+ if global_options.mytime:
+ s = s + ' \\time ' + global_options.mytime + '\n'
+
+ if global_options.mypartial:
+ s = s + ' \\partial ' + global_options.mypartial + '\n'
- s = ''
- s = tag + '\n\\version "2.7.18"\n\n'
+ if use_mykey == 1:
+ s = s + ' \\key ' + mykey(mysharps, myflats, myminor) + '\n'
+
+ s = s + '}\n'
+
for i in range (len (tracks)):
s = s + dump_track (tracks[i], i)
s = s + '\n\\score {\n <<\n'
-
i = 0
+ A = 0
for t in tracks:
track = track_name (i)
item = track_first_item (t)
if item and item.__class__ == Note:
- s = s + ' \\context Staff=%s \\%s\n' % (track, track)
+ if not ( A == 1 and track == 'trackB' ) :
+ s = s + ' \\context Staff=%s \\%s\n' % (track, track)
+ elif track == 'trackA' and not global_options.mytime:
+ s = s + ' \\context Staff=trackA << \\trackA \\\\ \\trackB >>\n'
+ A = 1
elif item and item.__class__ == Text:
s = s + ' \\context Lyrics=%s \\%s\n' % (track, track)
i += 1
- s = s + ' >>\n}\n'
+ s = s + ' >>\n \\layout{}\n \\midi{}\n}\n'
progress (_ ("%s output to `%s'...") % ('LY', out_file))
@@ -853,40 +927,56 @@ def get_option_parser ():
help=_ ("print absolute pitches"))
p.add_option ('-d', '--duration-quant',
metavar= _("DUR"),
- help=_ ("quantise note durations on DUR"))
+ help=_ ("quantise note durations on DUR"),
+ default='16')
p.add_option ('-e', '--explicit-durations',
action='store_true',
help=_ ("print explicit durations"))
p.add_option("-h", "--help",
- action="help",
- help=_ ("show this help and exit"))
- p.add_option('-k', '--key', help=_ ("set key: ALT=+sharps|-flats; MINOR=1"),
- metavar=_ ("ALT[:MINOR]"),
- default='0'),
+ action="help",
+ help=_ ("show this help and exit"))
+ p.add_option('-k', '--key',
+ help=_ ("set key: ALT=+sharps|-flats; MINOR=1"),
+ action = 'store',
+ dest = 'mykey',
+ metavar=_ ("ALT[:MINOR]"),
+ default = '0')
p.add_option ('-o', '--output', help=_ ("write output to FILE"),
metavar=_("FILE"),
action='store')
- p.add_option ('-s', '--start-quant',help= _ ("quantise note starts on DUR"),
- metavar=_ ("DUR"))
- p.add_option ('-t', '--allow-tuplet',
+ p.add_option ('-P', '--partial',
+ metavar =_ ("DUR"),
+ action = "store",
+ dest = "mypartial",
+ help =_ ("insert global \"\\partial DUR\" upbeat"))
+ p.add_option('-s', '--start-quant',
+ help= _ ("quantise note starts on DUR"),
+ default = '16',
+ metavar= _ ("DUR"))
+ p.add_option ('--skip',
+ action = "store_true",
+ help =_ ("use s instead of r for rests"))
+ p.add_option('-t', '--allow-tuplet',
metavar=_ ("DUR*NUM/DEN"),
action = "append",
dest="allowed_tuplets",
- help=_ ("allow tuplet durations DUR*NUM/DEN"),
+ help= _ ("allow tuplet durations DUR*NUM/DEN"),
default=[])
+ p.add_option ('-T', '--time',
+ metavar =_ ("NUM/DEN"),
+ action = "store",
+ dest = "mytime",
+ help =_ ("insert global \"\\time NUM/DEN\" signature"))
p.add_option ('-V', '--verbose', help=_ ("be verbose"),
- action='store_true'
- ),
- p.version = "midi2ly (LilyPond) @TOPLEVEL_VERSION@"
+ action='store_true')
+ p.version = "midi2ly (LilyPond) %s" % ( program_version )
p.add_option("--version",
- action="version",
- help=_ ("show version number and exit"))
+ action="version",
+ help=_ ("show version number and exit"))
p.add_option ('-w', '--warranty', help=_ ("show warranty and copyright"),
- action='store_true',
- ),
+ action='store_true')
p.add_option ('-x', '--text-lyrics', help=_ ("treat every text as a lyric"),
action='store_true')
-
p.add_option_group (ly.display_encode (_ ("Examples")),
description = r'''
$ midi2ly --key=-2:1 --duration-quant=32 --allow-tuplet=4*2/3 --allow-tuplet=2*4/3 foo.midi
@@ -904,6 +994,10 @@ def do_options ():
opt_parser = get_option_parser()
(options, args) = opt_parser.parse_args ()
+ if options.warranty:
+ warranty ()
+ sys.exit (0)
+
if not args or args[0] == '-':
opt_parser.print_help ()
ly.stderr_write ('\n%s: %s %s\n' % (program_name, _ ("error: "),
@@ -913,11 +1007,8 @@ def do_options ():
if options.duration_quant:
options.duration_quant = int (options.duration_quant)
- if options.warranty:
- warranty ()
- sys.exit (0)
if 1:
- (alterations, minor) = map (int, (options.key + ':0').split (':'))[0:2]
+ (alterations, minor) = map (int, (options.mykey + ':0').split (':'))[0:2]
sharps = 0
flats = 0
if alterations >= 0:
@@ -927,10 +1018,9 @@ def do_options ():
options.key = Key (sharps, flats, minor)
-
if options.start_quant:
options.start_quant = int (options.start_quant)
-
+
options.allowed_tuplets = [map (int, a.replace ('/','*').split ('*'))
for a in options.allowed_tuplets]
@@ -944,9 +1034,8 @@ def main():
for f in files:
g = f
- g = strip_extension (g, '.midi')
- g = strip_extension (g, '.mid')
- g = strip_extension (g, '.MID')
+ for ext in [ '.midi','.mid','.MID' ]:
+ g = strip_extension (g, ext)
(outdir, outbase) = ('','')
if not global_options.output:
signature.asc
Description: Ceci est une partie de message numériquement signée
_______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
