Skip to content

Conductor fixed #4334

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

Closed
wants to merge 4 commits into from
Closed

Conversation

Lasercar
Copy link
Contributor

@Lasercar Lasercar commented Mar 16, 2025

Does this PR close any issues? If so, link them below.

Fixes #4332, fixes #4304, fixes #4176

Briefly describe the issue(s) fixed.

Fix for the conductor - fixes the stuttering, the conductor updating when not focused, the song not ending properly, and the playspeed not being used issue.

There's a weird thing where GF's animation jumps when you spam pause and unpause with this but I don't really see a problem in that.

There's an interesting issue where GF's neck snaps at high framerates when you spam pause and unpause when the music's time is passed into the conductor instead (which only happens in this PR and the playspeed fix, when the pitch is different so that it's actually used). Basically, GF's dance breaks somehow and it snaps between the left and right dances, as if the onStepHit is being called too many times. Though weirdly, it doesn't affect characters (or just boppers in general) that don’t have dance left and right animations.

Include any relevant screenshots or videos.

@github-actions github-actions bot added status: pending triage Awaiting review. size: medium A medium pull request with 100 or fewer changes. pr: haxe PR modifies game code. labels Mar 16, 2025
@Lasercar Lasercar changed the base branch from develop to main March 16, 2025 11:52
@Lasercar Lasercar changed the base branch from main to develop March 16, 2025 11:52
@Lasercar Lasercar marked this pull request as ready for review March 16, 2025 11:52
@Lasercar Lasercar changed the base branch from develop to main March 16, 2025 11:57
@Lasercar Lasercar changed the base branch from main to develop March 16, 2025 11:57
@Hundrec Hundrec added type: major bug Involves a major bug, including crashes, softlocks, or issues blocking progression status: needs revision Cannot be approved because it is awaiting some work by the contributor. and removed status: pending triage Awaiting review. labels Mar 16, 2025
Copy link
Collaborator

@Hundrec Hundrec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a weird thing where GF's animation jumps when you spam pause and unpause with this but I don't really see a problem in that.

This is a problem.

what.did.they.do.to.you.mp4

@Lasercar
Copy link
Contributor Author

Lasercar commented Mar 16, 2025

There's a weird thing where GF's animation jumps when you spam pause and unpause with this but I don't really see a problem in that.

This is a problem.
what.did.they.do.to.you.mp4

I'll keep the other two PRs open, in that case.

Hmm, I guess it doesn't hurt to use the conductor's position for now, if the pitch is 1, until a fix is found or whatever.

@Lasercar Lasercar force-pushed the conductor-fixed branch 2 times, most recently from 459dacb to 7dee307 Compare March 16, 2025 12:30
@Lasercar Lasercar requested a review from Hundrec March 16, 2025 13:47
@Lasercar
Copy link
Contributor Author

Lasercar commented Mar 16, 2025

Issue also happens on 0.5.3 with the changes:

2025-03-17.00-03-23.mp4

So, it isn't the result of any of the changes currently on develop.

@Lasercar
Copy link
Contributor Author

Hmm, now that fix for the song ending bug has been merged, will these changes still be needed or will #4182 suffice?

@github-actions github-actions bot added size: small A small pull request with 10 or fewer changes. and removed size: medium A medium pull request with 100 or fewer changes. labels Mar 18, 2025
@Lasercar Lasercar requested a review from KoloInDaCrib March 18, 2025 06:12
Copy link
Contributor

@KoloInDaCrib KoloInDaCrib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recent changes actually made the game unplayable since now it checks for whether or not the game is out of focus for it to update the conductor

In my opinion the if-cases should be something like this
image

In this if-case (admittedly a bit hectic) it checks:

  • If the game is focused on while autoPause is activated

OR

  • autoPause is deactivated (the game updates all the time, regardless if the game window is focused on or not)

However, considering flixel already internally handles the update() function and when it should be called, this PR is essentially just your other PR that implements playbackSpeed, which makes this PR somewhat pointless. (unless you can somehow fix the conductor skipping forward that doesn't cause gf to skip to her next animation and therefore snap her neck - I didn't have that much success by using the justUnpaused variable but you may have something else in mind for this)

One last thing since I may not have a chance to mention it again - I don't think this pr or any of its previous versions "fixed" constant resyncing, as I think it was fixed by this commit which was merged to develop a while ago and that the other person who attempted to fix the issue themselves probably hadn't tested the develop branch to see if it was already fixed

@Lasercar Lasercar requested a review from KoloInDaCrib March 19, 2025 00:25
@Hundrec Hundrec added status: pending triage Awaiting review. and removed status: needs revision Cannot be approved because it is awaiting some work by the contributor. labels Apr 1, 2025
@Hundrec Hundrec added type: minor bug Involves a minor bug or issue. and removed type: major bug Involves a major bug, including crashes, softlocks, or issues blocking progression labels Apr 13, 2025
@Lasercar Lasercar requested a review from KoloInDaCrib April 29, 2025 10:22
@Hundrec
Copy link
Collaborator

Hundrec commented May 12, 2025

This PR needs a rename, but I'm not really sure what to call it!
Does anyone have any ideas?

@EliteMasterEric EliteMasterEric added size: small A small pull request with 10 or fewer changes. and removed size: small A small pull request with 10 or fewer changes. labels May 14, 2025
@Lasercar
Copy link
Contributor Author

This PR needs a rename, but I'm not really sure what to call it! Does anyone have any ideas?

#5107

@Lasercar Lasercar closed this May 17, 2025
@Hundrec Hundrec added status: rejected Issue did not pass review or PR cannot be approved. and removed type: minor bug Involves a minor bug or issue. status: pending triage Awaiting review. pr: haxe PR modifies game code. labels May 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: small A small pull request with 10 or fewer changes. status: rejected Issue did not pass review or PR cannot be approved.
Projects
None yet
4 participants