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

improved conductor time updating in playstate #2405

Closed
wants to merge 2 commits into from

Conversation

Snirozu
Copy link

@Snirozu Snirozu commented May 9, 2024

this pr should make note movement smoother on screens that have a higher refreshing rate than 60FPS

@biomseed
Copy link

biomseed commented May 10, 2024

this pr should make note movement smoother on screens that have a higher refreshing rate than 60FPS

is this why things feel slightly delayed but the input feels fine? if so this should be merged fast

@Snirozu
Copy link
Author

Snirozu commented May 10, 2024

this pr should make note movement smoother on screens that have a higher refreshing rate than 60FPS

is this why things feel slightly delayed but the input feels fine? if so this should be merged fast

i don't think if there are any issues with delayed visuals, but if so then they are on your end, there is an option in the game to set the offsets for visuals so maybe you have set them to a wrong value

but about this pr, it uses the elapsed time between game frames instead of the sound time which is very inconsistent (i don't know how sound time from lime/flixel gets updated so don't take my words lol)

@ninjamuffin99
Copy link
Member

would you be able to make a short before/after demo video of differences?

@ninjamuffin99 ninjamuffin99 added the status: needs clarification Requires more info from the contributor. label May 15, 2024
@Snirozu
Copy link
Author

Snirozu commented May 15, 2024

it's probably not noticeable here because obs may have done something with the framerate and i don't want to configure it twice but here are the recorded clips

before.-.Trim.mp4
pr.-.Trim.mp4

@KutikiPlayz
Copy link

my obs records at my actual monitor refresh rate, and I'm playing a song with a higher scroll speed so it should be more noticeable here

before-1.mp4
after-1.mp4

I've got no other changes to my build besides this pr (in fact it's still 0.3.2)

EnterTheVoid-x86 added a commit to EnterTheVoid-x86/Funkin that referenced this pull request May 24, 2024
@gamerbross
Copy link
Contributor

gamerbross commented May 26, 2024

in resyncVocals function, you dont need a time for argument in FlxG.sound.music.play,
FlxG.sound.music.play(Conductor.instance.songPosition);
can be
FlxG.sound.music.play(Conductor.instance.songPosition);
because if the sound is paused, this argument is ignored

	public function play(ForceRestart:Bool = false, StartTime:Float = 0.0, ?EndTime:Float):FlxSound
	{
		if (!exists)
			return this;
			
		if (ForceRestart)
			cleanup(false, true);
		else if (playing) // Already playing sound
			return this;
			
		if (_paused) // here, it just uses StartTime when the sound is not paused
			resume();
		else
			startSound(StartTime);
			
		endTime = EndTime;
		return this;
	}

(+ please we need this to get merged, its awesome)

@EliteMasterEric
Copy link
Member

We looked at this and can't really see the difference here, and the way this changes the Conductor to be no longer based on the FlxSound object makes me concerned about desyncing the song.

Rejected.

@gamerbross
Copy link
Contributor

The problem of using music time is visible in higher refresh rates, in 165hz or even 144hz I can clearly notice a difference, it's so much smoother. Probably this wasn't the best implementation but should get changed so elapsed is used instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs clarification Requires more info from the contributor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants