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

Allow plays with touch device mod to be submitted in multiplayer regardless of required/allowed mods #10704

Merged
merged 6 commits into from
Nov 9, 2023

Conversation

bdach
Copy link
Contributor

@bdach bdach commented Nov 2, 2023

Companion piece to ppy/osu#25348

Opening as draft as I'm not sure the direction will stick.

The general problem is thus: we want to distinguish scores with the touch device mod. For now, regardless of whether we want to specify that mod as required or allowed in a playlist room, the mod must be accepted at submission time even if the Touch Device mod is not required or allowed. So it gets it own special flag AlwaysValidForSubmission to avoid hardcoding the acronym, and that gets checked and excluded at score completion time to ensure that submission can go through.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were one-off manually generated from ppy/osu#25348 and an un-PRed osu-tools branch (ppy/osu-tools@master...bdach:osu-tools:mods-command-new-flag). I did it this way because there wasn't clear consensus on discord whether this should be a hardcode or a flag, so I basically picked one I liked better to force the issue. I'll PR the osu-tools change if this gets accepted.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the plan here? get this PR merged first or wait for the tools etc to be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the web change should be merged and deployed before game-side for 100% correctness. I'll PR the tools branch now but it is not urgent.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just need to remember to not have the changes here gone by unrelated release, I guess 🤔

@bdach bdach self-assigned this Nov 3, 2023
@bdach bdach marked this pull request as ready for review November 6, 2023 09:21
@@ -112,6 +112,19 @@ public function assertValidForMultiplayer(int $rulesetId, array $ids, bool $isRe
}
}

public function excludeModsAlwaysValidForSubmission(int $rulesetId, array $ids): array
{
$this->validateSelection($rulesetId, $ids);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can assume the array is already valid at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"monkey see, monkey do, monkey no understand" moment. Will remove.

Comment on lines 121 to 125
return collect($ids)
->filter(function ($id) use ($rulesetMods) {
return !$rulesetMods[$id]['AlwaysValidForSubmission'];
})
->all();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use collect here. A plain array_values(array_filter( works.

Also fn ($id) => $... is simpler and doesn't need use (...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"idiot that doesn't write php tries to write php" moment. Will apply in a sec.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the plan here? get this PR merged first or wait for the tools etc to be updated?

@nanaya nanaya merged commit 2cf908d into ppy:master Nov 9, 2023
3 checks passed
@bdach bdach deleted the touch-device-allowance branch November 9, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants