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

Use chart inst offset for song resync + other resync fixes #3058

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Burgerballs
Copy link
Contributor

@Burgerballs Burgerballs commented Jul 23, 2024

resyncVocals() function in PlayState now makes use of instrumentalOffset.

This PR also halves the requirement for a resync, because 100ms is a big enough difference to greatly change timings in my personal opinion.

The resyncVocals() function now also resyncs the instrumental track the same way as the vocals.

Resyncing is checked every beat instead of every step.

This PR eliminates any and all resyncing issues players are plagued with, below is a before and after;

Before:

2024-07-23.19-06-07.mp4

After:

2024-07-23.19-08-21.mp4

@Hundrec
Copy link
Contributor

Hundrec commented Jul 24, 2024

This finally fixes #2888. Yay!

@NotHyper-474
Copy link
Contributor

Doesn't #2706 already do the desync fix?

@Burgerballs
Copy link
Contributor Author

Burgerballs commented Jul 24, 2024

Doesn't #2706 already do the desync fix?

I tried the pr and it doesn't fix the desync issue, this pr makes other changes in order to fix that

@Cartridge-Man
Copy link

LGTM

@EliteMasterEric EliteMasterEric added type: minor bug Involves a minor bug or issue. status: accepted Approved for contribution. If it's not already merged, it may be merged on a private branch. medium A medium pull request with 100 or fewer changes labels Jul 25, 2024
@EliteMasterEric EliteMasterEric added this to the 0.5.0 milestone Jul 25, 2024
@EliteMasterEric
Copy link
Member

Worked well for me when I tried it, and since it works for a bunch of other reviewers I'd say it's good.

@Sword352
Copy link

Any reasons the tracks are being paused though? You can just set the sound's time and move on

@VirtuGuy
Copy link

Any reasons the tracks are being paused though? You can just set the sound's time and move on

You can't just set the sound's time and move on without pausing as that would cause issues getting both the instrumental and vocals synced.

@Burgerballs
Copy link
Contributor Author

Burgerballs commented Jul 26, 2024

Any reasons the tracks are being paused though? You can just set the sound's time and move on

Pausing both tracks at the same time stops the time variable from increasing while you're setting it, making it easier to resync, and makes it more accurate.

@EliteMasterEric
Copy link
Member

Pausing both tracks at the same time stops the time variable from increasing while you're setting it, making it easier to resync, and makes it more accurate.

I was under the impression FlxSound.time only increases during the update() function.

@Burgerballs
Copy link
Contributor Author

Pausing both tracks at the same time stops the time variable from increasing while you're setting it, making it easier to resync, and makes it more accurate.

I was under the impression FlxSound.time only increases during the update() function.

The time variable does change during the update function but it doesn't really increase if paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium A medium pull request with 100 or fewer changes status: accepted Approved for contribution. If it's not already merged, it may be merged on a private branch. type: minor bug Involves a minor bug or issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.