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

Always inherit the volume from the previous hit object on placement #28728

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

OliBomby
Copy link
Contributor

@OliBomby OliBomby commented Jul 3, 2024

Before this PR the next hit object you place would only inherit volume from the previous hit object if you have the 'Auto' bank selected. If you had any other bank selected, the volume would default to 100.
Now I made it always inherit volume from the previous hit object. I think this better matches the user's expectation and mimics stable behaviour.

@bdach bdach self-requested a review July 4, 2024 12:26
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 4, 2024
@bdach
Copy link
Collaborator

bdach commented Jul 4, 2024

Behaviourally, I agree that this is the correct change.

From a code quality standpoint, I refactored this slightly to reduce code duplication.

From a maintenance standpoint, if you submit multiple PRs that touch the same file, I'd ask that each individual change include test coverage to ensure no interference between them breaks them in the process. I have added tests and will continue to do so as I go through this series, but something to note for the future I guess.

@OliBomby
Copy link
Contributor Author

OliBomby commented Jul 4, 2024

From a maintenance standpoint, if you submit multiple PRs that touch the same file, I'd ask that each individual change include test coverage to ensure no interference between them breaks them in the process. I have added tests and will continue to do so as I go through this series, but something to note for the future I guess.

Alright I'll remind myself to add tests even for small changes

@bdach bdach merged commit ea9dd84 into ppy:master Jul 4, 2024
11 of 17 checks passed
@OliBomby OliBomby deleted the inherit-volume branch July 4, 2024 13:26
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.

2 participants