Skip to content

Conversation

@LumpBloom7
Copy link
Contributor

KeyBindingContainers allow actions to be pressed down multiple times through separate bindings when SimultaneousBindingMode.All is used.

However, ReplayInputHandler did not take this into account. It will only generate an OnPressed event for the first press of the action, and will only generate an OnRelease when all presses have been released.

This change makes it so that input state changes occur not only based on whether the action exists, but also the number of occurrences of said action.

Before After
Before.mp4
After.mp4

@bdach
Copy link
Collaborator

bdach commented Feb 28, 2025

Not sure how to feel about any of this in general. The core problem here is that your ruleset is allowing more than one object to be hittable with the same binding at a time purposefully and our structures just falling over from that.

At the least if this is to pass review in my eyes, this needs a test case explaining what this change is for, and inline documentation next to the change spelling it out why Except() cannot be used (i.e. that it deduplicates multiple presses of the same action). And even then I wouldn't merge this myself with at least a second review from someone else.

@LumpBloom7
Copy link
Contributor Author

LumpBloom7 commented Feb 28, 2025

Considering that rulesets are allowed to choose their own SimultaneousBindingMode without restrictions, it would make sense that the replay system should accommodate any of them.

My ruleset having multiple hit objects at the same time is irrelevant to the problem, as I am simply taking advantage of the flexibility permitted from targeting touch devices, where there can be multiple fingers tapping the same receptor. If a human can perform a sequence of inputs, the replay system must be able to replicate it.

That said, I will add a test case and some documentation later.

EDIT: My aim was bad and I pressed the "close PR" button.

@LumpBloom7 LumpBloom7 closed this Feb 28, 2025
@LumpBloom7 LumpBloom7 reopened this Feb 28, 2025
@bdach
Copy link
Collaborator

bdach commented Feb 28, 2025

My ruleset having multiple hit objects at the same time is irrelevant to the problem

I wouldn't call it irrelevant entirely. In every single default ruleset of ours it doesn't matter how many times a binding is pressed, the end result is still the same. But in your case you don't want it to be.

It's a significant paradigm shift with subtle implications and I'm not sure how we should be handling it if at all.

@pull-request-size pull-request-size bot added size/L and removed size/S labels Feb 28, 2025
I wasn't really testing ReplayInputHandler, but just the
ReplayState.Apply()
@LumpBloom7 LumpBloom7 force-pushed the ReplayInputHandler_SimultaneousBindingMode.All branch from 46fa892 to f979954 Compare February 28, 2025 20:43
@LumpBloom7
Copy link
Contributor Author

I wrote the requested tests, testing how ReplayState.Apply should work with each of the three SimultaneousBindingModes permitted by KeyBindingContainer.

I had to comment out many assertions because the actions triggered by ReplayInputHandler doesn't follow the same rules as regular input, effectively bypassing anything related to input queues and the input behaviour differences from SimultaneousBindingMode. (Related o!f issue)

The Except() implementation from before just so happens to match closely with how SimultaneousBindingMode.Unique would handle input. With that in mind, I also have doubts on this change, since this may break compatibility with existing rulesets that used SimultaneousBindingMode.Unique. I'm not sure on the way forward from here.

If replays are supposed to encode the inputs of a player or autoplay, then it would make sense that ReplayState.PressedActions would be a representation of currently pressed bindings (with duplicates) made by players, abstracted via ruleset xActions. These can then go through the regular input flow where it'll handle the input based on the rules built into InputManager

@peppy
Copy link
Member

peppy commented Jul 18, 2025

I somewhat regret making the All mode since it doesn't seem like something I'd want to see used in a ruleset.

@LumpBloom7 for your use case here, can you explain how this works in the game? ie why you can't set things up like osu! where there's two or more bindings, one per "physical" key?

@bdach
Copy link
Collaborator

bdach commented Jul 18, 2025

My understanding was that, to use a crude analogy, it's like you would have mania 8k on touch screen, but also allow mappers to put concurrent objects in every column, and users could hit such concurrent objects by touching a column with multiple fingers at once.

@peppy
Copy link
Member

peppy commented Jul 18, 2025

Kind of breaks my head (the approach taken to make that work). I'd probably consider making distinct actions per finger and working forward that way.

@LumpBloom7
Copy link
Contributor Author

LumpBloom7 commented Jul 20, 2025

The concurrent hitobjects was just a very obvious example of how it would fail.

Most of the current included rulesets never encounter the issue, simply because they were designed with keyboard/mouse first.

Sentakki, being designed as touch-first, takes into consideration likely scenarios that can be encountered during gameplay. (Let's drop concurrent hitobjects for now, and simply focus on concurrent inputs.)

During gameplay, it is common that concurrent inputs are made, for example, a second touch input being made on the same lane prior to the first touch input being released, perhaps due to rapid sequence of notes like a jack. It makes sense to acknowledge the second input, instead of blocking it in such a scenario. (I think the mania touch sensors do just that?)

Similarly, hold notes should remain held until the last input to said lane is released. (the finger being there describes intent)

SimultaneousBindingMode.[Unique|None] doesn't make sense in such a scenario. I don't think it makes sense for each finger to be assigned a unique action for every lane. (8*(10+ fingers) is ridiculous).

The main issue here is that ReplayInputHandler is built to only accommodate rulesets that behave as if their primary input is keyboard and mouse. Where SimultaneousBindingMode.All is never used.

PS: many touchscreen/arcade rhythm games do specifically leverage multitouch capabilities, allowing concurrent hitobjects. So it's not like sentakki is the odd one out trying to be different for the sake of it.

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.

3 participants