-
Notifications
You must be signed in to change notification settings - Fork 2.4k
(rejected)[ENCHANCEMENT] Additional Functionality for Playbars #4229
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
(rejected)[ENCHANCEMENT] Additional Functionality for Playbars #4229
Conversation
Hey, allow me to welcome you for making your first PR! Though usually, when you have an issue with an existing PR, you simply request changes on it, rather than making your own that makes the change, plus doing more. I'd make this PR only do the additional changes you've made, and make a PR on my branch for your changes, to mine or suggest changes to it. |
@Lasercar Oh ok, i could make it as addition to yours |
I guess I need to finish this branch and try to PR to yours? |
Yeah, but keep your additional changes - adding the same functionality to note snap and song remaining time, separate, as I had no part in doing that, ofc. |
Actually, no, I'll take the loss on this one, there's no way to separate the changes cleanly. Don't worry about doing that now, I've closed my PR. You can edit the descriptions of your PRs to remove the part about the conflict with mine. |
@Lasercar Oh damn, I guess it will be easier, But I actually used your solution with toolbar.selected, so you are made the contribution with it (Without it I wouldn't have a single idea). I just will change to "made on the basis of" your PR. Ok? |
Sure! I'm a little surprised that you learned off my code and figured out a better implementation. That's never happened to me before! |
done |
Oops! Be sure to rebase your PR on the |
oh yea i sure did a mess. |
Ahhhh, when you're getting the new changes, always add I'd probably reset it to 1dca768 and then pull --rebase the develop branch, that should hopefully fix all this. |
d8f5b13
to
d86fe68
Compare
You can still fix this, you just have to cherry pick your commits back.
so, that'd be
Might have to fetch them from your fork first though? For example: |
on repo it is fixed but pr is broken. i will later create another pr with same content later once i will get how to manage them more correctly. |
also thanks Lasercar. damn, i would be in loss without your help |
Oh, I had a look at it, uh There's a double up of the commits Also, whenever you update your develop and/or main branch on your fork to match the upstream, you still have to do it for your local (aka computer) branch, idk if you already know that or not.
That'll hard reset it to Trofem@6b9f8d0 Hmm, but that merge commit that'd still be included in changes though, so maybe reset back to Trofem@edb270d instead? And then When you get conflicts and have to merge it, you basically want to accept the changes that aren't overriding your stuff, and then accept your changes so that they don't disappear. If you get both in the same position, then you'll have to fix the conflict manually. rebasing is basically like a regular merge but it removes commits that don't exactly change anything (depending on how you fix the conflicts, that is), and doesn't create a merge commit. |
This pull request is a duplicate. Please direct all discussion to the original pull request. |
INGNORE ALL THIS, moved to #4861
Does this PR close any issues? If so, link them below.
Fixes #4141
Briefly describe the issue(s) fixed.
Enhancement:
This PR adds turning on/off logic for all playbars:
Difficulty (playbarDifficulty)
BPM (playbarBPM)
Note Snap (playbarNoteSnap)
Song Remaining Time (playbarSongRemaining) (new toolbox function) (hovering color added)
Also:
( #132 funkin.assets ).
Include any relevant screenshots or videos.
23_-_03_2025_18-53-55.mp4