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

[BUGFIX/ENHANCEMENT] Sustain trail alpha fix #2893

Closed

Conversation

Itsgithubfellow
Copy link

Sustain trail alpha would always default to 1 regardless of what alpha value was defined. Simply moving the alpha value into update() fixes this issue. This is similar to pre-rewrite sustain trails which were also semi-transparent.

@Itsgithubfellow Itsgithubfellow changed the title [BUG FIX/ENHANCEMENT] Sustain trail alpha fix [BUGFIX/ENHANCEMENT] Sustain trail alpha fix Jun 23, 2024
@EliteMasterEric EliteMasterEric added type: minor bug Involves a minor bug or issue. status: pending triage The bug or PR has not been reviewed yet. labels Jun 23, 2024
@nebulazorua
Copy link
Contributor

nebulazorua commented Jun 24, 2024

Why do it in the update? Also why remove the existing alpha = 1.0?
Are we sure it's a bug and not an intentional visual change? Commented out code suggests that's the case (the aforementioned removed alpha = 1.0 with the commented alpha = 0.6)

The code suggests to me that it was meant to become fully opaque when you hit the hold, similar to other VSRGs which make held holds lighter (ITG immediately springs to mind)

Maybe I'm wrong? Personally I'd just make it alpha in the create tho, not the update

@Itsgithubfellow
Copy link
Author

Why do it in the update? Also why remove the existing alpha = 1.0? Are we sure it's a bug and not an intentional visual change? Commented out code suggests that's the case (the aforementioned removed alpha = 1.0 with the commented alpha = 0.6)

The code suggests to me that it was meant to become fully opaque when you hit the hold, similar to other VSRGs which make held holds lighter (ITG immediately springs to mind)

Maybe I'm wrong? Personally I'd just make it alpha in the create tho, not the update

I tried editing the existing alpha = 1.0 and it did not change anything, also I believe if this was intentional Eric would've rejected this like he did with another PR that wanted to bring back the old hold anim system.

@lemz1
Copy link
Contributor

lemz1 commented Jun 24, 2024

I tried editing the existing alpha = 1.0 and it did not change anything

in Strumline.hx there is this line: note.holdNoteSprite.alpha = 1.0;
this is why changing it in create didnt do anything
you would need to change this to 0.6

Are we sure it's a bug and not an intentional visual change? Commented out code suggests that's the case (the aforementioned removed alpha = 1.0 with the commented alpha = 0.6)

Maybe @FunkinCrew considers using the old alpha value, which could be why @EliteMasterEric didn't reject this.

@Itsgithubfellow
Copy link
Author

Oh, I never saw that LOL, woops...
I guess that would be a better fix, I'm not sure what performance issues having `alpha = 0.6' in update would bring, if any.

@Itsgithubfellow
Copy link
Author

Just tested it out, it only applies the alpha when the note is being pressed, even when the alpha is changed in SustainTrail.hx

2024-06-24.19-58-00.mp4

@lemz1
Copy link
Contributor

lemz1 commented Jun 25, 2024

then you will have to look for all lines of code that change holdNotesprite.alpha. Probably most of it will be found in strumline and playstate

@anysad
Copy link
Contributor

anysad commented Jun 25, 2024

Just tested it out, it only applies the alpha when the note is being pressed

maybe you forgot to change the alpha in buildHoldNoteSprite() function in Strumline.hx?
image

@anysad
Copy link
Contributor

anysad commented Jun 25, 2024

2024-06-25.18-31-29.mp4

if you change it to the desired 0.6 alpha, then it becomes just like in the good old days

@Itsgithubfellow
Copy link
Author

Oh wow, that is a large oversight on my part. I thought everything would've been handled within SustainTrail.hx, good catch, and a WAYY better fix than mine

@EliteMasterEric EliteMasterEric changed the base branch from main to develop July 10, 2024 22:16
@EliteMasterEric EliteMasterEric added status: reviewing internally This PR is under internal review and quality assurance testing type: optimization Involves a performance issue or a bug which causes lag. small A small pull request with 10 or fewer changes and removed type: minor bug Involves a minor bug or issue. status: pending triage The bug or PR has not been reviewed yet. labels Jul 10, 2024
@EliteMasterEric EliteMasterEric self-assigned this Jul 10, 2024
@EliteMasterEric
Copy link
Member

which could be why @EliteMasterEric didn't reject this.

For the record, status: pending triage means I haven't looked into an issue, I have been just tagging issues as they come in because the issue template isn't doing that for me right now.

I messaged Dave about it but my recollection is that opaque holds were an intentional change. I will definitely merge the other PR that fixes the issue with mods changing opacity regardless.

@EliteMasterEric
Copy link
Member

Yeah the opacity change was intentional.

@EliteMasterEric EliteMasterEric added status: rejected Issue did not pass review or PR cannot be approved. and removed status: reviewing internally This PR is under internal review and quality assurance testing labels Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
small A small pull request with 10 or fewer changes status: rejected Issue did not pass review or PR cannot be approved. type: optimization Involves a performance issue or a bug which causes lag.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants