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

Add an "Adjust pitch" switch to DT/HT #24640

Merged
merged 8 commits into from Oct 18, 2023
Merged

Conversation

Givikap120
Copy link
Contributor

Adjust pitch switch in DoubleTime and HalfTime mods.
Also Nightcore and Daycore is now inherit from RateAdjust.

Adjust pitch switch in DoubleTime and HalfTime mods
Also Nightcore and Daycore is now inherit from RateAdjust
@rszyma
Copy link

rszyma commented Oct 16, 2023

Sorry not sorry for bumping this. I'd love to see this finally added. Is there anything holding back this PR?

@peppy peppy self-requested a review October 17, 2023 07:04
@peppy
Copy link
Sponsor Member

peppy commented Oct 17, 2023

I'm still not sure I like what this does to the mod inheritance. Would request a second opinion.

@peppy peppy requested a review from a team October 17, 2023 07:06
@bdach
Copy link
Collaborator

bdach commented Oct 17, 2023

I mean the last time this was attempted (#11887), it tried to not do anything to the inheritance, but because ModNightcore : ModDoubleTime, adding a toggle to ModDoubleTime would add a toggle to ModNightcore, which wasn't wanted, which was attempted to be papered over by making the bindable virtual and overriding it to be disabled in ModNightcore, which in turn caused issues with bindable disable not working all that well and as such introduced a dependency on a beached framework PR.

So I dunno how else you'd do this at this point. It's a mess.

@peppy
Copy link
Sponsor Member

peppy commented Oct 17, 2023

Wondering if some of the logic can be split out into a helper method / class to reduce the code share..

@Givikap120
Copy link
Contributor Author

Wondering if some of the logic can be split out into a helper method / class to reduce the code share..

i think of changing old DT into like DT_Raw, putting all logic here
and then actual DT only will implement the pitch switch
same with HT
this will reduce the code share, but kinda overengineers a little

@peppy
Copy link
Sponsor Member

peppy commented Oct 18, 2023

I've tidied things up using a helper class with composition structure.

@bdach please double-check this.

peppy
peppy previously approved these changes Oct 18, 2023
@peppy peppy requested a review from bdach October 18, 2023 08:40
@peppy peppy changed the title Added a "Adjust pitch" switch to DT/HT Add an "Adjust pitch" switch to DT/HT Oct 18, 2023
@bdach
Copy link
Collaborator

bdach commented Oct 18, 2023

I pushed some inline comments (cff69d6) because I was initially confused why Mod{Day,Night}core had the helper but were only using it for the multiplier. Looks good otherwise as far as I can tell.

@bdach bdach merged commit 12f343f into ppy:master Oct 18, 2023
15 of 17 checks passed
bdach added a commit to bdach/osu-tools that referenced this pull request Oct 19, 2023
I guess this would be one way to roll forward with
ppy/osu#24640, but there is a chance something
somewhere else could have relied on the inheritance and might now break,
so not sure whether this should be the solution here...

Check is slightly rewritten since it wasn't making much sense to me
previously (why add nightcore to allowed mods if double time is
selected?)
MrHeliX pushed a commit to MrHeliX/osu-tools that referenced this pull request Oct 21, 2023
I guess this would be one way to roll forward with
ppy/osu#24640, but there is a chance something
somewhere else could have relied on the inheritance and might now break,
so not sure whether this should be the solution here...

Check is slightly rewritten since it wasn't making much sense to me
previously (why add nightcore to allowed mods if double time is
selected?)
@exformation
Copy link

exformation commented Feb 17, 2024

Isn't this inverted?

This sounds like chipmunks singing, but the opposite should be true.
image

If you only increase the playback rate of a song, without adjusting pitch, the pitch scales and sounds wacky. If you "adjust for pitch", the pitch should sound the same as 1x.

@frenzibyte
Copy link
Member

When turning on the Adjust pitch setting, the pitch of the track changes alongside the tempo. The way you're describing how the setting works sounds backwards to expectation. If you want to discuss this further, open a Q&A discussion thread.

@ppy ppy locked and limited conversation to collaborators Feb 17, 2024
@Givikap120 Givikap120 deleted the pitch_change branch February 21, 2024 19:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants