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

Remove "Special" ManiaActions for center columns #29342

Merged
merged 8 commits into from
Aug 13, 2024

Conversation

cl8n
Copy link
Member

@cl8n cl8n commented Aug 7, 2024

my actual motivation for this change is that it builds up to making #25936 possible, but I believe that it can be motivated on its own as well

currently, center columns in odd-keycount mania stages are bound to one of two "Special" ManiaActions, instead of normal "Keys". I suspect that this was originally done because in some ways the center columns do get distinct behavior, as in skinning and beatmap conversion. however for the purpose of keybinds there is no reason to define these as "Special", all it's doing here is convoluting the code and probably causing confusion down the line when adding support for SpecialStyle proper (which has nothing to do with center columns as far as keybinds go).

in this PR I remove the two Special ManiaActions and remove the enum offset for the other Keys there. the rest of the PR is fixing everything to work with the new enum values, including the realm migration, which is required to not break existing mania keybinds

this PR was intended to have no user-facing changes but there ended up being one: in the keybinds menu, keys are now labeled according to the new enum, so "Special 1" for example is gone and replaced by Keys with shifted numbers. I see this as a good thing because it felt weird to not actually have the amount of "Key" binds indicated by the key count (e.g. 7K used to end at Key 6)

also I wanted to add a test for the migration but I couldn't figure out how to set it up. I confirmed that it works though with odd and even keycounts and single and dual stages

@peppy
Copy link
Member

peppy commented Aug 8, 2024

On a surface level I agree with this direction. Removing as much complexity as we can from mania is in the best interest.

Special columns had more meaning when the primary "mania" game mode was IIDX/beatmania where each and every player and mappter knew what the special (scratch) column was. That has changed over time.

@peppy peppy self-requested a review August 12, 2024 05:39
[Description("Key 1")]
Key1 = 10,
Key1,
Copy link
Member

Choose a reason for hiding this comment

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

If we wanted to make the enum match the key number, we could do this

Suggested change
Key1,
Key1 = 1,

Copy link
Member

Choose a reason for hiding this comment

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

Applied this via 952a73b (smoogi agrees to it).

Copy link
Member

@peppy peppy Aug 13, 2024

Choose a reason for hiding this comment

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

Or not, something relies on zero index somewhere who knows. Was hoping this would be agreed upon before committing to this.

@peppy peppy requested a review from a team August 12, 2024 05:42
@bdach
Copy link
Collaborator

bdach commented Aug 12, 2024

however for the purpose of keybinds there is no reason to define these as "Special", all it's doing here is convoluting the code and probably causing confusion down the line when adding support for SpecialStyle proper (which has nothing to do with center columns as far as keybinds go).

I am skeptical that this assessment is correct because of replays. I assume replays are expected to still play back correctly even if you switch the special column to be on the left or right (in that "special column" inputs should move with the column), but I am not sure this can be upheld if the notion of "special column" is removed.

I would probably need to cross-check stable but my hunch is that this PR breaks replay stuff. So it would be helpful to receive assurances that it does not.

@cl8n
Copy link
Member Author

cl8n commented Aug 12, 2024

however for the purpose of keybinds there is no reason to define these as "Special", all it's doing here is convoluting the code and probably causing confusion down the line when adding support for SpecialStyle proper (which has nothing to do with center columns as far as keybinds go).

I am skeptical that this assessment is correct because of replays. I assume replays are expected to still play back correctly even if you switch the special column to be on the left or right (in that "special column" inputs should move with the column), but I am not sure this can be upheld if the notion of "special column" is removed.

this PR does not affect playback of existing replays, but you are right that replays may need to do something different when "SpecialStyle: Right" support is added. something I want to clarify is that the "Special" actions removed in this PR aren't in any way related to the behavior created by "SpecialStyle: Left" or "SpecialStyle: Right" in stable, the actions as used on center columns were completely new in lazer, and so far they haven't done anything user-facing (see OP for the one exception). I think it was an oversight to associate the "Special"-ness of the center column on "SpecialStyle: None" with keybinds and actions, and this PR hopefully corrects that & simplifies things as a result

smoogipoo
smoogipoo previously approved these changes Aug 13, 2024
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.

lgtm

@smoogipoo smoogipoo dismissed their stale review August 13, 2024 05:21

want to mention something

@smoogipoo
Copy link
Contributor

In terms of special styles/the special key, the important consideration to be had is for nK+1.

Basically, a map is made in 8K. The creator then selects "SpecialStyle" in the beatmap settings, indicating that it's intended to be played bemani-style with a scratch key (usually to the left or right of the stage). The player's skin then decides where the scratch key is to be placed, with a similarly named "SpecialStyle" skin setting.

When special style is active, the left-most column becomes the special key but can also move to the right, bringing all hitobjects along with it.

How this is going to be done is a bit up in the air right now, but I would like to clarify the path forward before merging this as it may make it even more complicated to handle.


I believe that keybinding-wise we either want two sections (8K and 7K+1), or a "special key" bind as part of the 8K section.

In the case of two sections (1), maybe we can handle this with a PlayfieldType variant? Right now we have Single=0 and Dual=1000, but maybe we can do it by a Special=500 or so? And DualSpecial=1500? I'm not sure about this path.

In the case of a special key (2), how do you actually represent this as a binding? What I mean is how is this going to be signalled to the settings panel?


Curious how others think this should work - will we have a bunch of custom keybinding panel stuff just for mania or something?

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.

lgtm

@smoogipoo smoogipoo merged commit 4b4f0ec into ppy:master Aug 13, 2024
11 of 13 checks passed
@peppy
Copy link
Member

peppy commented Aug 13, 2024

I've asked for some initial feedback on usage of special key since I have little idea how much of the community (if any) care about it these days.

I'd only want to support this to the level we need to in order to keep people happy.

@cl8n cl8n deleted the remove-mania-action-special branch August 13, 2024 08:48
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.

4 participants