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' setting for RateAdjust mods #11887

Closed
wants to merge 1 commit into from

Conversation

H2n9
Copy link
Contributor

@H2n9 H2n9 commented Feb 23, 2021

As discussed in #11878, an adjust pitch setting has been added to ModRateAdjust. Nightcore and Daycore currently has this setting disabled.

Closes #9958

@H2n9 H2n9 mentioned this pull request Feb 23, 2021
1 task
@bdach
Copy link
Collaborator

bdach commented Feb 23, 2021

Depending on a framework pull that has been abandoned for 3 months and doesn't even pass tests doesn't bode well. You'd probably need to get @frenzibyte to actually finish that one. That aside I'm not sure if there's any value in having the "adjust pitch" thing show up for day/nightcore, if it'll just be disabled. I'd probably just nuke it altogether and only define it for DT/HT.

Aside from that this will need a web-side change in its current state. This setting should probably get sent to web for replays' sake, but I suppose it could be argued that it's purely a user-side thing (similar to #11200) and should not get sent to web. If we're going for the latter, that'll be another blocker.

@H2n9
Copy link
Contributor Author

H2n9 commented Feb 23, 2021

Depending on a framework pull that has been abandoned for 3 months and doesn't even pass tests doesn't bode well...

Regrettably that whole issue is way out of my league or I would of looked into it myself. I was hoping that considering there was still some more recent activity on other dependant framework PRs that some progress was still being made.

I guess its probably best I try to move forward with below then to avoid the whole issue.

That aside I'm not sure if there's any value in having the "adjust pitch" thing show up for day/nightcore, if it'll just be disabled. I'd probably just nuke it altogether and only define it for DT/HT.

I mainly avoided this as it requires some more extensive changes. As it stands, NC inherits from DT. So moving the whole adjust pitch logic out of ModRateAdjust and into DT while making NC inherit directly from ModRateAdjust is an option. Only concern then would be that Adjust Pitch logic would exist separately in DT/HT.
Maybe an in-between class like ModRateAdjustWithPitch (class names clearly not my strong point) to share the logic is another option.

@bdach
Copy link
Collaborator

bdach commented Feb 23, 2021

I wouldn't necessarily be opposed to removing the double time -> nightcore inheritance. As it can be seen with this PR the two begin to diverge in some ways. That said I'd prefer to get more opinions on the preferred way forward first.

@peppy
Copy link
Member

peppy commented Feb 24, 2021

If removing the inheritance fixes the underlying issue, I don't see a huge reason not to do that. As mentioned, they are kind of taking their own path as mods. The inheritance was only there as a convenience thing for as long as it was valid.

@SysError99
Copy link

Winded Mode with same speed seems also do the trick, but still agree that it should be a separated option.

@smoogipoo
Copy link
Contributor

Closing as stale (1 year old), with framework-side dependency closed, and I don't think this is high priority enough to be kept open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

adjust pitch option for DT and HT
6 participants