Add song BPM check in NotesLoaderSSC

[EDuToit]

Make sure that a BPMS was in the header after processing steps to prevent the calculated defaulting to 60 when DisplayBpm is calculated.

This is my first commit to a open source project any comments will be appreciated , Hope I can become a regular.

fixes #615

[nico-abram]

Looks good to me. I'm not sure if this is enough to close the issue. Wouldn't we need to correctly update the bpm in the song select screen when changing the chart?

[EDuToit]

I have a theory that the second part (not visually changing as the user change charts) is lua related due to the fact that I did not change the flag that specifies that the chart/steps contains its own timing and that all the chart specific bpm values are calculated and stored, the functions to retrieve the BPM is also in the LunaSteps class. I have not yet looked in to how Lua handles the data.

[poco0317]

I believe the BPM display on the screen is a particular class that should already properly react to this. I will test soon.

[EDuToit]

I am not sure how relevant this might be but its the reason I did not originally try to fix the bpm issue on my first commit.

The charts in question will also be picked up as broken in StepMania, not saying that is a reason we should not fix it but the tags were not used as in the ssc spec?

BPM should technically only be used in the header or do I have this wrong. (in the files in question) there were multiple declarations of the same bpm sets, but this only had the effect of redundant calculations as the value changed were betwean 190.04 and 189.991 and had 187 iterations in the sample file I used !PiXeLaTeD! ... I feel this adds redundant loading times as the process of calculating the final BPM had to iterate over all the values 187 times with no significant change and do this for each Difficulty.

If I can get a sample from someone that have variable BPM per Difficulty that makes more sence than the provided issue's example.

[EDuToit]

I believe the BPM display on the screen is a particular class that should already properly react to this. I will test soon.

If you don't see anything obvious, I will like to have a go at some LUA

[poco0317]

This https://github.com/etternagame/etterna/blob/425f10cfd58473754bba9021fc6f441e1accaefc/src/Etterna/Actor/Menus/BPMDisplay.cpp

[EDuToit]

When I got to the problem I debugged through this class to find the root cause. I see the set bpm from steps but cant remember off te top of my head of it was called at all.

Will investigate when I am home.

[EDuToit]

I looked in to the issue of only get BPM from song being called and my memory was correct. Even tho the flags are set that specifies that the step / chart contains its own BPM the only call for BPM on displaying the values in the song manager is to the get BPM from song.

This is however my findings by just using breakpoints and will have to investigate the LUA code to be sure of my temporary conclusion.

[EDuToit]

I feel quite lost with the LUA part I am mentioning, expecting to find some sort of file with a lua extension, I do however feel to state that I am quite unfamiliar with the combination of LUA scripts and c++

I read the docks on LUA and see that everything is an Actor and followed down the hierarchy to find that the BPMDisplay is in deed also an actor (with some text as a intermediary node) so excuse my ignorance of LUA as a term I was throwing around.

I also saw that the default visual studio folder view is really bad to see the architecture and after changing that, I might be able to reason a bit better about the code base. (bare with me as I set up the environment and settle in)

[EDuToit]

I investigated a bit further but only realized (with the better view of the architecture) that I need to do more research on how the drawing of the BPM is instantiated.

My thought are that the issue might be completely a front-end issue as I am quite sure that the correct information is calculated.

Another interesting aspect I saw was that the tag being set in the ssc parser of "contains own timing data" is completely localized to the ssc parser. When I have more time I will look in to the composition phase of the BPM per song and how its changed as I suspect that the issue might be with the theme, that is if my literal understanding of

Lua is used in Etterna for Themes and Noteskins.

is correct.

The original pr will then just allow for themes (going with my possible ignorant view of the them-ing process, that I have no clue on how much freedom in choosing what values are presented exist) to display more accurate BPM values if a less valid ssc file is presented

[poco0317]

The SSC notesloader is meant to support BPMs which are specific to a chart, and that is why the header BPM may not be totally necessary.
Your change compiles fine and as far as I can tell seems to fix the problems.
BPMDisplay holds a method which allows setting the bpm based on the given Steps, not just the Song. This is also exposed to Lua. After review, Til Death does in fact not acknowledge this.

Relevant code:

etterna/Themes/Til Death/BGAnimations/ScreenSelectMusic decorations/wifeTwirl.lua

Lines 383 to 397 in cce9555

	Def.BPMDisplay {
	File = THEME:GetPathF("BPMDisplay", "bpm"),
	Name = "BPMDisplay",
	InitCommand = function(self)
	self:xy(capWideScale(get43size(384), 384) + 62, SCREEN_BOTTOM - 100):halign(1):zoom(0.50)
	end,
	MortyFartsCommand = function(self)
	if song then
	self:visible(true)
	self:SetFromSong(song)
	else
	self:visible(false)
	end
	end
	},

With access to a Steps object like we have from that file (the file holds a global called steps) we could just do self:SetFromSteps(steps) and call it a day within a Command that runs whenever the steps change. Well that doesn't work. It seems that these changes don't fix that issue.
I am too lazy to investigate why this is, but it would appear that every Steps defaults to the BPM of the first Steps. Either that, or no BPM is saved in the Steps itself.

In conclusion, your changes are the right first step to fixing this.

[nico-abram]

What's the status of this?

@EDuToit are you fine with us merging this as-is, or would you rather we wait?

[EDuToit]

I am happy with it as-is,
It did solve the problem and the songs with variable bpm in the pack mentioned by the op, shows as variable in song select with this fix.