Skip to content
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

BPM change functionality & implementation #382

Merged
merged 6 commits into from
Feb 12, 2021
Merged

BPM change functionality & implementation #382

merged 6 commits into from
Feb 12, 2021

Conversation

PrincessMtH
Copy link
Contributor

Tweaks changeBPM functionality so that the BPM is not reset back to the original after having been changed.

Worth noting is that this may cause inconsistencies with the chart editor, particularly in the function recalculateSteps().
A potential fix for this could be done by doing this for its for loop instead:

var bpm:Int = _song.bpm;

for (i in 0...curSection)
{
	steps += 16;

	if (_song.notes[i].changeBPM)
		bpm = _song.notes[i].bpm;
	timeShit += (((60 / bpm) * 1000) / 4) * 16;
}

But the chart editor scares me so I do not touch it, not that there are many songs I could test with.

@ILuvGemz
Copy link

ILuvGemz commented Feb 9, 2021

It doesn't actually work. I tried merging it and it says some variables are missing. What should I do? I am new to this.

@PrincessMtH
Copy link
Contributor Author

After some more testing, I think this requires extra work before it can be merged. Not because the proposed change causes any new issues, but because the rest of the existing system for changing BPM is not yet functional.

Changing the BPM also updates the Conductor's crochet, which is used along with the absolute song position to calculate the current step/beat etc. (curStep, totalSteps, totalBeats). If the BPM changes, the calculation will suddenly be very different, and while the BPM will appear to change visually, it's most likely misaligned with the actual beats of the song. This also messes up calculations for what the current section is, which can eventually lead to the game trying to read from sections that do not exist, and thus crashing.

The solution here is to keep track of when the BPM changes and work relative to the last change. Still thinking about what the best way to do this could be. Perhaps BPM changes should be mapped out on song load? Keeping track of changes as they happen could be impacted by lag, and keeping track of it within the Conductor class is impractical as it doesn't ever seem to reset. Regardless, something is gonna require a bit of an overhaul. Ideas are welcome!

@PrincessMtH PrincessMtH marked this pull request as draft February 11, 2021 01:16
@ILuvGemz
Copy link

Thank you. This is the EXACT issue I have been suffering from.

trims off unnecessary data, supports bpm changes proper and according to my proposed system, also just happens to fix other issues with the chart editor
@PrincessMtH
Copy link
Contributor Author

In making the ChartingState compatible with this new BPM system I ended up overhauling it a bit more than I intended. Here's a quick demo of how it fared with a rechart of Monster with proper BPM changes (see below):

Demonstration video

The strumLine follows the tempo nicely and does not typically escape the bounds of the grid. Starting playback anywhere in the middle of the track isn't an issue either. Not much testing done with actual chart editing though, but placing notes seems to work just fine. After setting the BPM for a section, you may need to go back and forth to the previous one to have it reflect the new BPM. Vocals also stop playing after the song loops. Still figuring those out.
PlayState needs some extra changes too. Stuff like the screen zooms and GF dancing seem to update but desync somewhere. Still gotta look into that. So far so good, though!
(monster rechart here)

@ILuvGemz
Copy link

Hello! I’m sorry to be rude, but can you post it as a mod when you’re done? If not, I understand.

@PrincessMtH
Copy link
Contributor Author

In-game events (animations, fx etc.) now sync properly even after BPM has changed.
Chart editing requires more testing before I'm confident it can be merged, but I suppose that's what these things are reviewed for, so I'll open it up again. Should fix #183, though.

Hello! I’m sorry to be rude, but can you post it as a mod when you’re done? If not, I understand.

No problem! If you mean the Monster chart, I suppose I already have in the comments here. It kind of depends heavily on the BPM changes, so unless the code gets merged, I don't think I'll release it anywhere else.

@PrincessMtH PrincessMtH changed the title tweak changeBPM functionality BPM change functionality & implementation Feb 12, 2021
@PrincessMtH PrincessMtH marked this pull request as ready for review February 12, 2021 01:15
@ILuvGemz
Copy link

That’s cool! I wanted a version of FNF with changeBPM (under review) and section length changing (which I know you haven’t finished) so much, to the point where I put the entire thing on a 225 GameBanana point bounty! (If the section length is changeable, I would give it to you per request)

If you do not want to continue after the ChangeBPM, then I understand.

@PrincessMtH
Copy link
Contributor Author

Video demonstration of the Monster map (above). Visual quirks like the camera zoom, background elements (philly train/lights, halloween lightning etc.) all sync more accurately after the latest change.

That’s cool! I wanted a version of FNF with (...) section length changing

In the end, anything is possible, but a lot of the game's logic is built around having 4 steps in a beat and 4 beats in a section, regardless of what lengthInSteps wants you to believe. So it would just require a lot of work, and lengthInSteps would probably have to be replaced with some kind of time signature equivalent so the game knows how long a beat and a step are individually.

@ninjamuffin99
Copy link
Member

REAL GANGSTER HOURS MUTHUFUKIN BLES UP GOT BLESS U MTH

@ninjamuffin99 ninjamuffin99 merged commit 57359cf into FunkinCrew:master Feb 12, 2021
@ILuvGemz
Copy link

May I distribute the compiled .exe with changeBPM?

1 similar comment
@ILuvGemz
Copy link

May I distribute the compiled .exe with changeBPM?

@ninjamuffin99
Copy link
Member

May I distribute the compiled .exe with changeBPM?

hmm if u want yeah, although later today I WILL update game with a 0.2.7.1 update if u wanted to wait for that hehehe

kudorado pushed a commit to kudorado/Funkin that referenced this pull request Jul 18, 2021
BPM change functionality & implementation
ninjamuffin99 added a commit that referenced this pull request Mar 9, 2023
BPM change functionality & implementation
@JokeBambi
Copy link

epic

@EliteMasterEric EliteMasterEric added this to the Legacy milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants