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?

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

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".

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 )

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.

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!

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".)

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.

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

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
end

Though 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) # true

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.