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

End the funking song I'm going insane #4330

Conversation

Lasercar
Copy link
Contributor

@Lasercar Lasercar commented Mar 16, 2025

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

Fixes #4304, (finally) fixes #3142

Briefly describe the issue(s) fixed.

#4309 (comment)

@Hundrec

Has found the way to replicate the bug - when the song ends, click off the window, then click back into it - what ends up happening is that you bypass music's oncomplete function (because the music still plays even when you've clicked off) and so, the song doesn't end.

This also means that once the music playing when unfocused haxe issue is fixed, it should fix this issue too (but then again, maybe keep this in just in case it's also caused by something else?).

Include any relevant screenshots or videos.

@Lasercar Lasercar changed the base branch from main to develop March 16, 2025 06:41
@github-actions github-actions bot added status: pending triage Awaiting review. size: small A small pull request with 10 or fewer changes. pr: haxe PR modifies game code. and removed size: small A small pull request with 10 or fewer changes. labels Mar 16, 2025
@Hundrec Hundrec added size: small A small pull request with 10 or fewer changes. type: major bug Involves a major bug, including crashes, softlocks, or issues blocking progression labels Mar 16, 2025
@Lasercar Lasercar force-pushed the end-the-fucking-song-I'm-going-insane branch from 1d06406 to d9fcb8c Compare March 16, 2025 06:45
@Lasercar
Copy link
Contributor Author

[UH-OH!] yeah!
Screenshot 2025-03-16 164813

@Lasercar Lasercar force-pushed the end-the-fucking-song-I'm-going-insane branch from d9fcb8c to 3fce6d5 Compare March 16, 2025 06:50
@KoloInDaCrib
Copy link
Contributor

KoloInDaCrib commented Mar 16, 2025

Your version does not fix the original issue.

2025-03-16.08-16-25.mp4

For your information, I am not running anything else besides your pr, otherwise there would be visible changes.

image

I've even limited myself to using your preferred FPS cap of 80 without any offsets.

I honestly have a feeling you are purposefully sabotaging my pr to get yours merged and mine canned.

@JackXson-Real
Copy link

Your version does not fix the original issue.

2025-03-16.08-16-25.mp4

For your information, I am not running anything else besides your pr, otherwise there would be visible changes.

image

I've even limited myself to using your preferred FPS cap of 80 without any offsets.

I honestly have a feeling you are purposefully sabotaging my pr to get yours merged and mine canned.

He’s just trying to help. Apparently your PR hasn’t fixed it either, so at this point a fix for this bug is fair game. We all want this bug to be fixed, don’t assume someone is doing something to sabatoge you, especially on GitHub, this is not a competition.

@KoloInDaCrib
Copy link
Contributor

He’s just trying to help. Apparently your PR hasn’t fixed it either, so at this point a fix for this bug is fair game. We all want this bug to be fixed, don’t assume someone is doing something to sabatoge you, especially on GitHub, this is not a competition.

I don't have a problem with him "taking my spot" or whatever, I mostly have a problem with him claiming that my pr doesn't fix the issue, even though I have proven it does. Besides, you only speak for his name, but have you tried both prs and judged them based or not they fix the original issue or not?

@Lasercar
Copy link
Contributor Author

Your version does not fix the original issue.
2025-03-16.08-16-25.mp4

For your information, I am not running anything else besides your pr, otherwise there would be visible changes.

I've even limited myself to using your preferred FPS cap of 80 without any offsets.

I honestly have a feeling you are purposefully sabotaging my pr to get yours merged and mine canned.

Look, I did say in your PR that

If this doesn't end up fixing it, then maybe I'll have a go at fixing it too.

So I did? Was that the wrong thing to do?

And besides, because the bug can be reproduced on literally every fix everyone's made, it's basically fair game at this point.

IDK.

@Lasercar
Copy link
Contributor Author

Lasercar commented Mar 16, 2025

Oooh, I got again with my changes!

And my breakpoint on end song at if (event.eventCanceled) return; didn't fire, so something very funky is going on.

Ah, so the songPosition is being capped at the music music length, so all I need to do is add an equals (I hope).

@Lasercar Lasercar force-pushed the end-the-fucking-song-I'm-going-insane branch from 3fce6d5 to dc55f7d Compare March 16, 2025 08:01
@Lasercar Lasercar changed the title End the funking song i'm going insane End the funking song I'm going insane Mar 16, 2025
@Lasercar Lasercar changed the base branch from develop to main March 16, 2025 09:05
@Lasercar Lasercar changed the base branch from main to develop March 16, 2025 09:06
@trayfellow
Copy link

Funkin' github ranked

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.

Changes here integrated into #4309

@Hundrec Hundrec added status: reviewing internally Under consideration and testing. and removed status: pending triage Awaiting review. labels Mar 16, 2025
@Lasercar
Copy link
Contributor Author

Lasercar commented Mar 16, 2025

Superseded by #4334

(if you don't mind or can fix the weird issue hundrec found)

@Lasercar Lasercar closed this Mar 16, 2025
@Hundrec Hundrec added status: rejected Issue did not pass review or PR cannot be approved. and removed type: major bug Involves a major bug, including crashes, softlocks, or issues blocking progression status: reviewing internally Under consideration and testing. size: small A small pull request with 10 or fewer changes. pr: haxe PR modifies game code. labels Mar 16, 2025
@Lasercar Lasercar reopened this Mar 16, 2025
@github-actions github-actions bot added size: small A small pull request with 10 or fewer changes. pr: haxe PR modifies game code. labels Mar 16, 2025
@Hundrec Hundrec added type: major bug Involves a major bug, including crashes, softlocks, or issues blocking progression status: reviewing internally Under consideration and testing. and removed status: rejected Issue did not pass review or PR cannot be approved. labels Mar 16, 2025
@KoloInDaCrib
Copy link
Contributor

Noticed a bug with this while playtesting - if you have a positive offset, it will cause a null object reference to appear in the results screen (in the video I used visual offsets of 100ms)

2025-03-16.16-14-42.mp4

I assume this is because you use Conductor's songPosition variable (which has offsets accounted for)
image
which makes it so that endSong is called twice, and therefore the game moves to results screen twice (proven here)
image

In comparison my PR uses music's raw time (no offsets) and it the game works without any NORs

2025-03-16.16-03-50.mp4

In the future I recommend not using breakpoints, since by that you're limiting yourself to seeing what should happen instead of letting the game run to see if your changes have any unintended consequences

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.

See Kolo's comment above

@AbnormalPoof AbnormalPoof added status: needs revision Cannot be approved because it is awaiting some work by the contributor. and removed status: reviewing internally Under consideration and testing. labels Mar 16, 2025
@Lasercar Lasercar force-pushed the end-the-fucking-song-I'm-going-insane branch from dc55f7d to 324a19b Compare March 17, 2025 05:38
@Lasercar
Copy link
Contributor Author

Noticed a bug with this while playtesting - if you have a positive offset, it will cause a null object reference to appear in the results screen (in the video I used visual offsets of 100ms)
2025-03-16.16-14-42.mp4

I assume this is because you use Conductor's songPosition variable (which has offsets accounted for)

In comparison my PR uses music's raw time (no offsets) and it the game works without any NORs
2025-03-16.16-03-50.mp4

In the future I recommend not using breakpoints, since by that you're limiting yourself to seeing what should happen instead of letting the game run to see if your changes have any unintended consequences

Fixed it!

That was easy (and before you ask, no, I did not make the same changes as you):

2025-03-17.15-42-42.mp4

@Lasercar Lasercar requested a review from Hundrec March 17, 2025 05:45
@EliteMasterEric EliteMasterEric self-assigned this Mar 17, 2025
Copy link
Member

@EliteMasterEric EliteMasterEric left a comment

Choose a reason for hiding this comment

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

Gonna resolve the merge conflict with #4309 and merge this too.

@EliteMasterEric EliteMasterEric force-pushed the end-the-fucking-song-I'm-going-insane branch from 324a19b to 22b9174 Compare March 17, 2025 10:44
@github-actions github-actions bot added size: medium A medium pull request with 100 or fewer changes. and removed size: small A small pull request with 10 or fewer changes. labels Mar 17, 2025
@EliteMasterEric EliteMasterEric merged commit e83b88d into FunkinCrew:develop Mar 17, 2025
2 checks passed
@Lasercar Lasercar deleted the end-the-fucking-song-I'm-going-insane branch March 17, 2025 12:54
@AbnormalPoof AbnormalPoof added status: accepted PR was approved for contribution. If it's not already merged, it may be merged on a private branch. and removed status: needs revision Cannot be approved because it is awaiting some work by the contributor. labels Mar 17, 2025
@AbnormalPoof AbnormalPoof added this to the 0.6.0 (Pit Stop 2) milestone Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: haxe PR modifies game code. size: medium A medium pull request with 100 or fewer changes. status: accepted PR was approved for contribution. If it's not already merged, it may be merged on a private branch. type: major bug Involves a major bug, including crashes, softlocks, or issues blocking progression
Projects
None yet
7 participants