fix(gta-core-five): release held key state when a binding is destroyed - #4089
Open
Github-Samuel wants to merge 1 commit into
Open
fix(gta-core-five): release held key state when a binding is destroyed#4089Github-Samuel wants to merge 1 commit into
Github-Samuel wants to merge 1 commit into
Conversation
A `+command` binding takes a reference in `g_downSet` when its key goes down and gives it back when the key comes up. That reference belongs to the binding's `m_wasDown` state, but `~Binding` never gave it back, so a binding destroyed while its key was still held left the reference behind forever. Rebinding from the pause menu does exactly that within a single frame: `BindingManager::Update` dispatches the bindings first, which fires `+command` and takes the reference, and only then drains the queue where `Bind` erases the old binding. With one reference stuck in the set every later press finds the command already down and skips `+command`, and every release only removes one of the two references so `-command` never runs either. The bind stays dead until the game is restarted. Move the command dispatch into `Binding::RunCommands` and run the up path from the destructor when the binding is still held, so the reference is given back. This also covers the other two places a binding can be destroyed while down, unbinding a single key and clearing all bindings. Signed-off-by: Samuel Nicol <99494967+Github-Samuel@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal of this PR
Stop a keybind from dying permanently when it gets remapped while the key is being held down. Right now the
+commandfires but-commandnever does, so anything the bind toggles stays stuck on and the bind does nothing until the game is restarted.How is this PR achieving the goal
+commandbindings are reference counted throughg_downSet:static std::multiset<std::string, std::less<>> g_downSet;Binding::Updateinserts a reference when the key goes down and removes one when it comes up, only emitting+commandon the first reference and-commandon the last. That reference is owned by the binding'sm_wasDownstate, but~Bindingonly calledUnmap()and never gave it back.Remapping does exactly that inside one frame.
BindingManager::Updateruns the bindings first:so
+commandfires and takes a reference at the top, and the queuedBind()fromMapControlInternalruns at the bottom, where the old binding is erased whilem_wasDownis still true:The leftover reference is what kills the bind. Every later press finds the command already in the set and skips
+command, and every release removes one of the two references and still finds one, so-commandis skipped too.This PR splits the dispatch out of
UpdateintoBinding::RunCommandsand calls it from the destructor when the binding is still held:Nothing inside the dispatch changed, it is only moved. Using the existing path rather than erasing from
g_downSetdirectly keeps the;splitting and the tag handling identical for both callers.The same shape already exists on the raw keymap side, where
m_wasTriggeredis described as "used so we don't ignore key ups if the key was disabled while being pressed" incode/components/extra-natives-five/src/InputNatives.cpp.This also covers the two other places a binding can be destroyed while held,
unbindfor a single key andunbindall.This PR applies to the following area(s)
FiveM
Successfully tested on
Game builds: n/a, this is client input plumbing and does not depend on a game build
Platforms: Windows
Checklist
I could not do a full client build locally so I left the first box unchecked. What I did check is the reference counting itself, by taking the
g_downSetprotocol and the binding lifetime out into a small harness and running the reported sequence through it. Before the change the reference is left behind and every following press and release is swallowed, after it the reference is given back and the bind keeps working. Happy to adjust if you would rather see this solved somewhere other than the destructor.One thing I deliberately left alone: the
g_downSetbookkeeping sits inside theif (!ignore)branch, so a binding whose command is already gone (a resource stopping while its key is held) still leaks a reference. That is a separate case from this issue and it looked like it deserves its own change rather than being folded in here.Fixes issues
fixes #3793