-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set any 0 bpm segments to the previous segment's bpm #1296
Conversation
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 |
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. |
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 |
Oh I see, thank you for the clarification! I added a new 0 bpm section in notepad like so:
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. |
In
Uncommented the |
this is more in line with our philosophy. blame the author of the chart |
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) |
|
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. |
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 |
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