Don't fire single-button mappings while a combo using them is held

If you map something to L2+R2, the mappings for L2 and R2 on their own
would fire as well. Now, while a combo mapping is fully held, the
shorter mappings that share an input with it are suppressed - longest
match wins. Releasing part of the combo brings the shorter mappings
back, for the inputs that are still held.

Adds a ControlMapper unit test covering the sequence.

Fixes #20621

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JvJR8oJNSCimCM9KXVLjfq

Core/ControlMapper.cpp

@@ -391,6 +391,93 @@ void ControlMapper::SwapMappingIfEnabled(uint32_t *vkey) {
 	}
 }
 
+// Works out which mappings are currently being overridden by a longer one. If you map something
+// to L2+R2, you don't want whatever L2 and R2 are mapped to on their own to fire as well, so while
+// a combo is fully held, the shorter mappings sharing an input with it are suppressed.
+// mutex_ should be locked, and also KeyMap::LockMappings().
+void ControlMapper::UpdateComboSuppression() {
+	if (KeyMap::HasChanged(comboMappingsGeneration_)) {
+		KeyMap::GetAllComboMappingsNoLock(&comboMappings_);
+	}
+
+	comboSuppressionChanged_.clear();
+	if (comboMappings_.empty() && comboSuppression_.empty()) {
+		// By far the common case - nobody has mapped a combo, so there's nothing to suppress.
+		return;
+	}
+
+	std::map<InputMapping, size_t> prevSuppression = std::move(comboSuppression_);
+	comboSuppression_.clear();
+
+	for (const auto &combo : comboMappings_) {
+		// Is every input of the combo held down? Same conditions as the main loops below.
+		bool all = true;
+		double curTime = 0.0;
+		for (const auto &mapping : combo.mappings) {
+			auto iter = curInput_.find(mapping);
+			if (iter == curInput_.end()) {
+				all = false;
+				break;
+			}
+			// Stop reverse ordering from triggering.
+			if (g_Config.bStrictComboOrder && iter->second.timestamp < curTime) {
+				all = false;
+				break;
+			}
+			curTime = iter->second.timestamp;
+			if (iter->second.value <= 0.0f || iter->second.value <= GetDeviceAxisThreshold(iter->first.deviceId, mapping)) {
+				all = false;
+				break;
+			}
+		}
+		if (!all) {
+			continue;
+		}
+		// It is, so record it as the one to beat for each of its inputs.
+		for (const auto &mapping : combo.mappings) {
+			size_t &longest = comboSuppression_[mapping];
+			longest = std::max(longest, combo.mappings.size());
+		}
+	}
+
+	// Outputs are only re-evaluated when an input they use has changed, so when suppression
+	// starts or stops for an input, we have to treat that input as changed too. Otherwise
+	// releasing one button of a held combo wouldn't bring back what the others map to alone.
+	for (const auto &[mapping, size] : comboSuppression_) {
+		auto iter = prevSuppression.find(mapping);
+		if (iter == prevSuppression.end() || iter->second != size) {
+			comboSuppressionChanged_.push_back(mapping);
+		}
+	}
+	for (const auto &[mapping, size] : prevSuppression) {
+		if (!comboSuppression_.count(mapping)) {
+			comboSuppressionChanged_.push_back(mapping);
+		}
+	}
+}
+
+bool ControlMapper::SuppressionChanged(const KeyMap::MultiInputMapping &multiMapping) const {
+	for (const auto &changed : comboSuppressionChanged_) {
+		if (multiMapping.mappings.contains(changed)) {
+			return true;
+		}
+	}
+	return false;
+}
+
+bool ControlMapper::IsSuppressedByCombo(const KeyMap::MultiInputMapping &multiMapping) const {
+	if (comboSuppression_.empty()) {
+		return false;
+	}
+	for (const auto &mapping : multiMapping.mappings) {
+		auto iter = comboSuppression_.find(mapping);
+		if (iter != comboSuppression_.end() && multiMapping.mappings.size() < iter->second) {
+			return true;
+		}
+	}
+	return false;
+}
+
 // Can only be called from Key or Axis.
 // mutex_ should be locked, and also KeyMap::LockMappings().
 // TODO: We should probably make a batched version of this.
@@ -406,6 +493,8 @@ bool ControlMapper::UpdatePSPState(const InputMapping &changedMapping, double no
 	case ROTATION_LOCKED_VERTICAL180:   rotations = 3; break;
 	}
 
+	UpdateComboSuppression();
+
 	// For the PSP's digital button inputs, we just go through and put the flags together.
 	uint32_t buttonMask = 0;
 	uint32_t changedButtonMask = 0;
@@ -429,7 +518,7 @@ bool ControlMapper::UpdatePSPState(const InputMapping &changedMapping, double no
 		// If a mapping could consist of a combo, we could trivially check it here.
 		for (auto &multiMapping : inputMappings) {
 			// Check if the changed mapping was involved in this PSP key.
-			if (multiMapping.mappings.contains(changedMapping)) {
+			if (multiMapping.mappings.contains(changedMapping) || SuppressionChanged(multiMapping)) {
 				changedButtonMask |= mask;
 			}
 			// Check if all inputs are "on".
@@ -452,7 +541,7 @@ bool ControlMapper::UpdatePSPState(const InputMapping &changedMapping, double no
 				if (!down)
 					all = false;
 			}
-			if (all) {
+			if (all && !IsSuppressedByCombo(multiMapping)) {
 				buttonMask |= mask;
 			}
 		}
@@ -484,10 +573,14 @@ bool ControlMapper::UpdatePSPState(const InputMapping &changedMapping, double no
 		bool touchedByMapping = false;
 		float value = 0.0f;
 		for (auto &multiMapping : inputMappings) {
-			if (multiMapping.mappings.contains(changedMapping)) {
+			if (multiMapping.mappings.contains(changedMapping) || SuppressionChanged(multiMapping)) {
 				touchedByMapping = true;
 			}
 
+			if (IsSuppressedByCombo(multiMapping)) {
+				continue;
+			}
+
 			float product = 1.0f;  // We multiply the various inputs in a combo mapping with each other.
 			double curTime = 0.0;
 			for (auto mapping : multiMapping.mappings) {

Core/ControlMapper.h

@@ -73,6 +73,9 @@ public:
 private:
 	void UpdateSwapAxes();
 	bool UpdatePSPState(const InputMapping &changedMapping, double now);
+	void UpdateComboSuppression();
+	bool IsSuppressedByCombo(const KeyMap::MultiInputMapping &multiMapping) const;
+	bool SuppressionChanged(const KeyMap::MultiInputMapping &multiMapping) const;
 	float MapAxisValue(float value, int vkId, const InputMapping &mapping, const InputMapping &changedMapping, bool *oppositeTouched);
 	void SwapMappingIfEnabled(uint32_t *vkey);
 
@@ -124,6 +127,17 @@ private:
 
 	std::map<InputMapping, InputSample> curInput_;
 
+	// While a combo mapping is fully held, the shorter mappings that its inputs also belong to are
+	// suppressed - see UpdateComboSuppression. Maps an input to the size of the longest satisfied
+	// combo it takes part in, so a mapping is suppressed if it's shorter than that.
+	std::map<InputMapping, size_t> comboSuppression_;
+	// Every combo mapping in the keymap. Cached, since scanning them all isn't free and the
+	// mappings only change when the user edits them.
+	std::vector<KeyMap::MultiInputMapping> comboMappings_;
+	int comboMappingsGeneration_ = -1;
+	// Inputs whose suppression state changed in the current update, see UpdateComboSuppression.
+	std::vector<InputMapping> comboSuppressionChanged_;
+
 	// Callbacks
 	std::vector<ControlListener *> listeners_;
 };

Core/KeyMap.cpp

@@ -614,6 +614,17 @@ bool InputMappingToPspButton(const InputMapping &mapping, std::vector<int> *pspB
 	return found;
 }
 
+void GetAllComboMappingsNoLock(std::vector<MultiInputMapping> *combos) {
+	combos->clear();
+	for (const auto &iter : g_controllerMap) {
+		for (const auto &mapping : iter.second) {
+			if (mapping.mappings.size() > 1) {
+				combos->push_back(mapping);
+			}
+		}
+	}
+}
+
 // This is the main workhorse of the ControlMapper.
 bool InputMappingsFromPspButtonNoLock(int btn, std::vector<MultiInputMapping> *mappings, bool ignoreMouse) {
 	auto iter = g_controllerMap.find(btn);

Core/KeyMap.h

@@ -186,6 +186,8 @@ namespace KeyMap {
 
 	// Careful with these.
 	bool InputMappingsFromPspButtonNoLock(int btn, std::vector<MultiInputMapping> *keys, bool ignoreMouse);
+	// Collects every mapping that consists of more than one input, ie. a combo.
+	void GetAllComboMappingsNoLock(std::vector<MultiInputMapping> *combos);
 	void LockMappings();
 	void UnlockMappings();
 

unittest/UnitTest.cpp

@@ -106,6 +106,8 @@
 #include "Core/FileSystems/ISOFileSystem.h"
 #include "Core/MemMap.h"
 #include "Core/KeyMap.h"
+#include "Core/ControlMapper.h"
+#include "Core/HLE/sceCtrl.h"
 #include "Core/Util/PathUtil.h"
 #include "Core/MIPS/MIPSVFPUUtils.h"
 #include "GPU/Common/TextureDecoder.h"
@@ -2446,6 +2448,77 @@ bool TestInputMapping() {
 	return true;
 }
 
+// Records what the ControlMapper tells us, so a test can check it.
+class TestControlListener : public ControlListener {
+public:
+	void OnVKey(VirtKey vkey, bool down) override {
+		vkeyDown[vkey] = down;
+	}
+	void UpdatePSPButtons(uint32_t buttonMask, uint32_t changedMask) override {
+		buttons = (buttons & ~changedMask) | buttonMask;
+	}
+	uint32_t buttons = 0;
+	std::map<VirtKey, bool> vkeyDown;
+};
+
+static bool SendKey(ControlMapper *mapper, int keyCode, bool down) {
+	KeyInput key{};
+	key.deviceId = DEVICE_ID_PAD_0;
+	key.keyCode = (InputKeyCode)keyCode;
+	key.flags = down ? KeyInputFlags::DOWN : KeyInputFlags::UP;
+	return mapper->Key(key);
+}
+
+// A mapping shouldn't fire when a longer mapping sharing an input with it is held. See #20621.
+bool TestComboSuppression() {
+	using KeyMap::MultiInputMapping;
+
+	InputMapping a(DEVICE_ID_PAD_0, NKCODE_BUTTON_1);
+	InputMapping b(DEVICE_ID_PAD_0, NKCODE_BUTTON_2);
+
+	KeyMap::ClearAllMappings();
+	KeyMap::SetInputMapping(CTRL_CIRCLE, MultiInputMapping(a), true);
+	KeyMap::SetInputMapping(CTRL_SQUARE, MultiInputMapping(b), true);
+	MultiInputMapping combo(a);
+	combo.mappings.push_back(b);
+	KeyMap::SetInputMapping(VIRTKEY_PAUSE, combo, true);
+
+	TestControlListener listener;
+	ControlMapper mapper;
+	mapper.AddListener(&listener);
+
+	// A on its own presses Circle.
+	SendKey(&mapper, NKCODE_BUTTON_1, true);
+	EXPECT_EQ_INT((int)(listener.buttons & CTRL_CIRCLE), (int)CTRL_CIRCLE);
+	EXPECT_FALSE(listener.vkeyDown[VIRTKEY_PAUSE]);
+
+	// Adding B completes the combo, so Circle lets go and Square never presses.
+	SendKey(&mapper, NKCODE_BUTTON_2, true);
+	EXPECT_TRUE(listener.vkeyDown[VIRTKEY_PAUSE]);
+	EXPECT_EQ_INT((int)(listener.buttons & CTRL_CIRCLE), 0);
+	EXPECT_EQ_INT((int)(listener.buttons & CTRL_SQUARE), 0);
+
+	// Letting go of B ends the combo, and since A is still held, Circle comes back.
+	SendKey(&mapper, NKCODE_BUTTON_2, false);
+	EXPECT_FALSE(listener.vkeyDown[VIRTKEY_PAUSE]);
+	EXPECT_EQ_INT((int)(listener.buttons & CTRL_CIRCLE), (int)CTRL_CIRCLE);
+	EXPECT_EQ_INT((int)(listener.buttons & CTRL_SQUARE), 0);
+
+	// And releasing A leaves nothing pressed.
+	SendKey(&mapper, NKCODE_BUTTON_1, false);
+	EXPECT_EQ_INT((int)(listener.buttons & (CTRL_CIRCLE | CTRL_SQUARE)), 0);
+
+	// B on its own still presses Square - suppression only applies while the combo is held.
+	SendKey(&mapper, NKCODE_BUTTON_2, true);
+	EXPECT_EQ_INT((int)(listener.buttons & CTRL_SQUARE), (int)CTRL_SQUARE);
+	EXPECT_FALSE(listener.vkeyDown[VIRTKEY_PAUSE]);
+	SendKey(&mapper, NKCODE_BUTTON_2, false);
+
+	mapper.RemoveListener(&listener);
+	KeyMap::ClearAllMappings();
+	return true;
+}
+
 bool TestEscapeMenuString() {
 	char c;
 	std::string temp = UnescapeMenuString("&File", &c);
@@ -3010,6 +3083,7 @@ TestItem availableTests[] = {
 	TEST_ITEM(FastVec),
 	TEST_ITEM(SmallDataConvert),
 	TEST_ITEM(InputMapping),
+	TEST_ITEM(ComboSuppression),
 	TEST_ITEM(EscapeMenuString),
 	TEST_ITEM(VFS),
 	TEST_ITEM(Substitutions),