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

weekend1 rain in later songs now starts on stage start fix #115

Closed
wants to merge 1 commit into from

Conversation

Lasercar
Copy link

@Lasercar Lasercar commented Feb 1, 2025

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

FunkinCrew/Funkin#4078
Already fixed by this #9 PR

Briefly describe the issue(s) fixed.

In the other 2 songs of weekend 1, the rain doesn't start until the song starts. This fix provides passes the rainShaderStartIntensity for the rain's remappedIntensityValue instead of the songPosition if the it's less than zero.

Include any relevant screenshots or videos.

I noticed that AppleHair made a fix for that put the remappedIntensityValue update in a null check, so I've also done the same thing so that it's easier to merge? Idk if that's how it works.
If I need to make the change from a branch that starts with the community fixes let me know and I'll quickly fix this PR.

@Lasercar
Copy link
Author

Lasercar commented Feb 1, 2025

Hmm, I wonder if there's anything else I can fix/do...

Copy link

@AppleHair AppleHair left a comment

Choose a reason for hiding this comment

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

It's cool that you managed to figure out the solution on your own, but I made #9 8 months ago, and it does exactly the same thing. It was also already merged to the community fixes branch a while ago, so I don't see how this PR is necessary.

@Lasercar
Copy link
Author

Lasercar commented Feb 1, 2025

Hm? Your change fixes the story mode start, where the rain is at max strength until the first song starts.
This fixes the rain not appearing in the later songs before the song starts, though I can understand the confusion.

Oh, I see the problem, I wasn't clear enough with what it was I fixing.

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 1, 2025

@AppleHair Can you confirm that your PR resolves all issues with the rain, including the one linked from this PR?

@AppleHair
Copy link

Hm? Your change fixes the story mode start, where the rain is at max strength until the first song starts.
This fixes the rain not appearing in the later songs before the song starts, though I can understand the confusion.

Look at what #9 changes in the code and tell me how it's different from this PR

@Lasercar
Copy link
Author

Lasercar commented Feb 1, 2025

AppleHair's PR:
remappedIntensityValue = FlxMath.remapToRange(Conductor.instance.songPosition, 0, FlxG.sound.music.length, rainShaderStartIntensity, rainShaderEndIntensity);
to my PR:
remappedIntensityValue = FlxMath.remapToRange(Conductor.instance.songPosition > 0 ? Conductor.instance.songPosition : rainShaderStartIntensity, 0, FlxG.sound.music.length, rainShaderStartIntensity, rainShaderEndIntensity);

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 1, 2025

I took a glance on mobile, but couldn't verify whether the code was the same (or had the same effect) since I'm not a coder.
I only need confirmation that your solution resolves Funkin' issue #4078 before I mark this as a duplicate.

@Lasercar Lasercar changed the title weekend1 rain now starts on stage start fix weekend1 rain in later songs now starts on stage start fix Feb 1, 2025
@Lasercar
Copy link
Author

Lasercar commented Feb 1, 2025

Ah, ok I get it now.
So, AppleHair's change makes so that the the rain doesn't start until the song starts.
However, that introduces the issue I noticed, where the rain is absent until the song starts.

@Hundrec Hundrec added type: minor bug Involves a minor bug or issue. status: pending triage The bug or PR has not been reviewed yet. labels Feb 1, 2025
@Hundrec
Copy link
Collaborator

Hundrec commented Feb 1, 2025

Are you sure? The video in your bug report should have been on an unmodified build of 0.5.3, right?
AppleHair's change is coming to 0.6.0

@AppleHair
Copy link

AppleHair commented Feb 1, 2025

AppleHair's PR: remappedIntensityValue = FlxMath.remapToRange(Conductor.instance.songPosition, 0, FlxG.sound.music.length, rainShaderStartIntensity, rainShaderEndIntensity); to my PR: remappedIntensityValue = FlxMath.remapToRange(Conductor.instance.songPosition > 0 ? Conductor.instance.songPosition : rainShaderStartIntensity, 0, FlxG.sound.music.length, rainShaderStartIntensity, rainShaderEndIntensity);

Conductor.instance.songPosition > 0 will always be true when FlxG.sound.music != null (because the song is playing), so Conductor.instance.songPosition > 0 ? Conductor.instance.songPosition : rainShaderStartIntensity will always result in Conductor.instance.songPosition, because it only runs when FlxG.sound.music != null, so this doesn't make any difference.

@Lasercar
Copy link
Author

Lasercar commented Feb 1, 2025

Ah, so you fixed it by doing this,
var remappedIntensityValue:Float = rainShaderStartIntensity;
and putting the remap into a check if the song's playing

where as I did this: Conductor.instance.songPosition > 0 ? Conductor.instance.songPosition : rainShaderStartIntensity

And by making/adding your change, I basically made my change redundant, even though they fix the same thing.

Now I get it.

Whoops.

@Hundrec
Copy link
Collaborator

Hundrec commented Feb 1, 2025

Thanks for the clarification. I'm going to close this PR as a duplicate as it is redundant to AppleHair's solution.

@Hundrec Hundrec added status: duplicate Issue or PR is redundant to another. and removed type: minor bug Involves a minor bug or issue. status: pending triage The bug or PR has not been reviewed yet. labels Feb 1, 2025
@Hundrec Hundrec closed this Feb 1, 2025
@Lasercar Lasercar deleted the weekend1-rain-start-fix branch February 1, 2025 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate Issue or PR is redundant to another.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants