Bpm #29
Bpm #29
Conversation
|
ex2.mid is the only midi file I found on the internet with different tempos. Testing was quite limited by using that file alone. |
|
@Fukurokudzu thank you so much for this PR! I've just started a new job, so I apologize in advance because it might take me a bit of time to review this. Thank you for this contribution. |
|
@jimm thanks and congrats with new job! I'm new to ruby, so I might use that time for some further editing :] |
There was a problem hiding this comment.
@Fukurokudzu Thank you so much for this PR! I'm sorry that it has taken me so long to review it --- I just started a new job recently.
Please let me know what you think about my comments and questions.
lib/midilib/sequence.rb
Outdated
| end | ||
|
|
||
| # Returns bpm value for offset, nil if offset is out of range | ||
| def beats_per_minute_current(offset = 0) |
There was a problem hiding this comment.
When first reading this code, I found myself wondering what "offset" was --- an event index? A time from start? A measure number?
Might the code be more easily understandable if "offset" is called "time_from_start" everywhere in this PR?
There was a problem hiding this comment.
I took "offset" idea from this discussion. For me personally both offset and time from start are ok'ish, but offset is a bit shorter. I'll change it to time_from_start then
lib/midilib/sequence.rb
Outdated
| end | ||
|
|
||
| # Returns bpm value for offset, nil if offset is out of range | ||
| def beats_per_minute_current(offset = 0) |
There was a problem hiding this comment.
With this method, do we even need the original beats_per_minute method any more? I think it might be a good idea to get rid of it and name this method beats_per_minute. After all, if offset == 0 then the same value will be returned.
If we don't do that, then how about renaming this beats_per_minute_at instead of beats_per_minute_current? To me "current" implies that something like a clock knows what the "current" time from start is that you're asking for, whereas "at" means "you need to tell me where you look to get the bpm".
There was a problem hiding this comment.
It's my first oss commit, so my main goal was to make changes as safe as possible. I'm agree with you in both cases, so let's do it the way you think is better )
lib/midilib/sequence.rb
Outdated
| end | ||
|
|
||
| # Returns array with all tempos parts within sequence | ||
| def beats_per_minute_all |
There was a problem hiding this comment.
What do you see as the use case for this method? I can see asking for BPM at time x but I'm not sure when I would need a list of all of the BPM values without associated times.
There was a problem hiding this comment.
I've imagined a playlist for a DJ with some additional info about bpm, but you're right, it might not be very useful. If you think it's better to get rid of it, let's do that!
lib/midilib/sequence.rb
Outdated
| @@ -79,6 +81,13 @@ def pulses_to_seconds(pulses) | |||
| (pulses.to_f / @ppqn.to_f / beats_per_minute) * 60.0 | |||
| end | |||
|
|
|||
| # The same with offset. Returns nil if out of range | |||
| def pulses_to_seconds_current(pulses, offset = 0) | |||
There was a problem hiding this comment.
I have the same thoughts as I do for beats_per_minute_current below: do we still need pulses_to_seconds any more?
(Same comment about the name "offset" too --- I think it should be "time_from_start".)
Thanks a lot for your time to review those! |
|
|
||
| event = @tracks.first.events.detect { |e| e.is_a?(MIDI::Tempo) } | ||
| event ? Tempo.mpq_to_bpm(event.tempo) : DEFAULT_TEMPO | ||
| return nil if time_from_start > self.get_measures.last.end || time_from_start < 0 |
There was a problem hiding this comment.
I would almost in these cases get the first known tempo and last known tempo instead of returning nil here, such that:
- If the time offset is greater than the song duration return the last registered tempo
- If the time offset is less than zero return the first registered tempo
...otherwise downstream code will have to deal with the potential for nil.
| else | ||
| current_bpm = part[1] if part[0] <= time_from_start | ||
| end | ||
| end |
There was a problem hiding this comment.
This may be easier to work with if you use time ranges below instead, then you might be able to do something like this:
_, found_tempo = tempo_ranges.find { |range, _| range.cover?(time_from_start) }
found_tempo || DEFAULT_TEMPO...such that:
tempos = { 0..5 => 120, 5..20 => 100, 20..30 => 180 }
time_from_start = 10
_, found_tempo = tempos.find { |range, _| range.cover?(time_from_start) }
found_tempo
# => 100
| end | ||
| end | ||
| tempo_parts.transform_values! { |bpm| bpm.round(BPM_ROUND) } | ||
| end |
There was a problem hiding this comment.
Playing with the above idea of using ranges instead:
# New constant to work with for no events / no tempos
DEFAULT_TEMPO_RANGE = { (0..) => DEFAULT_TEMPO }
def get_tempo_parts
# We can get all the events, then use `grep` to find all of them that are MIDI Tempos
# and get them into the format we need. Coincidentally if you stop it here it'll do the
# same as the above.
tempo_events = (@tracks || [])
.flat_map(&:events)
.grep(MIDI::Tempo)
.map { |t| [t.time_from_start, Tempo.mpq_to_bpm(t.tempo).round(BPM_ROUND)] }
return DEFAULT_TEMPO_RANGE unless tempo_events.any?
# We do not have a tempo event at the start
tempo_events << [0, DEFAULT_TEMPO] if tempo_events.first.first != 0
tempo_ranges = {}
# This is a nice trick, group every 2 consecutive tempos
tempo_events.each_cons(2).each do |first_tempo, second_tempo|
first_tempo_start, first_tempo_bpm = first_tempo
second_tempo_start, second_tempo_bpm = second_tempo
# Note using `...` here which means up to and excluding
tempo_ranges[first_tempo_start...second_tempo_start] = first_tempo_bpm
end
last_tempo_start, last_bpm = tempo_events.last
tempo_ranges[last_tempo_start..] = last_bpm
tempo_ranges
endThough I don't think this necessarily needs to be a Hash.
each_cons is nifty for things like this:
[1,2,3,4].each_cons(2).to_a
=> [[1, 2], [2, 3], [3, 4]]...along with ... which is "up to but excluding" for Range:
(0...5).cover?(5) # false
(0..5).cover?(5) # trueThere was a problem hiding this comment.
Then again there's an edge there on what happens if those tracks have BPMs distinct to them that happen to not be in the same order, but not sure if that even makes sense.
Hey, Jimm! Pls have a look at this PR. Not sure it closes #8, but should make life a bit easier, I guess. Thanks!