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

Limit number of allowed beatmap combo colours to 8 #32110

Merged
merged 5 commits into from
Feb 27, 2025

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Feb 26, 2025

Closes #31936.

Matching stable.

@bdach bdach added area:beatmap-parsing .osu file format parsing area:editor labels Feb 26, 2025
@bdach bdach self-assigned this Feb 26, 2025
@bdach bdach marked this pull request as ready for review February 26, 2025 10:26
Comment on lines +131 to +133
bool isCombo = pair.Key.StartsWith(@"Combo", StringComparison.Ordinal)
&& int.TryParse(pair.Key[5..], out int comboIndex)
&& comboIndex >= 1 && comboIndex <= MAX_COMBO_COLOUR_COUNT;
Copy link
Collaborator Author

@bdach bdach Feb 26, 2025

Choose a reason for hiding this comment

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

By the way, I am choosing not to notice that none of this parsing logic actually checks that the combo colours are in order and just parses them sequentially without examining the number in the key, because it's very annoying to handle and it's been the case before I came here. Until someone invariably reports that as an issue, that is.

@smoogipoo smoogipoo requested a review from peppy February 27, 2025 02:41
smoogipoo
smoogipoo previously approved these changes Feb 27, 2025
Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

Seems reasonable

@peppy peppy force-pushed the combo-colour-count-limit branch from d305153 to 0b45377 Compare February 27, 2025 06:16
@peppy
Copy link
Member

peppy commented Feb 27, 2025

Wasn't a huge fan of the button disappearing, so made it disable instead @bdach please double check.

@bdach
Copy link
Collaborator Author

bdach commented Feb 27, 2025

Changes look fine, thanks 👍

@bdach bdach merged commit 79b737b into ppy:master Feb 27, 2025
7 of 10 checks passed
@bdach bdach deleted the combo-colour-count-limit branch February 27, 2025 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changed order of colors does not importing properly to stable from map created in lazer
3 participants