Set any 0 bpm segments to the previous segment's bpm

[niet-dev]

When I loaded a chart with bpms 170 -> 0 -> 170 in 0.70, the chart was treated as having a constant 170 bpm. To avoid timestamps going to infinity, we can treat 0 bpm segments as having the same bpm as the previous segment. Charts with bpms.size > 1 will always have a previous segment to draw from.

Closes #946

[poco0317]

editing this function can have a lot of consequences. in this case, it can change calc output. it probably doesnt break much though. need to see whether or not it is possible for 2 consecutive segments to have 0 bpm, which would make this fail

[niet-dev]

Sorry for my ignorance, but what would trigger the creation of a new segment when the bpm remains the same? I tested adding fakes and a stop in the middle of a 0 bpm section of a test file, but neither seemed to make a new segment.

Alternatively, if there's another direction you'd like me to go that won't affect the calc output, I'd be happy to try my hand at that instead.

[poco0317]

in SMLoader::ProcessBPMsAndStops the bpms are read from the bpm string in the .sm/.ssc and without looking at that code directly i was not sure if it would interpret 2 consecutive sections of equivalent bpm as 2 segments. it isnt really possible at least in AV to do this. but the way to do it and also the way this 0 bpm thing came up was by editing the .sm in notepad and disregarding that

as for the calc output thing it isnt as important in this case because its solving a NAN/INF issue which should never come up in the first place

[niet-dev]

Oh I see, thank you for the clarification!

I added a new 0 bpm section in notepad like so:

0.000=159.206
,44.000=0.000
,48.000=0.000
,52.000=159.206

And I still get 3 BPMSegments when debugging. As far as I can tell, it treats 2 consecutive 0 bpm sections as the same segment.

[niet-dev]

In SMLoader::ParseBPM:

if (fNewBPM == 0) {
//          LOG->UserLog(
//            "Song file", this->GetSongTitle(), "has a zero BPM; ignored.");
//          continue;

Uncommented the continue.

[poco0317]

this is more in line with our philosophy. blame the author of the chart

[poco0317]

now i dont think that outputs the actual path to the song but it would be nice if it did (so someone would know what pack it is in)

[niet-dev]

ParseBPMs does not seem to have access to the path directly, as SMLoader only stores the file name and extension. I can pass the path in via a parameter, but I'm not sure if this is a bad move.

[bluebandit21]

ParseBPMs only has one caller -- that seems fine to me?

Alternatively, you could have parseBPMs throw a C++ exception and catch the exception higher up at a level where something would have access to that info. That might actually lead to a cleaner design overall if this kind of "detect a problem and log an error" pattern was used in more places than just this :)

[niet-dev]

Alternatively, you could have parseBPMs throw a C++ exception and catch the exception higher up at a level where something would have access to that info.

This initially sounded like a good idea to me; I always feel a bit weird about changing the signature of a function unless I really know what I'm doing. However, I don't think throwing an exception would be a good choice in this case because we want the function to keep parsing the rest of the file when it finds a 0 bpm entry, rather than stopping at that point.

I think I can do something a bit cleaner, though, by just adding the path as an attribute of SMLoader instead. ParseBPMsAndStops has a precondition of no BPM change or stop having a 0 value, and ParseStops has a very similar line for logging, so this route would save having to change at least 2 function signatures.